Use stream buffering in report collector #1504
Conversation
Continuation of private-attribution#1501, we want to avoid submitting reports one by one from RC to each individual shard. That likely leads to fragmentation and we've been observing slow execution on the client side. This sets up the buffer size to be divisible by TCP MSS, but I don't have any real evidence that this is going to work well. We would need to experiment with it
ipa-core/src/bin/report_collector.rs
Outdated
| @@ -430,6 +431,9 @@ async fn hybrid( | |||
| count: usize, | |||
| set_fixed_polling_ms: Option<u64>, | |||
| ) -> Result<(), Box<dyn Error>> { | |||
| // twice the size of TCP MSS. This may get messed up if TCP options are used which is not | |||
| // in our control, but hopefully fragmentation is not too bad | |||
| const BUF_SIZE: NonZeroUsize = NonZeroUsize::new(1072).unwrap(); | |||
There was a problem hiding this comment.
@andyleiserson do you have any suggestion on picking up the buffer size?
There was a problem hiding this comment.
The tokio and std buffered I/O helpers use 8 kB, so maybe use that? I don't think it's necessary to try and match this to TCP MSS, I might not even expose the buffer size from BufferedBytesStream until we identify a need to tune it for individual uses.
There was a problem hiding this comment.
I think what's important is that the kernel has at least a full packet's worth of data, so that it can send full packets.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1504 +/- ##
==========================================
+ Coverage 92.87% 93.10% +0.22%
==========================================
Files 242 242
Lines 44177 44269 +92
==========================================
+ Hits 41031 41217 +186
+ Misses 3146 3052 -94 ☔ View full report in Codecov by Sentry. |
ipa-core/src/bin/report_collector.rs
Outdated
| @@ -430,6 +431,9 @@ async fn hybrid( | |||
| count: usize, | |||
| set_fixed_polling_ms: Option<u64>, | |||
| ) -> Result<(), Box<dyn Error>> { | |||
| // twice the size of TCP MSS. This may get messed up if TCP options are used which is not | |||
| // in our control, but hopefully fragmentation is not too bad | |||
| const BUF_SIZE: NonZeroUsize = NonZeroUsize::new(1072).unwrap(); | |||
There was a problem hiding this comment.
The tokio and std buffered I/O helpers use 8 kB, so maybe use that? I don't think it's necessary to try and match this to TCP MSS, I might not even expose the buffer size from BufferedBytesStream until we identify a need to tune it for individual uses.
|
I don't see any difference in performance on 1B records. I think the bottleneck is on the upload, so I'll hold off merging this |
|
I intend to merge it - there is nothing bad about this change and buffered writes from RC can still be useful for submitting medium-sized inputs |
Continuation of #1501, we want to avoid submitting reports one by one from RC to each individual shard. That likely leads to fragmentation and we've been observing slow execution on the client side.
This sets up the buffer size to be divisible by TCP MSS, but I don't have any real evidence that this is going to work well. We would need to experiment with it