Conversation
|
This PR is part of a stack of 15 bookmarks:
Created with jj-stack |
There was a problem hiding this comment.
🟡 Changes recommended
The queue is not production-integrated and has correctness issues in fairness, backpressure recovery, and expiry boundaries.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a transport-level engine for scheduling outgoing datagrams with prioritization, fairness, expiry, backpressure, and byte limits.
Changes:
- Introduces the datagram queue engine and comprehensive unit tests.
- Adds ordering support to send-group identifiers.
- Exposes queue-related outcomes, capacity, IDs, and default expiry calculation.
File summaries
| File | Description |
|---|---|
neqo-transport/src/datagram_queue.rs |
Implements scheduling, expiry, eviction, and backpressure. |
neqo-transport/src/streams.rs |
Makes send-group IDs orderable. |
neqo-transport/src/lib.rs |
Re-exports queue-related public types. |
Review details
Suppressed comments (1)
neqo-transport/src/datagram_queue.rs:514
- The comment says overflow applies backpressure, but this branch never sets
blocked. If the first pressure event isOverflowed(for example, the byte budget is reached before the count watermark), a caller that stops enqueueing will never gettruefromresume_if_unblocked()after space is freed. Mark the queue blocked for this outcome as well and cover the overflow/resume transition with a test.
} else {
DatagramQueueOutcome::Overflowed { dropped }
};
- Files reviewed: 3/3 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3982 +/- ##
==========================================
+ Coverage 96.98% 97.09% +0.10%
==========================================
Files 113 114 +1
Lines 39657 40743 +1086
Branches 39657 40743 +1086
==========================================
+ Hits 38461 39558 +1097
+ Misses 1182 1169 -13
- Partials 14 16 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Merging this PR will regress 2 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | write_frames 5-streams 1-active |
14.9 µs | 15.3 µs | -2.54% |
| ❌ | Simulation | write_frames 20-streams 1-active |
17.9 µs | 18.3 µs | -2.11% |
| ⚡ | Simulation | coalesce_acked_from_zero 1000 ranges |
655.3 µs | 600.5 µs | +9.14% |
| ⚡ | Simulation | coalesce_acked_from_zero 10 ranges |
10.3 µs | 9.7 µs | +6.8% |
| ⚡ | Simulation | inbound_frame in-order |
1.3 ms | 1.2 ms | +6.31% |
| ⚡ | Simulation | inbound_frame 5%-dup |
1.3 ms | 1.2 ms | +6.29% |
| ⚡ | Simulation | simulated/pacing-false/same-seed |
77.2 ms | 72.7 ms | +6.25% |
| ⚡ | Simulation | coalesce_acked_from_zero 3 ranges |
3.8 µs | 3.6 µs | +6.09% |
| ⚡ | Simulation | mark_sent retransmit |
34.9 µs | 33.4 µs | +4.51% |
| ⚡ | Simulation | mark_acked fragmented |
221.5 µs | 212.4 µs | +4.32% |
| ⚡ | Simulation | coalesce_acked_from_zero 1 ranges |
2.8 µs | 2.7 µs | +4.08% |
| ⚡ | WallTime | neqo-neqo-cubic |
20.9 ms | 20.2 ms | +3.37% |
| ⚡ | Simulation | simulated/pacing-true/same-seed |
75.1 ms | 72.8 ms | +3.17% |
| ⚡ | Simulation | inbound_frame 2%-loss |
2.4 ms | 2.4 ms | +2.84% |
| ⚡ | Simulation | simulated/1000-streams/each-1000-bytes |
174.7 ms | 171.3 ms | +2.02% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing users/jesup/datagram_engine (1a3c09a) with main (28cacc8)
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the full diff (datagram_queue.rs, the lib.rs re-exports, and the SendGroupId Ord derive). No prior review comments existed on this PR. No unsafe, no FFI, and no untrusted-input parsing here — the risk surface is the scheduling/accounting logic and the backpressure contract. Byte accounting (total_bytes/total_count) is symmetric across every add/remove path and capacity() saturates, so I found no underflow paths.
A few things that apply to the PR as a whole rather than any one line:
Duplicated scheduler skeleton. send_stream.rs already implements per-send-group round-robin with within-group SendOrder priority: per_group: IndexMap<SendGroupId, PerGroupQueues> plus a cursor, where PerGroupQueues is itself a BTreeMap<SendOrder, _> served highest-first. This module reimplements the same shape with BTreeMap<SendGroupId, GroupQueue> plus rr_next. Two semantic divergences look unintentional rather than chosen: stream groups round-robin in insertion order, datagram groups in ascending group-ID order; and the stream side keeps a separate unordered bucket. Since a SendGroupId can legitimately be shared between a WebTransportSendStream and a datagram writable, it would be worth either factoring out the shared skeleton or at least documenting why the two cursors order groups differently. Relatedly, SendStreams::set_sendgroup rejects SendGroupId::new(0) from callers, while enqueue accepts it as an ordinary key standing in for the null sendGroup — the asymmetry deserves a line in the enqueue docs so callers know they must do the mapping themselves.
Test surface points at the wrong API. Almost every test drives drain, which is #[cfg(test)]-only; peek_next_len/take_next, the pair production will use, has two tests. The interesting interactions (budget boundary, expiry interleaved with partial takes, round-robin cursor across takes) are only covered through the test-only wrapper. Moving the shared-core cases onto peek/take would test the real contract at no extra cost, given drain is just a loop over take_next.
Configurability and benchmarking. max_queued_bytes is hard-coded to DEFAULT_MAX_QUEUED_BYTES with no setter outside tests, whereas the existing queue depth is tunable via ConnectionParameters::outgoing_datagram_queue — a matching byte-budget parameter would avoid a second, differently-shaped knob later. On cost: lowest_priority_key is a full scan of groups and runs once per evicted datagram, so a burst that evicts k entries is O(k·groups) inside enqueue. That is fine at today's group counts, but since this is destined for the packet-build path, a neqo-transport/benches case covering enqueue-under-budget-pressure would be worth landing with it.
Landing shape. As noted inline, the module compiles into the lib target with no consumer, which I expect trips dead_code under -D warnings, and several doc comments describe QuicDatagrams integration that isn't in this PR. If the intent is to land the engine separately from the wiring, both are easy to resolve (export the type, soften the docs); if the wiring is imminent, folding it in would make the design reviewable against a real caller — in particular whether DatagramQueueOutcome/resume_if_unblocked is the right split of responsibility versus letting the queue own the ConnectionEvents handle the way QuicDatagrams does today.
5cf2dbe to
662dce4
Compare
There was a problem hiding this comment.
Self-contained, well-documented scheduling engine with genuinely good test density. The rationale comments on the constants (DEFAULT_MAX_QUEUED_BYTES, the 1.25x/20 ms max-age derivation, the deliberate absence of a ceiling) are the kind of thing that saves the next reader an hour.
Previously raised, now resolved: inclusive expiry boundary (>= max_age); per-iteration re-check of the eviction victim's priority; blocked armed on Overflowed/Rejected; resume_if_unblocked documented for expiry paths and covered by expiry_alone_releases_a_blocked_queue; BTreeMap::retain in expire_old; filter_map in lowest_priority_key replacing the expect/unreachable!; next_expiry_is_none_when_the_deadline_overflows renamed; DatagramQueue now re-exported, so the dead_code concern is gone. Still open from the earlier round: the module-level docs (and drain's "production code (QuicDatagrams::write_frames) pulls …") still describe integration that quic_datagrams.rs does not have in this PR.
Whole-PR observations:
- Duplicated round-robin.
SendStreamsalready does per-SendGroupIdround-robin with a persisted cursor (send_stream.rs#L2237). This adds a second, independent implementation of the same cursor-over-BTreeMap-keys pattern. Worth extracting a small shared helper, especially since the doc comment explicitly points at the stream scheduler as the analogue — and since a shared helper is the natural seam if the cross-type fairness gap called out at line 307 is ever closed. expireandnext_expiryare O(groups × order-buckets) per call, and both are expected to run everyprocess_output. All datagrams in a queue share onemax_ageand timestamps are non-decreasing by insertion, so a single insertion-ordered side index (VecDeque<(Instant, SendGroupId, SendOrder)>) would makenext_expiryO(1) andexpireO(expired). Probably not worth it at ~213 queued datagrams, but it's the obvious next step if profiles say otherwise.- No benchmark.
enqueueunder byte pressure is the interesting path: eviction callslowest_priority_key()(O(groups)) once per evicted datagram, so a large datagram arriving into a many-group full queue is O(groups × evictions). Acriterioncase alongside the existing transport benches would pin the constant down before this is on the packet-build hot path. max_queued_bytesis not configurable from outside the crate. Tests poke the private field; there is no setter andDEFAULT_MAX_QUEUED_BYTESisn't exported, yetDatagramQueueCapacityreports it. Either addset_max_queued_bytes(symmetric withset_high_water_mark) or drop it from the capacity snapshot.DatagramOutcomeis exported but nothing constructs it in this PR — fine as staging, just noting the crate-root public surface currently includes a type with no producer.- Re: Copilot's byte-fairness comment — I think the turn-based tradeoff is reasonably argued in the doc comment, and DRR isn't worth the per-group state today. Worth noting that
peek_next_lenalready hands the caller the byte count a deficit scheme would need, so this stays cheap to revisit without an API change.
|
@jesup naive question: why is this still 1.6K LoC? I thought the queue moved down into the QUIC layer now, should the adaptions you need me much lighter-weight? |
|
This patch is >50% tests. Of the remainder, almost half are comments; there's ~400LoC of actual non-test code here, and this is the basis for all the other patches. I could pull a few bits out of the initial patch, but it wouldn't dramatically reduce the size, and it would add yet more patches to the stack. I will ask claude to re-evaluate all the patches and push on DRY to see if they can be made smaller. |
|
Also: the code saved was mostly just some api-layer-crossing code, which was a small part of this patch (it eliminated adding some API changes to transport, but this (in other patches) has a different set of API changes to receive data on max-age, send back expiry reasons, etc). I moved it down here at Max's request to avoid two levels of queues and to centralize the handling. Because I needed to move stuff down and redo interfaces, I took the opportunity to clean the stack to remove internal commits and provide a single clean commit, and also to minimize later PRs that modified the initial patch's design |
|
For reference, asking claude to apply (more) DRY to all the patches (including the ones I haven't uploaded yet) produced some small reductions, generally one function pulled out each in ~3 or 4 of the ~14 commits (in 10 or 11 PRs) that are currently uploaded -- one in this patch, but it didn't actually change the line count, or at most by 1 line. So no major reductions. Looking at several of the DRY patches, while they centralized code (good thing, reduces risk of changes not hitting every usage for example), none of them actually reduced LoC by more than a line or two, at most. |
There was a problem hiding this comment.
Reviewed the current head (fd69c54). This is self-contained new code with no unsafe, no untrusted-input parsing, and no wire-format surface — the only public API change outside the new module is adding PartialOrd/Ord to SendGroupId, which is additive. The risk concentrates in the eviction/backpressure state machine, and that is now genuinely well covered: the multi-eviction priority re-check, the over-budget admit, the Rejected charge-exact resume, the zero-budget escape and the FIFO-within-bucket victim all have dedicated tests, and I traced each of the byte arithmetic examples in them by hand.
A number of earlier findings are resolved at this revision: the inclusive expiry boundary now agrees with next_expiry; expire_old uses partition_point + one bulk drain and writes ids into the caller's Vec; blocked is an Option<usize> combined with max on the Rejected path; high_water_mark is NonZeroUsize; the charge bills Vec::capacity(); the types are re-exported so nothing is dead in a non-test build; and the module doc now says plainly that QuicDatagrams is not yet wired up (#3983).
Architectural notes, none blocking:
default_max_ageas a parameter is an unenforced invariant. It is threaded through four methods (set_max_age,expire,next_expiry,drain), and the inclusive-boundary contract — "wake atnext_expiryand you are guaranteed to shed something" — only holds if the caller passes the same value to both. Nothing in the type system says so, and the value legitimately moves asmin_rttis sampled. Holding it as a field (set_default_max_age(min_rtt), refreshed where the RTT estimate is updated) would make that structural and shrink four signatures; the cost is one more piece of state to keep fresh. Worth deciding now rather than in #3983, since the call sites land there.- Backpressure signal is momentary after
Overflowed. When eviction leaves any headroom,blocked = Some(1)is satisfied by the very nextresume_if_unblocked()with nothing having drained. That may be exactly what you want (there is room again), but it makes armingblockedon that path close to a no-op, and the doc — "drains back below the high water mark and has freed the charge it is waiting on" — reads as if a drain were required. Either is fine; they should agree. - Benchmark.
neqo-transport/benches/send_streams.rsbenchmarks the analogous stream scheduler, and this queue sits on the same per-datagram path withlowest_priority_key()(O(groups)) inside the enqueue loop andexpire/next_expirywalking every bucket once perprocess_output. A sibling bench covering enqueue-under-eviction and drain-with-many-groups would make those costs visible before #3983 puts them on the hot path. - Re @mxinden's thread on
dropped: Vec<DatagramId>and your confirmation that nothing in Gecko reads the outcome today: agreed, and it goes further than the one field — droppingDatagramIdentirely would collapseexpire/set_max_age'sVec<Option<DatagramId>>returns to plain counts and removeDatagramOutcome, which is currently constructed only in a test. Easier to add back when a consumer exists than to carry unexercised. - Minor: the test module discards
enqueue's#[must_use]outcome withdrop(...)~60 times. A two-linefn push(q: &mut DatagramQueue, ...)helper alongside the existingdrain_idswould read better and keep the assertions where they matter.
fd69c54 to
6138309
Compare
There was a problem hiding this comment.
Re-reviewed at 6138309. The scheduling/eviction/expiry logic reads correct now, and the rewrite since the last pass resolved the issues I raised then: the expiry boundary is inclusive and pinned by expiry_is_inclusive_at_the_deadline; the eviction loop re-derives the victim once per bucket instead of once per datagram; the Rejected-vs-Overflowed split and the per-charge resume signal are well specified and tested. Three new items inline.
A few PR-wide observations:
-
enqueuechargesdata.capacity()unshrunk. An earlier thread concluded thatenqueuewouldshrink_to_fitthe incoming buffer before charging it; the current code does not (datagram_queue.rs:538), andenqueue_charges_the_full_allocation_not_just_the_payloadnow asserts the opposite. Intentional revert? It matters becauseDatagramQueueCapacity::remaining_bytesis meant to drive a content-process credit grant, and as written the grant a writer consumes depends on that writer's allocation slack, which it cannot observe. If the realloc is too expensive for the hot path, chargingdata.len()and lettingPER_DATAGRAM_OVERHEADabsorb the slack is the deterministic alternative. -
Hot-path cost before wiring up.
next_expiry()→GroupQueue::oldest_timestamp()is O(total buckets) and will run on everyprocess_output, per session; with distinctsend_ordervalues per datagram that is one bucket per datagram (~207 at the default budget).lowest_priority_key()is O(groups) per eviction round. Both are fine at today's scale but neither is bounded by anything except the byte budget. Worth aneqo-transport/benches/case for enqueue-under-eviction andnext_expiryat a realistic bucket count before #3983 puts this on the send path — the crate already has the bench harness. -
Public API surface.
DatagramQueue,QueuedDatagram(all fieldspub),DatagramQueueCapacity,DatagramQueueOutcome,DatagramOutcome,DatagramIdanddefault_datagram_max_ageare all exported fromneqo-transportwhile nothing in-tree constructs them. Given the module doc already describes theQuicDatagramsintegration in the present tense and then disclaims it, landing this together with #3983 would both shrink the exported surface to what is actually needed and make the docs true on arrival. -
Responding to @martinthomson's still-open thread on global
send_orderas an eviction key: I agree with the author's reasoning that the application's own ordering beats group identity as a shedding signal, andbyte_budget_admits_equal_priority_newcomer_across_groupscovers the starvation case that motivated the concern. The residual asymmetry — victim selection breaks ties by group ID while admission does not — is documented atlowest_priority_key, soSendGroupId::new(0)(null sendGroup) is systematically the first victim at equal order. That is probably the right default, but it deserves a line in the eventual WebTransport-facing docs rather than only in a private helper's comment.
6138309 to
af19e15
Compare
There was a problem hiding this comment.
Re-reviewed at af19e15. The scheduling/eviction/expiry logic reads correct now, and the eviction loop's four outcomes (fits, outranked-with-nothing-evicted, outranked-after-evicting, queue-emptied-still-over-budget) are each pinned by a test. Four minor items inline; one of them is a real coverage hole with a livelock failure mode.
Resolved since my last pass (thanks): Duration * u32 overflow panic in the now-pub default_max_age; Some(0) high water mark wedge (NonZeroUsize); the two-field blocked/blocked_charge split; charge() billing len() instead of capacity(); expire_old's double Vec<QueuedDatagram> allocation; missing #[must_use] on set_max_age and its untested expire tail; the once-per-evicted-datagram victim re-derivation; set_max_queued_bytes having zero coverage; untested take_all; blocked being assigned rather than combined. Also Overflowed's Vec<DatagramId> → dropped: usize per @mxinden, and EXPLICIT_MAX_AGE_FLOOR = GRANULARITY plus the partition_point bulk drain per @martinthomson.
Still open from earlier rounds, not re-raised inline: next_expiry/expire are O(groups × send_order buckets) at a once-per-process_output call site; the WebTransport must-not-starve rule for a null [[SendOrder]] is unmodelled (SendOrder is a bare i64, unlike SendStreams' Option<SendOrder>); clippy::module_name_repetitions is warn workspace-wide with -D warnings and nothing in the file carries an #[expect] — CI hasn't reported on this SHA yet, so worth confirming locally.
Additional opinion on @martinthomson's Option<DatagramId> comment: I'd push on this one rather than keep the Option. By your own audit, WebTransport sends everything untracked today, so the common case allocates a Vec<Option<u64>> full of Nones purely so the caller can call .len() on it. (usize, impl Iterator<Item = DatagramId>) — or a &mut Vec<DatagramId> out-param plus a returned count — gives both callers exactly what they use with no allocation in the untracked case.
Architecture. Two things follow from landing the API ahead of its consumer (#3983), beyond the doc-tense discussion already had:
DatagramOutcomeis constructed nowhere in-tree; its sole test assertsid()round-trips. So theExpired/Droppedsplit — the part of the API a content-process consumer actually observes — is pinned only by its own definition. If the Gecko patch queue is close, wiring even one of the two producers (expire→Expired) in this PR would validate the shape.- No benchmark.
neqo-transport/benches/exists, andenqueueunder sustained eviction pressure plusnext_expiryare both per-packet-build costs once #3983 lands them inprocess_output. A criterion bench added here would make thePER_DATAGRAM_OVERHEAD/with_capacity(1)/BTreeMap-scan trade-offs measurable rather than argued.
No security concerns: no unsafe, no wire-format parsing, and all arithmetic on the eviction path is bounded by a caller-supplied budget with saturating_sub at the two places that can go negative.
af19e15 to
b31aa87
Compare
There was a problem hiding this comment.
Re-reviewed the whole file at b31aa87. The backpressure state machine now looks internally consistent: blocked: Option<usize> is combined rather than clobbered on Rejected, the resume predicate (below_watermark() && max_queued_bytes - total_bytes >= charge) is the exact inverse of the rejection condition, and the empty-queue escape hatch covers max_queued_bytes == 0. The eviction loop terminates unconditionally (lowest_priority_key only ever names a non-empty bucket, so evict_at always makes progress), and the accounting cannot underflow — every -= is derived from datagrams actually removed. No unsafe, no untrusted-input parsing.
Previously-raised items I can confirm resolved: the Duration * u32 overflow panic in default_max_age (now checked_mul), the one-age-comparison-per-datagram expiry (now partition_point + bulk drain, per @martinthomson), the triple allocation in expire, the priority check inside the multi-eviction loop, charge() billing len() instead of capacity(), the unexported types breaking -D warnings, high_water_mark: Option<NonZeroUsize>, the evict_lowest_priority dangling intra-doc link, #[must_use] on set_max_age, take_all coverage, and the // === test headings.
Architectural note that ties my inline comments together: this module reimplements three things neqo-transport already has — DatagramTracking (as Option<DatagramId>), OutgoingDatagramOutcome (as DatagramOutcome), and the QuicDatagrams::blocked / ConnectionEvent::OutgoingDatagramSpaceAvailable resume handshake (as blocked / resume_if_unblocked). Each is defensible in isolation, but the net effect is that QuicDatagrams will end up owning two of everything until #3983 collapses them, and in the meantime the crate root grows four public types with no in-tree caller. Reusing the existing types now costs little and removes that translation layer from #3983.
Two open points I raised earlier and won't re-litigate, but which I'd still weigh differently than the replies did:
- The module doc's present-tense claim that
QuicDatagramsholds oneDatagramQueueper session and round-robins between them is contradicted by the "Not yet wired up" paragraph three lines later. Documenting the end state of a stack is reasonable, but a reader landing here from the crate root can't tell which half is true today; phrasing the first paragraph in the future tense would cost nothing. PER_DATAGRAM_OVERHEAD = 64still undercounts whensend_ordervalues are distinct: each new order allocates aVecDequethat reserves 4 slots on first push, so a per-frame-unique-order sender (a plausible WebTransport pattern) charges ~64 B for ~200 B of real allocation. The byte budget is a memory backstop, so a systematic 3× undercount on that shape is worth either a comment or a per-bucket charge.
Responding to @martinthomson's point that the Option in the expiry list is unnecessary: agreed that a None isn't individually actionable, and if QueuedDatagram carried DatagramTracking instead, expire could return Vec<DatagramTracking> and the caller would pass each straight to ConnectionEvents::datagram_outcome, which already discards the untracked ones — keeping the count without the awkward Option.
Nothing blocking from me.
b31aa87 to
a547999
Compare
There was a problem hiding this comment.
Reviewed at a547999: the new datagram_queue.rs, the lib.rs re-exports, and the NULL_GROUP_ID move plus Ord derive on SendGroupId. No unsafe, no FFI, no untrusted-input parsing — the surface is scheduling policy and byte accounting, and the queue is outgoing-only, so a peer cannot drive it.
Correctness re-check. Byte/count accounting is symmetric across all four removal paths (evict_at, take_next, expire_old, take_all) and capacity() saturates, so I found no underflow. The eviction loop terminates (every evict_at frees ≥ PER_DATAGRAM_OVERHEAD) and total_bytes stays bounded by budget + one datagram even on the deliberate admit-over-budget path. blocked's combine-don't-clobber rule holds at all three write sites. peek_next_len and take_next resolve to the same entry. expire_old's partition_point precondition is now guarded by the debug_assert! in push.
Previously raised, resolved at this head (not re-raised): the evict_lowest_priority dangling intra-doc link (now Self::evict_at); blocked assignment clobbering a larger outstanding charge; the missing debug_assert! on per-bucket timestamp monotonicity; evicted_any duplicating evicted_count; dropped_ids removed from Overflowed; the triple-Vec expiry path collapsed into one output vec; partition_point bulk removal per @martinthomson; EXPLICIT_MAX_AGE_FLOOR = GRANULARITY; Default instead of new().
Architectural note. This lands 2,160 lines that nothing in the crate constructs — QuicDatagrams is untouched and still holds one unbounded VecDeque per connection (#3983). That is a defensible split, but it means the only thing exercising the scheduler is its own unit tests, and several public-API decisions (DatagramId tracking, QueuedDatagram's pub fields, take_all's reset semantics) are being frozen into neqo-transport's public surface before a single caller has validated them. Worth considering whether DatagramQueue and friends should stay pub(crate) until #3983 lands and only then be exported — that would also let the doc comments describe what exists rather than the stack's end state, which is the root of several threads above.
Docs. Agreeing with @larseggert's "still VERY wordy": at ~1.4 lines of prose per line of logic this reads as a design document embedded in a source file. The rationale comments (the 1.25×/20 ms derivation, the admission-vs-victim-selection asymmetry, why group ID breaks victim ties only) are genuinely valuable and worth keeping; the ones restating what the next line does, and the per-test paragraphs explaining what the assertion already says, are the ones to cut.
Benchmarks. None added for what is intended to run per process_output. See the inline note on GroupQueue::pop_front — benches/send_streams.rs is a ready template for the analogous scheduler.
Note: GitHub's API returned only 100 of the 124 existing review comments here and no pagination was available in this environment, so if any of the above duplicates something raised in the most recent round, please disregard it.
a547999 to
7feab9b
Compare
There was a problem hiding this comment.
I read the full review history (57 reviews, 131 comments — counts matched the index), traced the scheduling, eviction, expiry, accounting and backpressure state machines in neqo-transport/src/datagram_queue.rs, and checked the NULL_GROUP_ID move plus the Ord derive on SendGroupId. Everything I found that was substantive is already raised in a prior thread (multi-eviction priority re-check, blocked charge combining, charge(capacity()), partition_point monotonicity, the over-budget admit, doc tense vs. #3983, benchmarks, DatagramOutcome/DatagramId surface without a consumer).
No new defects survived filtering; posting no review comments.
Performance profiles for profiler.firefox.comBenchmarks (14)
|
7feab9b to
6b42740
Compare
There was a problem hiding this comment.
I found nothing new worth commenting on at 6b42740; the correctness pass's only finding, unreported IDs of evicted datagrams, was deliberately removed after @mxinden asked for it (thread r4023850251).
DatagramQueue holds one session's outgoing datagrams: round-robin between send groups (equal turns, not equal bytes), highest send_order first within a group, max-age expiry inclusive at timestamp + max_age, and a byte budget that evicts the lowest-priority datagram rather than the newest. enqueue reports Ok/AboveWatermark/Rejected/Overflowed, and anything but Ok arms the resume signal resume_if_unblocked reports once the queue drains back below the high water mark - by a send or by expiry. DatagramQueue is exported only so this commit's lib target has a live user; the next one, which gives QuicDatagrams a queue per session, takes the export back out.
6b42740 to
1a3c09a
Compare
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to
All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
|
Client/server transfer resultsPerformance differences relative to 28cacc8. Transfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Table above only shows statistically significant changes. See all results below. All resultsTransfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Download data for |
No description provided.