Add support for WebTransport SendOrder to neqo (issue #1432) #1437

jesup · 2023-05-18T21:40:17Z

No attempt at fairness within a sendorder (or for unordered streams) is attempted.

A few typos are corrected as well

…s within a sendorder (or for unordered streams) is attempted

neqo-transport/src/connection/mod.rs

…s within a sendorder (or for unordered streams) is attempted

jesup · 2023-05-19T13:30:15Z

I had added the custom Ord impl to switch the order, but didn't actually modify it to switch the order... :-)
Added logging for sendorder changes

KershawChang · 2023-05-22T12:42:24Z

I think we should have some tests in neqo as well.
It should be easier to test this in neqo than writing a wpt test.

neqo-http3/src/connection.rs

neqo-http3/src/connection_client.rs

neqo-http3/src/features/extended_connect/webtransport_session.rs

neqo-transport/src/connection/mod.rs

neqo-transport/src/streams.rs

martinthomson · 2023-05-23T08:13:27Z

neqo-transport/src/connection/tests/stream.rs

+        assert_eq!(
+            out.as_dgram_ref().is_some(),
+            (d_num + 1) % usize::try_from(DEFAULT_ACK_PACKET_TOLERANCE + 1).unwrap() == 0
+        );


We're not really testing for ACKs, so maybe you don't need to worry about checking this.

Cut-and-paste from the next test, I can remove that

Though if I make it a function I might want to keep it

neqo-transport/src/connection/tests/stream.rs

neqo-transport/src/streams.rs

neqo-transport/src/connection/tests/stream.rs

KershawChang

I noticed there are some failed tests, so I think this patch still has more to do.

neqo-http3/src/connection.rs

neqo-http3/src/features/extended_connect/webtransport_session.rs

neqo-http3/src/connection_client.rs

neqo-transport/src/send_stream.rs

neqo-transport/src/streams.rs

…essed in stream_id order; the other was a bug in remove_terminal()

KershawChang

This looks good to me, but I think this still needs @martinthomson 's approval.

neqo-transport/src/send_stream.rs

neqo-transport/src/connection/tests/stream.rs

neqo-http3/src/connection.rs

martinthomson · 2023-05-26T16:48:12Z

neqo-transport/src/send_stream.rs

+    // This avoids the complexity of trying to hold references to the
+    // Streams which are owned by the IndexMap.
+    sendordered: BTreeMap<SendOrder, HashSet<StreamId>>,
+    regular: Vec<StreamId>, // streams with no SendOrder set, sorted in stream_id order


Did you consider iterating map instead? You would need to mark any stream that has a sendorder somehow (i.e., skip if self.sendorder.is_some()), but then you save the overhead of maintaining this collection.

Possibly, but: In stream-per-frame (and perhaps most instances where sendorder is used), all or almost all the streams will have sendorders, and so we'll iterate over map but ignore almost or all the iterations. That seems a higher overhead (every time we're sending packets) than the maintenance overhead.

Hmm. Having a count of the number of unordered streams would allow us to add .take(count) when iterating, which would shortcut very nicely. WDYT?

I'm not sure I understand your suggestion here

If, rather than maintaining a collection of streams that are unordered, you merely kept a count, then in the two cases we think most likely:

All streams are unordered. You iterate over the main IndexMap directly, but .take(x) where x = self.map.len(), means that it is a full iteration.

All streams are ordered. You iterate over the main IndexMap, but .take(0), which turns into a no-op.

The advantage is that you don't have an indirection for the first iteration; you iterate the collection directly. In the second, you short-circuit that iteration (because iterators are lazy and .take(0) doesn't hit the underlying iterator at all. It's only when you have a mix that you end up paying extra, but that's not a situation we think likely. If there are a few streams that are unordered, then you'll have a few to track, but maybe you can count the number of unordered streams with unsent data so that you have to hit the collection less often.

neqo-transport/src/send_stream.rs

neqo-transport/src/streams.rs

neqo-transport/src/connection/tests/stream.rs

martinthomson · 2023-05-26T18:57:07Z

neqo-http3/src/features/extended_connect/webtransport_session.rs

+    fn set_sendorder(&mut self, _conn: &mut Connection, _sendorder: Option<SendOrder>) {
+	// Not relevant on session
+    }


I think that we should let this pass through. We will simply not call it in gecko (who knows, maybe this makes sense at some point).

Pass through to where?

I am suggesting that you change the send order, rather than do nothing

Sessions don't have a sendorder to set, so I'm still confused by your suggestion. Unless you mean "set the sendorder of all streams to N", which would be unhelpful and there's no reason for it.

No, I'm just suggesting that you change the session stream's sendorder. Yes, it doesn't make much sense, but a no-op is far more surprising than a function that does the expected thing, even if it shouldn't be used.

Maybe add a debug assertion here?
I assume this function should not be called at all.

I don't see why this wouldn't work:

self.borrow_mut().control_stream_send.set_sendorder(conn, sendorder)

neqo-http3/src/connection_client.rs

jesup · 2023-05-29T04:07:21Z

Updated for almost all the comments; will look at using Vec instead of HashSet for each sendordered group tomorrow

neqo-transport/src/send_stream.rs

KershawChang · 2023-06-03T10:36:50Z

@jesup please also fix all format and clippy errors. Thanks.

martinthomson · 2023-06-05T00:58:41Z

neqo-http3/src/features/extended_connect/webtransport_session.rs

+    fn set_sendorder(&mut self, _conn: &mut Connection, _sendorder: Option<SendOrder>) {
+	// Not relevant on session
+    }


I don't see why this wouldn't work:

self.borrow_mut().control_stream_send.set_sendorder(conn, sendorder)

martinthomson · 2023-06-05T00:59:14Z

neqo-http3/src/lib.rs

    /// # Errors
-    /// Error my occure during sending data, e.g. protocol error, etc.
+    /// Error my occur during sending data, e.g. protocol error, etc.


Suggested change

/// Error my occur during sending data, e.g. protocol error, etc.

/// Errors might occur during sending data, e.g., protocol error.

martinthomson · 2023-06-05T01:02:37Z

neqo-http3/src/send_message.rs

@@ -271,6 +271,10 @@ impl SendStream for SendMessage {
        self.stream.has_buffered_data()
    }

+    fn set_sendorder(&mut self, _conn: &mut Connection, _sendorder: Option<SendOrder>) {
+	// Not relevant for SendMessage


Again. self.stream.set_sendorder(conn, sendorder) seems workable here, even if we don't end up using it. (Yes, this requires an additional step to implement that on BufferedStream, but it's a trivial thing.)

Yes, we're building for webtransport, but this type has a direct relationship with an underlying stream and a random noop is a surprise I'd prefer to avoid.

martinthomson · 2023-06-05T01:03:56Z

neqo-transport/src/connection/tests/stream.rs

+    stream_id: StreamId,
+}
+
+#[cfg(test)]


Suggested change

#[cfg(test)]

martinthomson · 2023-06-05T01:06:59Z

neqo-transport/src/streams.rs

+    if self.sendorder.is_some() && other.sendorder.is_some() {
+      // We want reverse order (high to low) when both values are specified.
+      other.sendorder.cmp(&self.sendorder)


The alternative (I keep coming up with these, sorry) might be better:

Suggested change

if self.sendorder.is_some() && other.sendorder.is_some() {

// We want reverse order (high to low) when both values are specified.

other.sendorder.cmp(&self.sendorder)

if let (Some(s), Some(o)) = (&self.sendorder, &other.sendorder) {

// We want reverse order (high to low) when both values are specified.

o.cmp(s)

martinthomson · 2023-06-05T01:07:58Z

neqo-transport/src/streams.rs

+                self.send.insert(
+                    new_id, stream,
                );


Is this really what cargo fmt produces? I'm surprised that it's breaking across lines like this.

martinthomson · 2023-06-05T01:10:55Z

neqo-transport/src/connection/mod.rs

+    /// # Errors
+    /// None


Suggested change

/// # Errors

/// None

martinthomson · 2023-06-05T01:12:01Z

neqo-http3/src/connection.rs

+    /// Set the stream SendOrder.
+    /// # Errors
+    /// Returns InvalidStreamId if the stream id doesn't exist


Clippy will complain here.

Suggested change

/// Set the stream SendOrder.

/// # Errors

/// Returns InvalidStreamId if the stream id doesn't exist

/// Set the stream `SendOrder`.

/// # Errors

/// `InvalidStreamId` if the stream id doesn't exist.

jesup · 2023-08-30T20:01:42Z

This was landed as part of PR #1443

Add support for WebTransport SendOrder to neqo. No attempt at fairnes…

76452ef

…s within a sendorder (or for unordered streams) is attempted

jesup requested review from KershawChang and martinthomson as code owners May 18, 2023 21:40

valenting reviewed May 19, 2023

View reviewed changes

neqo-transport/src/connection/mod.rs Outdated Show resolved Hide resolved

jesup added 3 commits May 19, 2023 09:01

Add support for WebTransport SendOrder to neqo. No attempt at fairnes…

bb77b25

…s within a sendorder (or for unordered streams) is attempted

merge

ccff857

Change comment for stream_sendorder errors

d4d3363

Add unit test for sendorder

26ac19e

martinthomson reviewed May 23, 2023

View reviewed changes

KershawChang requested changes May 23, 2023

View reviewed changes

jesup added 5 commits May 24, 2023 12:19

Responses to comments

994faa2

Fix other test failures -- one relies on unordered streams being proc…

bb8553b

…essed in stream_id order; the other was a bug in remove_terminal()

Switch to a sorted Vec for the not-sendordered streams for efficiency.

e7b0563

Fix lints

d8d333a

Lint fix

5e43fb5

KershawChang reviewed May 26, 2023

View reviewed changes

neqo-transport/src/send_stream.rs Outdated Show resolved Hide resolved

neqo-transport/src/connection/tests/stream.rs Outdated Show resolved Hide resolved

martinthomson reviewed May 26, 2023

View reviewed changes

resolve comments

2b81bd5

move sendorder HashSets to Vecs

4bff968

martinthomson reviewed May 30, 2023

View reviewed changes

neqo-transport/src/send_stream.rs Outdated Show resolved Hide resolved

use let vec = if let ...

f13d785

jesup requested a review from KershawChang June 1, 2023 16:14

KershawChang requested a review from martinthomson June 2, 2023 15:05

KershawChang approved these changes Jun 3, 2023

View reviewed changes

martinthomson reviewed Jun 5, 2023

View reviewed changes

martinthomson linked an issue Jun 8, 2023 that may be closed by this pull request

Support SendOrder for WebTransportStream #1432

Open

jesup added 12 commits June 12, 2023 09:45

sendorder_fixes

f7db2f9

fix rebase problem

eef62d8

Merge branch 'main' into sendorder

c622ee6

Merge branch 'mozilla:main' into sendorder

5655070

lint fixes

d6ea89e

more lint fixes

721689e

cargo fmt

ec5079d

Merge branch 'main' into sendorder

1788da8

correct unbitrot

29e5838

Merge branch 'mozilla:main' into sendorder

2161005

fix formatting

ba0d578

formatting 2

6eda53c

jesup closed this Aug 30, 2023

jesup deleted the sendorder branch August 30, 2023 20:01

jesup restored the sendorder branch September 14, 2023 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for WebTransport SendOrder to neqo (issue #1432) #1437

Add support for WebTransport SendOrder to neqo (issue #1432) #1437

jesup commented May 18, 2023

jesup commented May 19, 2023

KershawChang commented May 22, 2023

martinthomson May 23, 2023

jesup May 24, 2023 •

edited

jesup May 24, 2023

KershawChang left a comment

KershawChang left a comment

martinthomson May 26, 2023

jesup May 27, 2023

martinthomson May 28, 2023

jesup Jun 1, 2023

martinthomson Jun 2, 2023 •

edited

martinthomson May 26, 2023

jesup May 26, 2023

martinthomson May 28, 2023

jesup Jun 1, 2023

martinthomson Jun 2, 2023

KershawChang Jun 3, 2023

martinthomson Jun 5, 2023

jesup commented May 29, 2023

KershawChang commented Jun 3, 2023

martinthomson Jun 5, 2023

martinthomson Jun 5, 2023

martinthomson Jun 5, 2023

martinthomson Jun 5, 2023

martinthomson Jun 5, 2023

martinthomson Jun 5, 2023

martinthomson Jun 5, 2023

martinthomson Jun 5, 2023

jesup commented Aug 30, 2023

	/// Error my occur during sending data, e.g. protocol error, etc.
	/// Errors might occur during sending data, e.g., protocol error.

Add support for WebTransport SendOrder to neqo (issue #1432) #1437

Add support for WebTransport SendOrder to neqo (issue #1432) #1437

Conversation

jesup commented May 18, 2023

jesup commented May 19, 2023

KershawChang commented May 22, 2023

Choose a reason for hiding this comment

jesup May 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KershawChang left a comment

Choose a reason for hiding this comment

KershawChang left a 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

martinthomson Jun 2, 2023 • edited

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

jesup commented May 29, 2023

KershawChang commented Jun 3, 2023

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

jesup commented Aug 30, 2023

jesup May 24, 2023 •

edited

martinthomson Jun 2, 2023 •

edited