refactor!: rename write_chunk to write_bytes and write_chunks to write_bytes_many#536
Conversation
Also remove the Written struct from the public API.
Performance Comparison Report
|
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5996.9 Mbps | 7835.8 Mbps | -23.5% | 92.8% / 97.8% |
| medium-concurrent | 5516.9 Mbps | 7693.7 Mbps | -28.3% | 92.3% / 97.4% |
| medium-single | 4342.1 Mbps | 4481.5 Mbps | -3.1% | 95.7% / 111.0% |
| small-concurrent | 3946.3 Mbps | 5243.9 Mbps | -24.7% | 97.0% / 111.0% |
| small-single | 3642.7 Mbps | 4659.8 Mbps | -21.8% | 92.7% / 113.0% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 3128.0 Mbps | 3659.1 Mbps | -14.5% |
| lan | 782.4 Mbps | 796.4 Mbps | -1.8% |
| lossy | 69.8 Mbps | 55.9 Mbps | +25.0% |
| wan | 83.8 Mbps | 83.8 Mbps | ~0% |
Summary
noq is 20.3% slower on average
45ff73d5eb5ab514d5f75a342a524bf1e39a58eb - artifacts
Raw Benchmarks (localhost)
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 6037.7 Mbps | 7945.0 Mbps | -24.0% | 96.8% / 98.6% |
| medium-concurrent | 5784.8 Mbps | 8016.9 Mbps | -27.8% | 97.0% / 98.3% |
| medium-single | 3795.2 Mbps | 4473.8 Mbps | -15.2% | 96.6% / 98.5% |
| small-concurrent | 3998.1 Mbps | 5182.6 Mbps | -22.9% | 96.7% / 99.5% |
| small-single | 3656.7 Mbps | 4735.2 Mbps | -22.8% | 95.7% / 98.4% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 2935.9 Mbps | N/A | N/A |
| lan | 782.5 Mbps | N/A | N/A |
| lossy | 55.9 Mbps | N/A | N/A |
| wan | 83.8 Mbps | N/A | N/A |
Summary
noq is 23.3% slower on average
7b63c469ef0f34577fd16ef56803e75867c70bbf - artifacts
No results available
79d885dad2c02c57c0d4f3306a1f1080935b3ef4 - artifacts
No results available
0e070fee0c6edc510fe6ee19737efaffabecfadd - artifacts
No results available
a1ca52a10fead7d6071876d6d7e00ea7731c9b22 - artifacts
Raw Benchmarks (localhost)
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5497.1 Mbps | 8188.3 Mbps | -32.9% | 96.4% / 150.0% |
| medium-concurrent | 5512.0 Mbps | 7688.9 Mbps | -28.3% | 97.3% / 150.0% |
| medium-single | 4143.3 Mbps | 4469.8 Mbps | -7.3% | 89.8% / 97.9% |
| small-concurrent | 3878.4 Mbps | 5025.4 Mbps | -22.8% | 95.2% / 103.0% |
| small-single | 3540.6 Mbps | 4623.3 Mbps | -23.4% | 95.1% / 103.0% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 3128.4 Mbps | 3994.9 Mbps | -21.7% |
| lan | 782.4 Mbps | 810.3 Mbps | -3.5% |
| lossy | 69.8 Mbps | 69.8 Mbps | ~0% |
| wan | 83.8 Mbps | 83.8 Mbps | ~0% |
Summary
noq is 23.8% slower on average
567c543c1e2e6b6723cefbe34923fbf226bbb9c8 - artifacts
No results available
10c707b2a8b67130fd1a4599e648319f40abd9bc - artifacts
Raw Benchmarks (localhost)
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5385.7 Mbps | 7980.9 Mbps | -32.5% | 95.3% / 99.3% |
| medium-concurrent | 5430.9 Mbps | 7712.1 Mbps | -29.6% | 93.8% / 98.1% |
| medium-single | 3352.8 Mbps | 4596.0 Mbps | -27.1% | 97.8% / 150.0% |
| small-concurrent | 3910.4 Mbps | 5264.4 Mbps | -25.7% | 91.3% / 99.5% |
| small-single | 3572.8 Mbps | 4838.8 Mbps | -26.2% | 94.2% / 102.0% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | N/A | 4037.0 Mbps | N/A |
| lan | N/A | 824.3 Mbps | N/A |
| lossy | N/A | 69.8 Mbps | N/A |
| wan | N/A | 83.8 Mbps | N/A |
Summary
noq is 28.8% slower on average
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/noq/pr/536/docs/noq/ Last updated: 2026-05-07T08:14:33Z |
flub
left a comment
flub
left a comment
There was a problem hiding this comment.
Opiniated! I'm kind of -0 because it's making it harder for folks to migrate between Quinn and noq without meaningfully improving the api. chunks is not that bad, it is descriptive.
While you have a good point, when we decided to check the API this was not part of the criteria we established for what's good or bad API. We basically agreed to follow @rklaehn's lead and impose this new check (which, again, is a fair observation) will just make his job harder. Our API will not be compatible with quinn and I think the faster we accept this, better. If people come knocking on our door about that we can add a "migrating from quinn" section to the |
|
Ok, fair. I won't use the Quinn argument gain. Still -0 on it though:
|
|
I use write_bytes for fns that use Bytes in other crates, but I am also fine with keeping Chunks. So write_chunk and write_many_chunks? But what about the changed signature? I always found the If we keep chunks this would obviously also apply to the other PR #535 |
flub
left a comment
flub
left a comment
There was a problem hiding this comment.
Thanks for pointing out the return value change. Yeah, I agree with making is simpler. It seems odd to introduce yet another type for this.
So write_chunk and write_many_chunks?
That would get my approval :)
And yes, whatever we end up doing needs to be consistent in #535
flub
left a comment
flub
left a comment
There was a problem hiding this comment.
Some nits, but overall LGTM.
Co-authored-by: Floris Bruynooghe <flub@devork.be>
dignifiedquire
changed the title
…ytes (n0-computer#535) ## Description Remove Chunk usage in ordered read API Make read_chunk return just a Bytes. Also rename read_chunks to read_chunks_many (it already returns Bytes). Add a bytes_read fn for the rare case where you do need the offset despite being in ordered read mode. Not sure if people agree, but there was an inconsistency before between read_chunk (returns a Chunk, including offset) and read_chunks (fills a bunch of Bytes, no offset). Also it doesn't seem useful to have Chunk at all for ordered streams. You usually don't care about the offset unless you are reading unordered streams. So I would like to confine Chunk usage to only the (now separate) unordered read API. ## Breaking Changes noq::RecvStream::read_chunk returns a Bytes. noq::RecvStream::read_chunks renamed to read_many_chunks. ## Notes & open questions Note: while the [other related PR](n0-computer#536) is just renaming, this one is I think actually removing some weirdness. Why does read_chunk give you an offset but read_chunks does not. And there is no way to get the offset if you need it if you use read_chunks. --------- Co-authored-by: Floris Bruynooghe <flub@n0.computer>
3 participants
Description
Remove the Written struct from the public API.
write_many_chunks had a somewhat weird API. On the one hand it was mutating the passed buffers to zero out fully written buffers and split the half-written buffer, but then on the other hand it expected the caller to remove the empty buffers from the buffer array for the next call.
With the change we only return the total number of written bytes and remove the buffers from the slice ourselves. That makes the typical use simpler.
In the rare case where you do need the number of fully written buffers you can still get it.
Breaking Changes
noq::SendStream::write_bytes_many: renamed to write_many_chunks, and returns just the size.
noq::Written: removed
Notes & open questions