AnyEvent-Redis icon indicating copy to clipboard operation
AnyEvent-Redis copied to clipboard

Protocol::Redis for parsing Redis' messages

Open und3f opened this issue 13 years ago • 19 comments

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.

und3f avatar Mar 19 '11 20:03 und3f

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).

dgl avatar Mar 19 '11 22:03 dgl

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.

vti avatar Mar 19 '11 23:03 vti

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

und3f avatar Mar 20 '11 09:03 und3f

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

dgl avatar Mar 20 '11 09:03 dgl

Fixed that

und3f avatar Mar 20 '11 13:03 und3f

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.

otterley avatar Apr 04 '11 15:04 otterley

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.

und3f avatar Apr 04 '11 16:04 und3f

That sounds very promising, then. :)

otterley avatar Apr 04 '11 16:04 otterley

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

dgl avatar Apr 05 '11 00:04 dgl

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

und3f avatar Apr 10 '11 07:04 und3f

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

und3f avatar May 11 '11 11:05 und3f

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

jzawodn avatar Sep 19 '11 19:09 jzawodn

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

jzawodn avatar Sep 19 '11 19:09 jzawodn

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

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

jzawodn avatar Sep 19 '11 19:09 jzawodn

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 avatar Sep 19 '11 19:09 dgl

(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.)

dgl avatar Sep 19 '11 20:09 dgl

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 < [email protected]>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 avatar Sep 19 '11 20:09 jzawodn

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

jzawodn avatar Sep 19 '11 21:09 jzawodn

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.

und3f avatar Sep 19 '11 21:09 und3f