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 93.26% 93.25% -0.02%
==========================================
Files 238 238
Lines 43688 43780 +92
==========================================
+ Hits 40746 40826 +80
- Misses 2942 2954 +12 ☔ 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 |
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