fix(bin): enable pacing on client & server by default

[mxinden]

Updated version:

Before #1724 neqo-server would always run with ConnectionParameters::pacing true. neqo-client would provide a command line flag --pacing which defaulted to false.

#1724 consolidated the QuicParameters struct from neqo-client and neqo-server. Since both neqo-client and neqo-server default to pacing false.

This commit replaces the --pacing flag with --no-pacing. Pacing is enabled by default on both neqo-server and neqo-client.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 89.48%. Comparing base (9ff3751) to head (fb324a2).
Report is 2 commits behind head on main.

Files	Patch %	Lines
neqo-bin/src/lib.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1753   +/-   ##
=======================================
  Coverage   89.48%   89.48%           
=======================================
  Files         126      126           
  Lines       38866    38866           
=======================================
  Hits        34780    34780           
  Misses       4086     4086           

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

[github-actions]

Benchmark results

Performance differences relative to ef5caeb.

• coalesce_acked_from_zero 1+1 entries
  time: [192.85 ns 193.24 ns 193.68 ns]
  change: [-0.4995% +0.0449% +0.5020%] (p = 0.87 > 0.05)
  No change in performance detected.

• coalesce_acked_from_zero 3+1 entries
  time: [235.67 ns 236.26 ns 236.89 ns]
  change: [-0.6060% -0.1697% +0.2504%] (p = 0.45 > 0.05)
  No change in performance detected.

• coalesce_acked_from_zero 10+1 entries
  time: [235.51 ns 236.48 ns 237.58 ns]
  change: [-0.7379% -0.0915% +0.5800%] (p = 0.80 > 0.05)
  No change in performance detected.

• coalesce_acked_from_zero 1000+1 entries
  time: [214.79 ns 224.26 ns 245.28 ns]
  change: [-9.4943% -1.5072% +4.5563%] (p = 0.79 > 0.05)
  No change in performance detected.

• RxStreamOrderer::inbound_frame()
  time: [120.36 ms 120.44 ms 120.51 ms]
  change: [+0.1333% +0.2183% +0.3053%] (p = 0.00 < 0.05)
  Change within noise threshold.

• transfer/Run multiple transfers with varying seeds
  time: [117.04 ms 117.36 ms 117.68 ms]
  thrpt: [33.991 MiB/s 34.084 MiB/s 34.177 MiB/s]
  change:
  time: [-1.1259% -0.7901% -0.4235%] (p = 0.00 < 0.05)
  thrpt: [+0.4253% +0.7964% +1.1387%]
  Change within noise threshold.

• transfer/Run multiple transfers with the same seed
  time: [117.73 ms 117.92 ms 118.12 ms]
  thrpt: [33.865 MiB/s 33.921 MiB/s 33.976 MiB/s]
  change:
  time: [-0.8572% -0.6423% -0.4237%] (p = 0.00 < 0.05)
  thrpt: [+0.4255% +0.6465% +0.8646%]
  Change within noise threshold.

⬇️ Download logs

[larseggert]

It's probably an oversight that the two have different defaults? I think the defaults should be the same, for consistency.

[martinthomson]

I found clap-rs/clap#815 (comment) for doing negation. So you can have --pacing and --no-pacing options. Not sure what this does with a default of true though...

[mxinden]

It's probably an oversight that the two have different defaults? I think the defaults should be the same, for consistency.

I suggest making pacing=true the default on both client and server then. This is also consistent with the default of ConnectionParameters:

neqo/neqo-transport/src/connection/params.rs

Line 105 in ef5caeb

pacing: true,

We can then expose a --no-pacing flag on both neqo-server and neqo-client.

Sounds good?

[larseggert]

Yes. Thank you!