chore: Cleanups for write_appdata_frames

[jesup]

Resolves #1439

We could drop it below Normal as well

[martinthomson]

Pinging @jesup

Please also consider putting datagrams below Normal priority streams.

[jesup]

Pinging @jesup

Please also consider putting datagrams below Normal priority streams.

so... should I do that, or should I not? Mixing datagrams and normal streams will not be the norm, I suspect. If we want ultimate flexibility, we could allow for setting the priority of datagrams via a sendOrder value, or a before-vs-after-normal setting. But I don't think we need that.
A related question is the spec - should the priority of datagrams relative to normal be speced in some way?
For now I've kept it right above normal, but it's trivial to move it below.

[jesup]

I decided to move datagrams below normal, since they're inherently unreliable

[jesup]

Hmmmpf, that default grease thing snuck in here. No idea how... Still not happy with git

[martinthomson]

If you are able, a test is what this change requires.

[KershawChang]

@jesup this patch seems to have conflicts. Could you rebase again?

[martinthomson]

Pinging @jesup. (I realize that we've left a few items open for probably too long and I'm checking in again.)

[larseggert]

@jesup what is needed to land this?

[larseggert]

Now that I looked at the code, this actually got fixed already in #1447 and the test got added in #1450. I did make some cleanups while rebasing this, so I think we should still merge it.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.12%. Comparing base (c004359) to head (4cbe869).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1440      +/-   ##
==========================================
- Coverage   93.13%   93.12%   -0.01%     
==========================================
  Files         117      117              
  Lines       36353    36345       -8     
==========================================
- Hits        33857    33848       -9     
- Misses       2496     2497       +1     

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

[github-actions]

Benchmark results

Performance differences relative to 34d9603.

• drain a timer quickly time: [419.63 ns 425.78 ns 431.59 ns]
  change: [+34.944% +37.546% +40.101%] (p = 0.00 < 0.05)
  💔 Performance has regressed.

• coalesce_acked_from_zero 1+1 entries
  time: [215.31 ns 215.75 ns 216.25 ns]
  change: [-1.3289% -0.8280% -0.4118%] (p = 0.00 < 0.05)
  Change within noise threshold.

• coalesce_acked_from_zero 3+1 entries
  time: [248.02 ns 248.66 ns 249.37 ns]
  change: [-1.6236% -1.2659% -0.8764%] (p = 0.00 < 0.05)
  Change within noise threshold.

• coalesce_acked_from_zero 10+1 entries
  time: [248.21 ns 249.22 ns 250.32 ns]
  change: [-1.4325% -0.8635% -0.2178%] (p = 0.00 < 0.05)
  Change within noise threshold.

• coalesce_acked_from_zero 1000+1 entries
  time: [221.02 ns 221.29 ns 221.56 ns]
  change: [-2.4148% -1.8751% -1.3551%] (p = 0.00 < 0.05)
  💚 Performance has improved.

• RxStreamOrderer::inbound_frame()
  time: [118.41 ms 118.47 ms 118.54 ms]
  change: [-0.8149% -0.6018% -0.4232%] (p = 0.00 < 0.05)
  Change within noise threshold.

• transfer/Run multiple transfers with varying seeds
  time: [118.91 ms 119.21 ms 119.50 ms]
  thrpt: [33.473 MiB/s 33.554 MiB/s 33.637 MiB/s]
  change:
  time: [-0.4562% -0.1097% +0.2403%] (p = 0.53 > 0.05)
  thrpt: [-0.2397% +0.1098% +0.4583%]
  No change in performance detected.

• transfer/Run multiple transfers with the same seed
  time: [119.53 ms 119.71 ms 119.89 ms]
  thrpt: [33.365 MiB/s 33.415 MiB/s 33.464 MiB/s]
  change:
  time: [-0.3303% -0.0954% +0.1370%] (p = 0.44 > 0.05)
  thrpt: [-0.1368% +0.0955% +0.3314%]
  No change in performance detected.

• 1-conn/1-100mb-resp (aka. Download)/client
  time: [1.0883 s 1.1002 s 1.1129 s]
  thrpt: [89.854 MiB/s 90.894 MiB/s 91.883 MiB/s]
  change:
  time: [-2.5688% -1.0599% +0.5931%] (p = 0.23 > 0.05)
  thrpt: [-0.5896% +1.0712% +2.6365%]
  No change in performance detected.

• 1-conn/10_000-parallel-1b-resp (aka. RPS)/client
  time: [422.41 ms 424.56 ms 426.78 ms]
  thrpt: [23.431 Kelem/s 23.554 Kelem/s 23.674 Kelem/s]
  change:
  time: [-1.6263% -0.8292% -0.0623%] (p = 0.04 < 0.05)
  thrpt: [+0.0623% +0.8362% +1.6532%]
  Change within noise threshold.

• 1-conn/1-1b-resp (aka. HPS)/client
  time: [50.426 ms 50.794 ms 51.187 ms]
  thrpt: [19.536 elem/s 19.687 elem/s 19.831 elem/s]
  change:
  time: [-1.5228% -0.4044% +0.6768%] (p = 0.48 > 0.05)
  thrpt: [-0.6723% +0.4061% +1.5463%]
  No change in performance detected.

Client/server transfer results

Transfer of 134217728 bytes over loopback.

Client	Server	CC	Pacing	Mean [ms]	Min [ms]	Max [ms]	Relative
msquic	msquic			693.9 ± 185.6	451.7	998.1	1.00
neqo	msquic	reno	on	1127.5 ± 280.9	831.5	1537.7	1.00
neqo	msquic	reno		994.9 ± 206.1	808.7	1414.5	1.00
neqo	msquic	cubic	on	1091.1 ± 223.3	808.1	1499.3	1.00
neqo	msquic	cubic		1134.1 ± 270.5	853.3	1637.9	1.00
msquic	neqo	reno	on	4554.9 ± 268.1	4117.7	5062.0	1.00
msquic	neqo	reno		4470.4 ± 277.8	4062.7	4990.2	1.00
msquic	neqo	cubic	on	4547.9 ± 247.6	4229.6	5099.7	1.00
msquic	neqo	cubic		4548.5 ± 234.9	4161.7	4904.2	1.00
neqo	neqo	reno	on	3458.8 ± 316.8	3137.8	4181.4	1.00
neqo	neqo	reno		3566.1 ± 317.2	3184.1	4145.5	1.00
neqo	neqo	cubic	on	4314.3 ± 269.3	4035.5	4900.6	1.00
neqo	neqo	cubic		4183.1 ± 581.0	2758.2	4988.9	1.00

⬇️ Download logs

[larseggert]

Actually, I think I misunderstood something, because we didn't merge #1447.

But I still don't understand what the change in this PR compared to main actually is?