miyagawa / AnyEvent-Redis

Asynchronous Redis client
http://search.cpan.org/dist/AnyEvent-Redis
41 stars 16 forks source link

Protocol::Redis for parsing Redis' messages #17

Open und3f opened 13 years ago

und3f commented 13 years ago

Hi.

AnyEvent::Redis::Protocol contain complicated AnyEvent::Handle-depend code for decoding Redis protocol. I had same problem in MojoX::Redis.

Protocol::Redis (https://github.com/und3f/protocol-redis) is a protocol parser independent from any event loop, designed specially for asynchronous Redis clients. It is covered by tests and could simplify AnyEvent::Redis::Protocol.

It is used in MojoX::Redis https://github.com/und3f/mojox-redis If you are interesting i'll help adopt AnyEvent::Redis::Protocol

Using same Redis protocol parser will help to unify Redis protocol in Perl world and make it community-driven.

dgl commented 13 years ago

Sounds interesting.

One of the reasons for the AE::Redis parser being rather complex is subroutine calls are rather slow in Perl, but I'd be interested to know how Protocol::Redis would compare, certainly it would be cleaner to use a parser like that. (I was also thinking of supporting hiredis via XS as an optional parser for speed at some point, so having a cleaner separation of protocol would make sense for that too).

vti commented 13 years ago

Making a protocol implementation separated from event loop, http client etc is always a good idea, since it encourages code reuse and eliminates wheel reinventing.

und3f commented 13 years ago

Having same Redis-parser interface we can optimize it and create XS versions. The main goal - redis parser will be available for all perl's redis clients

dgl commented 13 years ago

I just wrote a quick PoC of this in 80b75b4f2736614e3fc251e3ed6ee86746480eae.

The first test (client.t) does pass for me.

It seems like there's a few issues though, e.g. I get: Deep recursion on subroutine "Protocol::Redis::_change_state

Other tests such as multi.t don't pass though, seems like there's some issues in the parsing. See https://github.com/dgl/protocol-redis/commit/4bf2121882a0615bfe5f47b86e347bdb7fc6fbc3

und3f commented 13 years ago

Fixed that

otterley commented 13 years ago

I too think it's an interesting idea, but not at the cost of performance. If Protocol::Redis' parser is slower than AE::Redis::Protocol's hand-optimized parser, that's not a tradeoff I want to make.

Once Protocol::Redis becomes an XS module, I'd have a more favorable opinion. But I'd want to wait until then to depend on it.

und3f commented 13 years ago

According to benchmarks of dgl AE::Redis doesn't lose in speed with Protocol::Redis.

Protocol::Redis::XS is independent module, fully compatible with P::R API, so it can be used instead.

otterley commented 13 years ago

That sounds very promising, then. :)

dgl commented 13 years ago

Still needs a bit of work, but I've put Protocol::Redis::XS support in c376f7548f46dc701603a3e5fe8a210ecdf0b59e.

und3f commented 13 years ago

I've accidentally closed it, nice that new issues has reopen feature :)

und3f commented 13 years ago

Together with dgl we stabilized P::R and v1 is now on CPAN.

jzawodn commented 13 years ago

Any thoughts on releasing a new AnyEvent::Redis to CPAN?

jzawodn commented 13 years ago

Also, I see http://search.cpan.org/~whitney/AnyEvent-Hiredis-0.01/ so someone has begun integration of hiredis with AnyEvent....

jzawodn commented 13 years ago

And also related: http://search.cpan.org/~whitney/Hiredis-Raw-0.03/

It seems like one of those should be ripe for integration.

dgl commented 13 years ago

I'm afraid I've been a bit busy with other things lately. My original plan was to use Protocol::Redis by default and then the Protocol::Redis::XS I wrote which wraps hiredis's parser, but maybe one of those modules would be a better fit.

I did some very simple benchmarks with and without P::R:::XS and didn't see a huge benefit. Really it comes down to performance so some sort of PoC / benchmarks of the various modules would be most welcome, else I'll get to it eventually (but probably won't be for awhile).

dgl commented 13 years ago

(Also I suspect that the expensive bit is the (method) calls in async callbacks rather than the parsing, so e.g. the extra method call that AnyEvent::Hiredis seems to introduce would probably be worth removing using some XS if the aim is performance.)

jzawodn commented 13 years ago

I understand.

I'm doing some testing now and am able to max out a single CPU core using AnyEvent::Redis (CPAN version) at a rate that feels low to me. I'm doing some profiling now and may test out some of these other modules to see how they fare too. But first I swung by to see if there was anything interesting cooking in the unreleased code and it seems there is. :-)

Thanks for all the work you've already done here... I'll let you know if/when I publish some numbers.

Jeremy

On Mon, Sep 19, 2011 at 12:56 PM, David Leadbeater < reply@reply.github.com>wrote:

I'm afraid I've been a bit busy with other things lately. My original plan was to use Protocol::Redis by default and then the Protocol::Redis::XS I wrote which wraps hiredis's parser, but maybe one of those modules would be a better fit.

I did some very simple benchmarks with and without P::R:::XS and didn't see a huge benefit. Really it comes down to performance so some sort of PoC / benchmarks of the various modules would be most welcome, else I'll get to it eventually (but probably won't be for awhile).

Reply to this email directly or view it on GitHub: https://github.com/miyagawa/AnyEvent-Redis/issues/17#issuecomment-2137761

jzawodn commented 13 years ago

Interestingly, nytprof is telling me that AnyEvent::Handle is a bigger culprit right now than AnyEvent::Redis in my benchmarking.

und3f commented 13 years ago

Interestingly, nytprof is telling me that AnyEvent::Handle is a bigger culprit right now than AnyEvent::Redis in my benchmarking.

I am sure that profiling results would change in version with Protocol::Redis.