Exchange: bench, do not preallocate buffers #416
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
antiguru
force-pushed
the
exchange_bench
branch
2 times, most recently
from
268bdff to
a6c142e
Compare
|
Benchmark results temporarily at https://dev.antiguru.de/criterion/report/ |
|
@antiguru , I took a close look at the PR. Unfortunately, trying it out is not as easy as just switching to a different timely branch; I'd have to modify our code that constructs the dataflow graph to apply the new pacts to every operator. I apologize, but unfortunately I won't have the cycles to work on this now :( |
antiguru
force-pushed
the
exchange_bench
branch
2 times, most recently
from
4d9839b to
9f52d13
Compare
antiguru
force-pushed
the
exchange_bench
branch
from
9f52d13 to
5823de2
Compare
antiguru
force-pushed
the
exchange_bench
branch
from
5823de2 to
3477d4f
Compare
antiguru
changed the title
antiguru
force-pushed
the
exchange_bench
branch
from
e04a831 to
59b11f5
Compare
The LazyExchange PACT avoids allocations ahead of time while maintaining the performance of the eagerly-allocating Exchange PACT. Extend the Exchange operator with an `exchange_pact` function taking an explicit PACT. Signed-off-by: Moritz Hoffmann <[email protected]>
Signed-off-by: Moritz Hoffmann <[email protected]>
Signed-off-by: Moritz Hoffmann <[email protected]>
Signed-off-by: Moritz Hoffmann <[email protected]>
Signed-off-by: Moritz Hoffmann <[email protected]>
Signed-off-by: Moritz Hoffmann <[email protected]>
Instead of defining distinct exchange pushers, make the default generic and provide necessary hooks for specialized exchange pushers. Signed-off-by: Moritz Hoffmann <[email protected]>
Signed-off-by: Moritz Hoffmann <[email protected]>
I did some measurements comparing the * current default, * a non-preallocating exchange * id. plus eagerly releasing exchange. The benchmark in the chain of commits does not report a significant change in performance for all variants. Erring on the side of caution, I'd propse to make the second variant, i.e. not pre-allocating, but retaining if push returned a buffer, the default exchange pact. Currently, all non-zerocopy allocators will not return an allocation on push, so in this case, the second and third variant are equivalent. # Why is there no change in performance? For single-recrod exchanges, this can be reasoned as follows. In the old approach, we'd have an allocated buffer, where we write a single element. On flush, the buffer would be sent to the pusher, which might or might not return a buffer. If it doesn't, we'd allocate a new one. In the new approach, we don't have a buffer to append to, so we'd first allocate one. On flush, we send the buffer downstream and maybe get one back. We don't eagerly allocate a new one if there is no buffer available. Both approaches allocate a new buffer if the pusher doesn't/didn't return one earlier. Only the order of allocations is changed. While in the old approach we send data and allocate, we allocate and send data in the new approach. On top of this, the new approach needs to pay a cost of checking the buffer's capacity for each record on the input, but I'd hope for the optimizer to sort this out. The same argument seems to hold for larger batch sizes. Signed-off-by: Moritz Hoffmann <[email protected]>
frankmcsherry
approved these changes
Nov 19, 2021
Signed-off-by: Moritz Hoffmann <[email protected]>
antiguru
force-pushed
the
exchange_bench
branch
from
59b11f5 to
090e5e0
Compare
Signed-off-by: Moritz Hoffmann <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR:
I compared the current implementation versus the changes in this PR, and the benchmark doesn't report significant changes in performance. Obviously, this PR encodes a policy change from eagerly allocated buffers to allocating them on-demand, which could have an impact on memory utilization and processing latency.
As explained on the commit message, the cost of allocating buffers exists today, only that it is now during initial construction and with the default process allocator after push operations. We shift those to allocations before the fist buffer push, which means that the absolute number of allocations stays the same. Regarding latency, we need to check before pushing to a buffer whether it has the right capacity, which might add to latency. However, I'd hope that the compiler can fold this into the subsequent test whether the buffer is full, so I wouldn't expect much of a latency impact either.