Avoid unnecessary system calls in redisAsyncCommand()

[tezc]

Currently, this is how async operations work:

1- Append new command to the output buffer. 2- __redisAsyncCommand() registers write event unconditionally. 3- redisAsyncWrite() will be called on the next iteration of the event loop. 4- Write event will be cleared.

Steps 2 and 4 are system calls. They introduce unnecessary overhead.

For better performance, we can directly write to the socket(step-3) without registering a write event. Still, it is better to detect if the write event is already registered. In this case, we won't try to write to the socket as it will probably fail (because the socket buffer is full).

[michael-grunder]

This looks correct to me at first glance, although I didn't write the async layer.

How big a performance improvement are you seeing with it, assuming you're using the new logic?

[tezc]

Hi, @michael-grunder

I see %2 - %20 improvement in my use case. (I use hiredis as part of a bigger project, so a loop benchmark with hiredis commands only probably would give us even better results).

I'm pretty sure this is how we are supposed to use event loops. (Write to socket first, if it is partial, register for the write event).

The downside is, now inside redisAsyncCommand(), we can call __redisAsyncDisconnect() which will trigger disconnect/cleanup callbacks. Not sure if this is a problem or surprising to anybody. Other than that, I don't see a problem but maybe I'm missing something, let's bring more people with async API experience, @yossigo wdty?

Edit: Maybe another side effect is about batching. Today, you can call redisAsyncCommand() multiple times but it will call redisAsyncWrite() once when the write event is triggered. With this PR, this is not going to be the case. I'm not sure if anybody has any assumptions on this.

[yossigo]

@tezc I agree that generally, one should create the write event only after failing to write. In this specific case, I think it will take a bit more to handle it well.

Invoking the disconnect callback directly from redisAsyncCommand() is a significant breaking change and makes the API less intuitive. We could return an error and defer the callback to the next event loop iteration, but that will be a lot more complex than this simple change.

The other point is that the async interface does not have explicit pipelining support (like redisAppendCommand()), so pipelining has to rely on this behavior, and we'll need to come up with an alternative.

Another option to consider is to create a new redisUnbufferredAsyncCommand(), to be used when the extra delay is not desired.

[michael-grunder]

I think the improvement is worth making, but we'll need to make sure not to introduce any breaking changes.

I'm planning on putting together v1.1.0 this weekend, and then we can revisit this PR for the next release.

[tezc]

Okay, I'll see what I can do. Sure, we can revisit this later.