Closed akoshelev closed 2 months ago
Attention: Patch coverage is 83.72642%
with 69 lines
in your changes missing coverage. Please review.
Project coverage is 91.14%. Comparing base (
7bb1325
) to head (bf06b8c
). Report is 59 commits behind head on main.:exclamation: Current head bf06b8c differs from pull request most recent head 134b659
Please upload reports for the commit 134b659 to get more accurate results.
Files | Patch % | Lines |
---|---|---|
ipa-core/src/helpers/buffers/circular.rs | 83.72% | 69 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
I've been running IPA at scale and one thing #1085 revealed is that network stack (hyper and kernel) does not really like the large buffers we've been sending after vectorization was enabled. @andyleiserson showed that keeping active work at 1024 results in 1.6Mb chunks sent down from the send buffers. Reducing active work to 32 eliminated the stall, but, according to my perf tests, it made our network utilization worse.
Fixing the size of
take
, allowed to increase active work parameter to extreme values (100k) and below are the results of those experiments (1M rows, real hardware, no match key encryption, HTTPS).This change aims to decouple the vectorization/active work parameters from network send buffers by using circular buffer implementation inside
OrderingSender
. It only brings the implementation ofCircularBuf
but I have the change to makeOrderingSender
use this buf sitting locally in my branch.The ultimate goal is obviously to allow more configuration for steps, but having constant size chunks dropped down to hyper send buffers seems useful anyway.
Circular buffer
Nothing fancy here, just a vector with read and write pointers. I've been contemplating whether I should make it thread-safe, but I am not convinced we need it in the long term if we pick shard-per-core model. It is not worse than our current implementation of
OrderingSender
because it also operates on a single allocation, but managing two pointers made it too complicated, so I decided to take the buffering business away fromOrderingSender
.Ordering sender
With this change we lose two features that
OrderingSender
currently supportsOrderingSender
allowed randomly sized writes as long as they can fit into the buffer. It will no longer be supported if we go withCircularBuf
- all writes must be of the same size. This is also how we useOrderingSender
today.Both of these features were never used, but it is important to note that they won't be available if we migrate to
CircularBuf
.