Skip to content

Conversation

Ralith
Copy link
Collaborator

@Ralith Ralith commented Aug 26, 2023

Work towards #1576, #599.

This replaces the requirement for callers to pass EndpointEvents around with shared memory managed by the proto layer, using a purpose-built lock-free intrusive mpsc queue called SharedList. This simplifies the proto API and may be more efficient, although endpoint events are not typically on a hot path.

Intensive review is required for the implementation of SharedList, which contains internal unsafety and relies on careful use of memory orderings for correctness. I couldn't find an off-the-shelf, well-audited data structure with similar semantics.

Alternative approaches to this API change include a std mpsc channel and the pre-existing EndpointEvent enum, or an mpsc channel of Arc<EndpointEvents> with an additional AtomicBool flag to minimize redundant messaging. These would avoid rolling our own data structure, at the cost using a more reviewed but needlessly complex and less specialized data structure. Further, SharedList may prove useful elsewhere within quinn: a similar queue-once MPSC pattern exists in streams, and might be leveraged to increase application parallelism when performing stream I/O (see discussion in #1433).

Ralith added 2 commits August 26, 2023 16:38
This was only needed to determine the base time for the next
CID retirement timer, for which the marginal delay caused by fetching
a new `Instant::now()` on demand is harmless.
Simplifies the -proto API and reasoning about memory use
@Ralith Ralith force-pushed the shared-events branch 3 times, most recently from 01e26a1 to 3fe2a6b Compare August 27, 2023 00:29
@Ralith
Copy link
Collaborator Author

Ralith commented Aug 27, 2023

In retrospect this might be more gracefully posed by first refactoring to use an internal mpsc queue, then replacing the mpsc queue with the custom one. Let me know if that's worth investing time in.

@Ralith Ralith marked this pull request as draft January 4, 2024 19:28
@Ralith
Copy link
Collaborator Author

Ralith commented Jan 4, 2024

Closing in favor of #1726, though if we continue in that direction we may eventually want to port the intrusive queue over.

@Ralith Ralith closed this Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant