Publish batching in Redis Engine

[FZambia]

As part of #59

[FZambia]

To summarize things done in this pr:

Added pubRequest struct and pubCh to Redis Engine. Now every publish results in sending pubRequest to pubCh - and one goroutine reads from that channel, batches publish requests and sends to Redis (keeping publish engine method synchronous). Almost the same like @banks already did for managing subscriptions.

[banks]

Hmm, this works but it feels off to me...

It's intentionally creating overhead and contention on the publish channel etc as well as spawning maybe thousands of goroutines. Perhaps that's not too bad in practice, but in theory it would be possible to do this much more efficiently if we had a real async API in our engine interface.

Maybe something for the future?

[FZambia]

Actually that was not so simple because we don't call engine method directly from broadcastCmd - we call application's publish method which contains lot of helper logic.

But anyway I think I changed it in a way you suggested - not adding new engine method though but returning error channel from existing and added helper method to publish synchronously to Application.

Result is wonderful: broadcast now 2 times faster for 10000 channels than version with goroutines. So overall performance improvement for broadcast is about 11-12x

[banks]

Great!

Will look at code again tomorrow but that just didn't look very efficient!

Thanks

On 22 Mar 2016, at 21:27, Alexandr Emelin notifications@github.com wrote:

Actually that was not so simple because we don't call engine method directly from broadcastCmd - we call application's publish method which contains lot of helper logic.

But anyway I think I changed it in a way you suggested - not adding new engine method though but returning error channel from existing and added helper method to publish synchronously to Application.

Result is wonderful: broadcast now 2 times faster for 10000 messages than version with goroutines.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

[banks]

Nitpick but that name is a little bit confusing. It's real purpose is a synchronous helper so maybe publishSync would be cleaner name?

Not a big deal if you disagree though.

[FZambia]

Agree, just deleted it

[banks]

This looks great to me 👍

If I get a chance, I'll try it out, but don't wait if you are happy.

[FZambia]

ok, thanks a lot for looking! I can't find something I can improve here at moment - so going to merge this then. This async publish method in engine opens a road to further optimizations maybe - i.e. something like async: true in publish command if user don't need to wait for publish result to speed up publish commands coming from single Redis queue for example. But let's discuss this later outside this pr.