feat: Use `std::io::BufWriter` with a 64KB buffer for dumping data #1752

larseggert · 2024-03-16T05:32:53Z

Let's see if this makes things faster.

codecov · 2024-03-16T05:48:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.97%. Comparing base (ef5caeb) to head (12af7c7).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1752   +/-   ##
=======================================
  Coverage   92.97%   92.97%           
=======================================
  Files         119      119           
  Lines       37253    37253           
=======================================
  Hits        34637    34637           
  Misses       2616     2616

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-03-16T05:56:34Z

Benchmark results

Performance differences relative to ef5caeb.

coalesce_acked_from_zero 1+1 entries
time: [193.43 ns 193.91 ns 194.42 ns]
change: [-0.2698% +0.2948% +0.7795%] (p = 0.29 > 0.05)
No change in performance detected.
coalesce_acked_from_zero 3+1 entries
time: [235.94 ns 236.53 ns 237.23 ns]
change: [-0.3898% +0.0346% +0.4613%] (p = 0.88 > 0.05)
No change in performance detected.
coalesce_acked_from_zero 10+1 entries
time: [235.69 ns 236.58 ns 237.62 ns]
change: [-0.6618% -0.0811% +0.5301%] (p = 0.79 > 0.05)
No change in performance detected.
coalesce_acked_from_zero 1000+1 entries
time: [213.99 ns 214.17 ns 214.37 ns]
change: [-10.280% -3.6253% +0.4736%] (p = 0.35 > 0.05)
No change in performance detected.
RxStreamOrderer::inbound_frame()
time: [120.45 ms 120.54 ms 120.63 ms]
change: [+0.2043% +0.3029% +0.3933%] (p = 0.00 < 0.05)
Change within noise threshold.
transfer/Run multiple transfers with varying seeds
time: [116.42 ms 116.74 ms 117.05 ms]
thrpt: [34.172 MiB/s 34.265 MiB/s 34.358 MiB/s]
change:
time: [-1.6658% -1.3134% -0.9705%] (p = 0.00 < 0.05)
thrpt: [+0.9800% +1.3309% +1.6940%]
Change within noise threshold.
transfer/Run multiple transfers with the same seed
time: [117.11 ms 117.28 ms 117.45 ms]
thrpt: [34.057 MiB/s 34.107 MiB/s 34.155 MiB/s]
change:
time: [-1.3857% -1.1836% -0.9796%] (p = 0.00 < 0.05)
thrpt: [+0.9893% +1.1978% +1.4052%]
Change within noise threshold.

⬇️ Download logs

mxinden

Missing a <BufWriter as Write>::flush on fin, right? Otherwise some data might remain in the buffer and measurements are hard to compare.

larseggert · 2024-03-16T08:55:28Z

Probably, although the docs say it is called (just if there is an error that remains unhandled.)

neqo-bin/src/bin/client/http3.rs

Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]>

larseggert · 2024-03-16T11:12:31Z

That whole "write to file" functionality could be factored out.

mxinden

I combined the suggestions in larseggert#19.

mxinden · 2024-03-17T07:55:23Z

neqo-bin/src/bin/client/http09.rs

@@ -228,7 +228,7 @@ impl<'b> Handler<'b> {
                return Ok(fin);
            }

-            if let Some(out_file) = maybe_out_file {
+            if let Some(ref mut out_file) = maybe_out_file {


Suggested change

if let Some(ref mut out_file) = maybe_out_file {

if let Some(out_file) = maybe_out_file {

ref mut needed? Compiles for me without.

mxinden · 2024-03-17T08:01:57Z

neqo-bin/src/bin/client/http09.rs

+                let mut buf_writer = maybe_out_file
+                    .take()
+                    .map(|file| BufWriter::with_capacity(BUFWRITER_BUFFER_SIZE, file));


Why construct a new BufWriter on each read call? Shouldn't the BufWriter be wrapping the File for the lifetime of the File? That would be consistent with the http3.rs implementation.

mxinden · 2024-03-17T08:19:34Z

neqo-bin/src/bin/client/http3.rs

+                let buf_writer =
+                    out_file.map(|file| BufWriter::with_capacity(BUFWRITER_BUFFER_SIZE, file));


How about moving this into get_output_file. get_output_file is called by both http3 and http09, thus no need for duplication.

larseggert · 2024-03-17T19:59:17Z

Thanks for that, it's the better approach. Can you point the PR at the main repo?

mxinden · 2024-03-17T20:28:48Z

Thanks for that, it's the better approach. Can you point the PR at the main repo?

👍 see #1756.

larseggert · 2024-03-17T20:35:18Z

Overtaken by #1756

feat: Use std::io::BufWriter with a 64KB buffer for dumping data

f9e7dcb

Let's see if this makes things faster.

larseggert requested review from KershawChang and martinthomson as code owners March 16, 2024 05:32

larseggert marked this pull request as draft March 16, 2024 05:32

larseggert marked this pull request as ready for review March 16, 2024 06:04

larseggert enabled auto-merge March 16, 2024 07:46

mxinden reviewed Mar 16, 2024

View reviewed changes

Flush BufWriter on FIN

280ff37

mxinden approved these changes Mar 16, 2024

View reviewed changes

neqo-bin/src/bin/client/http3.rs Outdated Show resolved Hide resolved

larseggert and others added 3 commits March 16, 2024 12:38

Update neqo-bin/src/bin/client/http3.rs

77e7058

Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]>

Fix

d28d36d

Also for h09

946493a

larseggert added 2 commits March 16, 2024 13:36

Merge branch 'main' into feat-client-bufwriter

ed30855

Merge branch 'main' into feat-client-bufwriter

12af7c7

mxinden reviewed Mar 17, 2024

View reviewed changes

mxinden mentioned this pull request Mar 17, 2024

feat(bin/client): use BufWriter when writing to file #1756

Merged

larseggert closed this Mar 17, 2024

auto-merge was automatically disabled March 17, 2024 20:35
Pull request was closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Use `std::io::BufWriter` with a 64KB buffer for dumping data #1752

feat: Use `std::io::BufWriter` with a 64KB buffer for dumping data #1752

larseggert commented Mar 16, 2024

codecov bot commented Mar 16, 2024 •

edited

Loading

github-actions bot commented Mar 16, 2024 •

edited

Loading

mxinden left a comment

larseggert commented Mar 16, 2024

larseggert commented Mar 16, 2024

mxinden left a comment

mxinden Mar 17, 2024

mxinden Mar 17, 2024

mxinden Mar 17, 2024

larseggert commented Mar 17, 2024

mxinden commented Mar 17, 2024

larseggert commented Mar 17, 2024

	if let Some(ref mut out_file) = maybe_out_file {
	if let Some(out_file) = maybe_out_file {

		let buf_writer =
		out_file.map(\|file\| BufWriter::with_capacity(BUFWRITER_BUFFER_SIZE, file));

feat: Use std::io::BufWriter with a 64KB buffer for dumping data #1752

feat: Use std::io::BufWriter with a 64KB buffer for dumping data #1752

Conversation

larseggert commented Mar 16, 2024

codecov bot commented Mar 16, 2024 • edited Loading

Codecov Report

github-actions bot commented Mar 16, 2024 • edited Loading

Benchmark results

mxinden left a comment

Choose a reason for hiding this comment

larseggert commented Mar 16, 2024

larseggert commented Mar 16, 2024

mxinden left a comment

Choose a reason for hiding this comment

mxinden Mar 17, 2024

Choose a reason for hiding this comment

mxinden Mar 17, 2024

Choose a reason for hiding this comment

mxinden Mar 17, 2024

Choose a reason for hiding this comment

larseggert commented Mar 17, 2024

mxinden commented Mar 17, 2024

larseggert commented Mar 17, 2024

feat: Use `std::io::BufWriter` with a 64KB buffer for dumping data #1752

feat: Use `std::io::BufWriter` with a 64KB buffer for dumping data #1752

codecov bot commented Mar 16, 2024 •

edited

Loading

github-actions bot commented Mar 16, 2024 •

edited

Loading