feat(s2n-quic-dc): only poll accepted streams that are ready #2409
Conversation
camshaft
force-pushed
the
camshaft/dc-waker-set
branch
4 times, most recently
from
61e1b28 to
e43dce3
Compare
| is_active | ||
| }); | ||
| // we did a full scan so reset the value | ||
| self.gc_count = 0; |
There was a problem hiding this comment.
This retains feels possibly expensive - it's going to be near random in the common case I think, so lots of shifting/copying of the indices. Plus still O(n) for worker count.
If we don't do this at all, I think the downside is that when evicting due to sojourn we would need to search for the right entry? I think just popping isn't enough, but maybe a heap which we use to incrementally sort ends up cheaper?
There was a problem hiding this comment.
Yeah it's tough to say. I think it's definitely better than what we're doing today, but let me try refactoring to use a heap instead.
There was a problem hiding this comment.
Fixed in e237253. I ended up going with a "linked list" structure (reusing the existing workers allocation) that made push/pop/remove all O(1).
| /// Registers a waker with the given ID | ||
| pub fn waker(&mut self, id: usize) -> Waker { | ||
| // reserve space in the locally ready set | ||
| self.ready.resize_for_id(id); |
There was a problem hiding this comment.
Isn't our size fixed? We manage to fix the worker set so it feels surprising we can't do that here. I think that would eliminate the locking entirely...
There was a problem hiding this comment.
It is fixed after we create all of the Wakers. This just made it so you don't have to specify the capacity up front. In the critical path, this isn't used at all, since those wakers are cached in the slot.
| let shift = self.shift; | ||
| let id = self.index * SLOT_SIZE + shift; | ||
| let mask = 1 << shift; | ||
| self.shift += 1; |
There was a problem hiding this comment.
This looks linear to me - I would use trailing_zeros to skip to next 1.
| /// * `entries` is only managed by [`List`] | ||
| /// * `idx` is less than `usize::MAX` | ||
| #[inline] | ||
| pub unsafe fn pop<L>(&mut self, entries: &mut [L]) -> Option<usize> |
There was a problem hiding this comment.
I feel like the bounds check avoidance is very unlikely to meaningfully contribute performance and the loss of safety feels not worth it as such. Can we start with making this safe (panicking if we have issues)?
camshaft
force-pushed
the
camshaft/dc-waker-set
branch
from
6f9795c to
2b3b9c1
Compare
|
Unrelated test failures are fixed in #2412 |
Description of changes:
The TCP currently polls all pending streams on every task wakeup:
s2n-quic/dc/s2n-quic-dc/src/stream/server/tokio/tcp.rs
Lines 333 to 335 in e4a2365
This isn't super efficient since only a handful of streams may be ready to be polled again.
This change, instead, adds a
waker::Setwhich hands out specific wakers to each stream and then tracks which ones actually woke up. It then only polls those tasks on wakeup, skipping the still-pending ones.Call-outs:
I refactored the TCP acceptor a bit to make it easier to test, especially write some fuzz tests for since the management logic is getting quite complicated.
Testing:
I added quite a few tests. Most interesting one is probably the fuzz test for the accept manager, which ensures all struct invariants are kept no matter which order events occur:
s2n-quic/dc/s2n-quic-dc/src/stream/server/tokio/tcp/manager.rs
Lines 617 to 641 in e43dce3
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.