perf: Only register wakeups in the consumer when no messages are ready

[Marwes]

By using tokio's Notify type and it's capability to get notified even
if no one was available to be notified when the notification fires we
avoid the need for locking and we keep synchronization out of the
common path (a message being available).

This does force tokio to be pulled in for non-tokio users, however they
will only get the now required sync feature which excludes most of
tokio.

https://docs.rs/tokio/1.0.1/tokio/sync/struct.Notify.html

[benesch]

Thanks! This is a good idea. I landed a very simple version of this optimization in #321.

I'm not sure whether Tokio's Notified struct will buy us much on top of that. Did you happen to benchmark the performance of this change? My preference is to avoid introducing the dependency on tokio/sync unless the performance gains are substantial—there are some async-std/smol folks who really dislike having Tokio in the tree. And it looks to me like Notified will acquire a lock in the pending case anyway (https://github.com/tokio-rs/tokio/blob/3b6bee822dfe70caea7eb22b51fefb28d162f966/tokio/src/sync/notify.rs#L555), though it's complicated enough that I'm not totally sure.

[benesch]

I don't think I get this. In the typical case, when the message queue becomes non-empty, we'll wake up precisely one waiter. Then we'll end up here, because we know there is at least one message available, at which point we'll notify all the wakers. So why not just call wakers.notify_wakers from the context to start?

[Marwes]

The problem with calling notify_waiters is that it will only notify the currently registered waiters, whereas notify_one will register one waiter or the next waiter, if none are currently waiting.

We also can't call both notify functions when the non-empty callback is called since it would be possible to race it such that

Task 1 sees no message, returns from poll
Non empty callback notifies all waiters
Task 1 acquires the handle from `notified()` but won't be woken up until the next non-empty callback.

Always notifying when we know there are likely messages solves this issue.

[benesch]

Ah, ok, thanks for explaining.

[Marwes]

I'm not sure whether Tokio's Notified struct will buy us much on top of that. Did you happen to benchmark the performance of this change?

No, this is mostly speculative and it is nice to remove locking in async code.

My preference is to avoid introducing the dependency on tokio/sync unless the performance gains are substantial—there are some async-std/smol folks who really dislike having Tokio in the tree.

I can see the optics being a bit bad, however this only actually includes and compiles tokio's sync module which is runtime independent. So despite tokio appearing in the lockfile, there isn't a lot of code being included and there is no need to worry about the tokio runtime (which is the actual, practical issue of tokio being included). There is no reason Notified couldn't live outside (other than the effort of maintaining it).

And it looks to me like Notified will acquire a lock in the pending case anyway (https://github.com/tokio-rs/tokio/blob/3b6bee822dfe70caea7eb22b51fefb28d162f966/tokio/src/sync/notify.rs#L555), though it's complicated enough that I'm not totally sure.

But the common case is that the we do not have a Notified handle and there is a message available (so we do not need to create a new one either. In this case all we do is call notify_wakers which, as long as there are no wakers, are just some atomic checks before returning. Granted, your solution also does this so perhaps this PR isn't as useful. It is only a gain when there are no more messages to consume in which case it avoids an extra poll with all the syncrhonization that entails inside librdkafka.

[benesch]

It is only a gain when there are no more messages to consume in which case it avoids an extra poll with all the syncrhonization that entails inside librdkafka.

Ok, yep, that makes sense. I think the combination of the Tokio dep + the extra complexity makes me leery of merging this, but I'd be happy to be swayed by performance numbers.

[benesch]

Thanks again for this @Marwes but I think I'm going to hold off on the extra complexity unless there is compelling improved performance. I'm afraid I don't have time to do that investigation myself, but I'd be happy to reconsider if you provide evidence!