feat(s2n-quic-core): add spsc channel #1614

camshaft · 2023-01-27T23:30:47Z

Description of changes:

This change adds a spsc (single-producer/single-consumer) channel to s2n-quic-core. This will be used to split the endpoint up into multiple tasks, which should improve packet processing latencies.

Call-outs:

I needed the unsafe_assert macro from s2n-quic-crypto moved into s2n-quic-core for this. I also renamed it to assume!, since you already have to call it in an unsafe block, the name seemed redundant.

Testing:

The feature includes a bolero test that generates random sequences of events to push/pop items from the queue. I ran the test under kani but am waiting on model-checking/kani#2169 to be released. I also used loom to exhaustively explore atomic interleavings and assert that invariants hold in each case (I caught a few very subtle bugs with this - pretty cool!).

I've also included a benchmark that compares crossbeam-channel:

spsc/s2n/send_recv/4096 time:   [7.2070 µs 7.2119 µs 7.2182 µs]
                        thrpt:  [567.46 Melem/s 567.95 Melem/s 568.34 Melem/s]
Found 8 outliers among 100 measurements (8.00%)
  4 (4.00%) high mild
  4 (4.00%) high severe

spsc/crossbeam/send_recv/4096
                        time:   [36.976 µs 36.999 µs 37.027 µs]
                        thrpt:  [110.62 Melem/s 110.71 Melem/s 110.77 Melem/s]
Found 13 outliers among 100 measurements (13.00%)
  7 (7.00%) low severe
  2 (2.00%) high mild
  4 (4.00%) high severe

spsc/s2n/send_recv_iter/4096
                        time:   [4.5944 µs 4.5991 µs 4.6044 µs]
                        thrpt:  [889.57 Melem/s 890.61 Melem/s 891.52 Melem/s]
Found 25 outliers among 100 measurements (25.00%)
  10 (10.00%) low severe
  8 (8.00%) low mild
  4 (4.00%) high mild
  3 (3.00%) high severe

spsc/crossbeam/send_recv_iter/4096
                        time:   [34.846 µs 34.864 µs 34.886 µs]
                        thrpt:  [117.41 Melem/s 117.49 Melem/s 117.55 Melem/s]
Found 4 outliers among 100 measurements (4.00%)
  2 (2.00%) high mild
  2 (2.00%) high severe

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

WesleyRosenblum · 2023-02-02T02:32:51Z

quic/s2n-quic-core/src/sync/spsc/slice.rs

+
+impl<'a, T> Slice<'a, Cell<T>> {
+    #[inline]
+    pub unsafe fn assume_init(self) -> Slice<'a, UnsafeCell<T>> {


What is the purpose of this function?

Added a comment inline

WesleyRosenblum · 2023-02-02T02:41:28Z

quic/s2n-quic-core/src/sync/spsc/slice.rs

+        if let Some(cell) = self.head.0.get(index) {
+            cell
+        } else {
+            assume!(index >= self.head.0.len());


How do you know to use assume! here instead of a regular debug_assert!?

They're they exact same when debug assertions are enabled. However, when they're not, the assume tells the compiler that it's impossible for this check to be false and then the compiler will optimize the code based on that assumption. Without extensive testing/proofs it's generally best to stick with a debug_assert. But in this case, I'm using loom, kani, and bolero so the confidence is quite high that these assumptions always hold.

I guess my question is more, is there something particular about this code that makes you believe the compiler would benefit from the assume! and give us better performance, or is it more that we might as well use assume! since all the testing and proofs give us high confidence that its safe.

Yeah, it's mostly the latter. I want to try and give the compiler as much information as we can about the assumptions and prove that they will always hold. In this particular case, I didn't dive into the asm to know if it made a difference in the final outcome.

WesleyRosenblum · 2023-02-02T02:45:47Z

quic/s2n-quic-core/src/sync/spsc/state.rs

+    }
+
+    #[inline]
+    pub fn acquire_filled(&mut self) -> Result<bool> {


Can you add a comment on what the boolean in the result means? Same on acquire_capacity

WesleyRosenblum · 2023-02-02T02:49:05Z

quic/s2n-quic-core/src/sync/spsc/recv.rs

+
+    #[inline]
+    pub fn clear(&mut self) -> usize {
+        // don't update the cursor so the caller can observe any updates through peek


Isn't the cursor being updated on line 115?

Updated the comment

WesleyRosenblum · 2023-02-02T02:55:54Z

quic/s2n-quic-core/src/sync/spsc/recv.rs

+        let len = pair.len();
+
+        for entry in pair.iter() {
+            unsafe {


There are several unsafe blocks in these files, could you maybe summarize in a comment somewhere any notes on why/how these operations are safe?

WesleyRosenblum · 2023-02-02T03:00:44Z

quic/s2n-quic-core/src/sync/spsc/tests.rs

+}
+
+#[test]
+// TODO enable this once https://github.com/model-checking/kani/pull/2172 is merged and released


Looks like it is merged, just waiting on release

WesleyRosenblum · 2023-02-02T03:06:55Z

quic/s2n-quic-core/src/sync/spsc/state.rs

+    #[inline]
+    pub fn capacity(&self) -> usize {
+        self.invariants();
+        self.capacity - 1


The cursor management is based on the one in VecDeque, which requires that we add one and round up to the next power of two. It basically boils down to the fact that if you allow the caller to write the full capacity, then you can't distinguish between an empty and a full queue because the cursors are equal. So if you limit capacity to total - 1, you can assume that when cursors are equal, the queue is empty.

WesleyRosenblum · 2023-02-03T18:04:54Z

quic/s2n-quic-core/src/sync/spsc/primitive.rs

+///
+/// This can be used to optimize the code to avoid needless calculations.
+pub trait IsZst {
+    const IS_ZST: bool;


Not going to block on this, but I would call this IS_ZERO_SIZED

I was following the stdlib convention here, since the design of the queue cursors is based on VecDeque.

camshaft force-pushed the camshaft/spsc branch from 205c11a to 8d521aa Compare January 27, 2023 23:55

camshaft marked this pull request as ready for review January 28, 2023 00:17

camshaft added 5 commits January 27, 2023 20:26

feat(s2n-quic-core): add spsc channel

e9ce431

remove state behavior

d2b34e7

add benchmarks

0bd8392

deduplicate assume/unsafe_assert

f00d59a

add kani and loom tests

8c79376

camshaft force-pushed the camshaft/spsc branch from 8d521aa to 8c79376 Compare January 30, 2023 17:02

camshaft mentioned this pull request Jan 30, 2023

Unhandled variant in ParserItem JSON model-checking/kani#2169

Closed

camshaft added 2 commits February 1, 2023 14:36

fix bugs found with loom

14c6483

fix loom ci job

eb3bc58

WesleyRosenblum reviewed Feb 2, 2023

View reviewed changes

pr feedback

a6a5143

WesleyRosenblum reviewed Feb 3, 2023

View reviewed changes

WesleyRosenblum approved these changes Feb 3, 2023

View reviewed changes

camshaft merged commit a1e7606 into main Feb 3, 2023

camshaft deleted the camshaft/spsc branch February 3, 2023 18:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(s2n-quic-core): add spsc channel #1614

feat(s2n-quic-core): add spsc channel #1614

camshaft commented Jan 27, 2023 •

edited

Loading

WesleyRosenblum Feb 2, 2023

camshaft Feb 2, 2023

WesleyRosenblum Feb 2, 2023 •

edited

Loading

camshaft Feb 2, 2023

WesleyRosenblum Feb 3, 2023 •

edited

Loading

camshaft Feb 3, 2023

WesleyRosenblum Feb 2, 2023

WesleyRosenblum Feb 2, 2023

camshaft Feb 2, 2023

WesleyRosenblum Feb 2, 2023

WesleyRosenblum Feb 2, 2023

WesleyRosenblum Feb 2, 2023

camshaft Feb 2, 2023

WesleyRosenblum Feb 3, 2023

camshaft Feb 3, 2023

feat(s2n-quic-core): add spsc channel #1614

feat(s2n-quic-core): add spsc channel #1614

Conversation

camshaft commented Jan 27, 2023 • edited Loading

Description of changes:

Call-outs:

Testing:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WesleyRosenblum Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WesleyRosenblum Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camshaft commented Jan 27, 2023 •

edited

Loading

WesleyRosenblum Feb 2, 2023 •

edited

Loading

WesleyRosenblum Feb 3, 2023 •

edited

Loading