walk: Encapsulate the buffering behavior in a struct #895
Conversation
tavianator
force-pushed
the
receiver-buffer
branch
from
3eef8ab to
08d41c4
Compare
The new ReceiverBuffer struct allows us to factor out the receiver
implementation into a number of helper methods. The new implementation
uses rx.{recv,recv_timeout} instead of a for loop, which enables us to
switch to streaming mode at the right time without waiting for more
results.
Fixes sharkdp#868.
tavianator
force-pushed
the
receiver-buffer
branch
from
08d41c4 to
16ae03c
Compare
| } | ||
|
|
||
| /// Wait for a result or state change. | ||
| fn poll(&mut self) -> Result<(), ExitCode> { |
There was a problem hiding this comment.
It's a little weird that this function is poll but it isn't actually part of a Future. I don't have a better name though. It's just a little unfortunate that it happens to share a name with the most important function of a Future.
| } | ||
|
|
||
| /// Wait for a result or state change. | ||
| fn poll(&mut self) -> Result<(), ExitCode> { |
There was a problem hiding this comment.
| fn poll(&mut self) -> Result<(), ExitCode> { | |
| fn poll(&mut self) -> ControlFlow<ExitCode> { |
Would probably be a better return type here, but that would require increasing the MSRV to 1.55.
There was a problem hiding this comment.
I didn't realize ControlFlow was stabilized. I'll leave it up to @sharkdp whether that MSRV bump would be acceptable.
There was a problem hiding this comment.
Probably not worth bumping MSRV just for this, but may be worth changing when we do bump it.
|
Have you done any benchmarks on this? |
|
@tmccombs Benchmarks seem totally neutral |
This lets us avoid rust-lang/rust#39364, which could potentially be seen now that we're using recv_timeout().
|
Thank you! I'm always usually fine with bumping MSRV, because |
|
This PR (specifically a4bb734) introduces a pretty severe performance regression (20-30%) for me. For example: I found this with @tmccombs: I think you have also tried crossbeam-channel in the past? I vaguely remember that I tried it as well at some point and also saw a performance regression. |
|
@sharkdp Interesting, I did re-do the benchmarks after switching to crossbeam and found there was still no measurable difference: The pattern doesn't seem to affect it, whether there's many matches or few. I wonder if there is a platform-specific thing going on, either OS or hardware. What platform(s) do you see the regression on? My tests were on a Threadripper 3960X running Linux. |
|
I also see no perf regression on an i7-6700HQ. So it's not that crossbeam is only fast on new hardware :) |
|
Anyway I can prepare a revert until we understand what's going on so it doesn't block a new release |
@sharkdp [noticed][1] a quite severe performance degredation due to this change. Switch back to std::sync::mpsc until we can fix the performance regression. This reverts commit a4bb734. [1]: sharkdp#895 (comment)
|
I never did benchmarks for crossbeam. I'm actually a little surprised if that is it, since crossbeam is supposed to be faster than std mpsc (although that might depend on how it is used) |
Hmm. Strange. Im also on Linux, Intel Core i7-4700MQ. Maybe it depends on the target folder instead? I will try to do some more comprehensive benchmarks. Thank you for opening the revert PR. |
|
@sharkdp I'd be curious to compare |
|
Ok. Let's continue the discussion in #933. |
The new ReceiverBuffer struct allows us to factor out the receiver
implementation into a number of helper methods. The new implementation
uses rx.{recv,recv_timeout} instead of a for loop, which enables us to
switch to streaming mode at the right time without waiting for more
results.
Fixes #868.