Read ordered chunks from RecvStream #952

stammw · 2020-12-29T19:57:47Z

Hello !

I've started trying to implement #929.

Still I'd better be adding some tests before its ready. So I'm setting this PR WIP for a first feedback.

quinn-proto/src/connection/assembler.rs

djc · 2020-12-29T20:08:10Z

quinn-proto/src/connection/streams.rs

+            hash_map::Entry::Occupied(e) => e,
+        };
+        let rs = entry.get_mut();
+        match rs.read_chunk() {


This is pretty repetitive, can we factor out a bunch of the match here (ideally in a separate commit)? Could apply to read_unordered() as well, ideally, but just between read() and read_chunk() would be good if that makes it hard.

I got a solution I find a little complex in feb3b63, what do you think about this?

It looks like you can just inline the IntoReadResult into the closure passed to try_read()? We already have the nested map() calls anyway. Or am I missing something?

Ho yeah maybe ! 😅
For the moment I get stuck because self.add_read_credits(len as u64) needs to mutate self to compute transmit_max_data, and it is already mutably borrowed by the closure. But I'll try a little harder in a bit.

quinn-proto/src/connection/assembler.rs

djc · 2020-12-29T20:09:55Z

quinn-proto/src/connection/streams.rs

+        if let Some(bytes) = self.assembler.read_chunk()? {
+            Ok(Some(bytes))
+        } else {
+            self.read_blocked().map(|()| None)
+        }


A match expression would be more readable and more compact, I think?

quinn/src/streams.rs

Ralith

Thanks for working on this!

Ralith · 2020-12-29T22:03:33Z

quinn/src/streams.rs

@@ -347,6 +347,11 @@ where
        }
    }

+    /// [`read_chunk()`]: RecvStream::read_chunk


This should be documented, including an explicit warning against relying on the chunk boundaries having any particular relationship to the sender's writes.

I got this a better comment. I'm not sure I grasped everything you'd like it to say?

Ralith

Wording tweaks for the comments:

Ralith · 2020-12-30T20:32:58Z

quinn/src/streams.rs

+        ReadChunk { stream: self }
+    }
+
+    /// [`read_chunk()`]: RecvStream::read_chunk


Suggested change

/// [`read_chunk()`]: RecvStream::read_chunk

/// Foundation of [`read_chunk()`]: RecvStream::read_chunk

Ralith · 2020-12-30T20:33:46Z

quinn/src/streams.rs

+        self.poll_read_generic(cx, |conn, stream| conn.inner.read_unordered(stream))
+    }
+
+    /// Read the next segment of data from next valid offset in the stream.


Suggested change

/// Read the next segment of data from next valid offset in the stream.

/// Read the next segment of data

Ralith · 2020-12-30T20:42:11Z

quinn/src/streams.rs

+    /// Yields a segment of data consecutive to the last ordered read offset, or `None`
+    /// if the stream was finished. Segments are received in order.
+    ///
+    /// Beware, the returned chunks bounds won't always match those written by the peer.
+    ///
+    /// Chunk reads have reduced overhead and higher throughput, and should therefore be
+    /// preferred when applicable.


Tweaked wording a bit:

Suggested change

/// Yields a segment of data consecutive to the last ordered read offset, or `None`

/// if the stream was finished. Segments are received in order.

///

/// Beware, the returned chunks bounds won't always match those written by the peer.

///

/// Chunk reads have reduced overhead and higher throughput, and should therefore be

/// preferred when applicable.

/// Yields a segment of data beginning immediately after the last data yielded by

/// `read` or `read_chunk`, or `None` if the stream was finished.

///

/// Slightly more efficient than `read` due to not copying. Chunks boundaries

/// do not correspond to peer writes, and hence cannot be used as framing.

Ralith · 2020-12-30T20:49:47Z

quinn-proto/src/connection/mod.rs

+    /// window is filled. When in-order delivery is required, the siblings `read()` or `read_chunk()`
+    /// method should be used.


Suggested change

/// window is filled. When in-order delivery is required, the siblings `read()` or `read_chunk()`

/// method should be used.

/// window is filled. When in-order delivery is required, the sibling `read()` or `read_chunk()`

/// methods should be used.

djc · 2021-01-02T20:36:40Z

(Note that #961 is blocked on this, so would be nice to follow through soon...)

Ralith · 2021-01-02T20:54:50Z

My wording in #961 was unclear--I don't actually consider it blocked, I'm just holding off on adding additional refactoring that would make rebasing this PR harder than it otherwise would be.

Matthias247 · 2021-01-04T02:27:41Z

While we are at it: I would also appreciate a vectored read API - which can retrieve more than 1 chunk at a time. Because locking a Mutex and potentially doing a task wakeup for reading just 1kB of data (due to Quic packet sizes) is fairly unefficient.

One API I've used in a project for that is:

async fn read(&mut self, &mut [Bytes]) -> Result<(usize, bool), Error>

Where the caller provides an array (initialized with dummy elements / Bytes::new()) - and the implementation fills as many array items as possible and returns the number of chunks read in the first return value. Second return value is for indicating end of stream.

stammw · 2021-01-04T22:32:09Z

I finally got the read methods factorization right (yeah the IntoResult thing was weird...).

I've addesd a test for assembler::read_chunk() in the first commit.

All the wording changes have been addressed.

A new commit contains @Matthias247 suggestion. This is still WIP as I'm waiting for your feedback on this public API change, and it should probably be tested in the functional test module.

djc · 2021-01-04T22:42:20Z

Did you see #955?

stammw · 2021-01-04T22:51:41Z

I should probably have paid more attention, sorry... Now the IntoResult solution is totally removed here. I'm a little too tired to judge which one is better.

djc · 2021-01-04T22:59:31Z

Maybe @Ralith or @Matthias247 want to take a look which version they prefer.

Matthias247 · 2021-01-06T01:46:10Z

quinn-proto/src/connection/assembler.rs

+
+            if chunk.offset > self.bytes_read {
+                // Next chunk is after current read index
+                return Ok(None);


Isn't None end of stream? i'm not sure how this function is used, but it might need to return a blocked status instead.

This method works as it's siblings: it returns None when there is nothing to assemble. Blocked status is managed at the stream level in quinn-proto::connection::streams::Streams::read_chunk().

See the chunks_dedup() test at the end of this file for an example of expected behavior.

Matthias247 · 2021-01-06T01:50:29Z

Maybe @Ralith or @Matthias247 want to take a look which version they prefer.

I skimmed both commits, and I have a slight preference for this one since I found the ReadResult/StreamReadResult/IntoReadResult types a bit harder to follow.

I think this change currently has a good balance between code reuse and readability.

@stammw Also thanks for incorporating my feedback!

quinn-proto/src/connection/streams.rs

stammw · 2021-01-06T07:12:31Z

The current read methods factorization solution (without IntoResult) is actually better IMO.

I've added a test for read_chunks(), so this is ready to be merged if you guys like the current state of this PR.

djc

LGTM, modulo small doc nits.

djc · 2021-01-06T07:27:51Z

quinn/src/streams.rs

+    /// Slightly more efficient than `read` due to not copying. Chunks boundaries
+    /// do not correspond to peer writes, and hence cannot be used as framing.


Suggested change

/// Slightly more efficient than `read` due to not copying. Chunks boundaries

/// do not correspond to peer writes, and hence cannot be used as framing.

/// Slightly more efficient than `read` due to not copying. Chunk boundaries

/// do not correspond to peer writes, and hence cannot be used as framing.

djc · 2021-01-06T07:28:03Z

quinn/src/streams.rs

+    /// Slightly more efficient than `read` due to not copying. Chunks boundaries
+    /// do not correspond to peer writes, and hence cannot be used as framing.


Suggested change

/// Slightly more efficient than `read` due to not copying. Chunks boundaries

/// do not correspond to peer writes, and hence cannot be used as framing.

/// Slightly more efficient than `read` due to not copying. Chunk boundaries

/// do not correspond to peer writes, and hence cannot be used as framing.

stammw · 2021-01-06T19:14:47Z

Should be ok now.

Happy to contribute to quinn a bit :)

Issues solved

stammw force-pushed the read-ordered-chunks branch from 99d6a16 to c925a15 Compare December 29, 2020 20:06

djc reviewed Dec 29, 2020

View reviewed changes

Ralith requested changes Dec 29, 2020

View reviewed changes

stammw force-pushed the read-ordered-chunks branch 3 times, most recently from 0f6bece to c1594b4 Compare December 30, 2020 20:32

Ralith previously requested changes Dec 30, 2020

View reviewed changes

stammw force-pushed the read-ordered-chunks branch from c1594b4 to 66e55a5 Compare December 30, 2020 22:35

Ralith mentioned this pull request Jan 1, 2021

Limit concurrent streams rather than accept queue size #961

Merged

stammw force-pushed the read-ordered-chunks branch from 66e55a5 to 34b892c Compare January 4, 2021 22:24

stammw force-pushed the read-ordered-chunks branch from 34b892c to cad6f1a Compare January 4, 2021 22:43

stammw force-pushed the read-ordered-chunks branch from cad6f1a to 4840713 Compare January 4, 2021 22:54

djc mentioned this pull request Jan 5, 2021

Fix missing flow control checks in RESET_STREAM final offset handling #969

Merged

Matthias247 reviewed Jan 6, 2021

View reviewed changes

quinn-proto/src/connection/streams.rs Show resolved Hide resolved

stammw force-pushed the read-ordered-chunks branch from 4840713 to 6fbfc6e Compare January 6, 2021 06:53

stammw changed the title ~~WIP: Read ordered chunks from RecvStream~~ Read ordered chunks from RecvStream Jan 6, 2021

stammw mentioned this pull request Jan 6, 2021

Remove the intermediate buffer in h3-quinn hyperium/h3#38

Closed

djc reviewed Jan 6, 2021

View reviewed changes

stammw added 2 commits January 6, 2021 20:12

Read ordered chunks from RecvStream

226fe0b

Factorize proto read methods

ff0b4e9

Read multiple chunks into a slice of Bytes

6eeda35

stammw force-pushed the read-ordered-chunks branch from 6fbfc6e to 6eeda35 Compare January 6, 2021 19:13

djc approved these changes Jan 6, 2021

View reviewed changes

djc merged commit 69c620a into main Jan 6, 2021

djc deleted the read-ordered-chunks branch January 6, 2021 20:51

Matthias247 mentioned this pull request Jan 22, 2021

Assembler panic #982

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read ordered chunks from RecvStream #952

Read ordered chunks from RecvStream #952

stammw commented Dec 29, 2020

djc Dec 29, 2020

stammw Dec 30, 2020

djc Dec 30, 2020

stammw Dec 30, 2020

djc Dec 29, 2020

stammw Dec 30, 2020

Ralith left a comment

Ralith Dec 29, 2020

stammw Dec 30, 2020

Ralith left a comment

Ralith Dec 30, 2020

Ralith Dec 30, 2020

Ralith Dec 30, 2020

Ralith Dec 30, 2020

djc commented Jan 2, 2021

Ralith commented Jan 2, 2021

Matthias247 commented Jan 4, 2021

stammw commented Jan 4, 2021

djc commented Jan 4, 2021

stammw commented Jan 4, 2021

djc commented Jan 4, 2021

Matthias247 Jan 6, 2021

stammw Jan 6, 2021 •

edited

Loading

Matthias247 commented Jan 6, 2021

stammw commented Jan 6, 2021

djc left a comment

djc Jan 6, 2021

djc Jan 6, 2021

stammw commented Jan 6, 2021

	/// [`read_chunk()`]: RecvStream::read_chunk
	/// Foundation of [`read_chunk()`]: RecvStream::read_chunk

	/// Read the next segment of data from next valid offset in the stream.
	/// Read the next segment of data

		/// window is filled. When in-order delivery is required, the siblings `read()` or `read_chunk()`
		/// method should be used.

		/// Slightly more efficient than `read` due to not copying. Chunks boundaries
		/// do not correspond to peer writes, and hence cannot be used as framing.

Read ordered chunks from RecvStream #952

Read ordered chunks from RecvStream #952

Conversation

stammw commented Dec 29, 2020

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

Ralith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ralith 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

djc commented Jan 2, 2021

Ralith commented Jan 2, 2021

Matthias247 commented Jan 4, 2021

stammw commented Jan 4, 2021

djc commented Jan 4, 2021

stammw commented Jan 4, 2021

djc commented Jan 4, 2021

Choose a reason for hiding this comment

stammw Jan 6, 2021 • edited Loading

Choose a reason for hiding this comment

Matthias247 commented Jan 6, 2021

stammw commented Jan 6, 2021

djc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stammw commented Jan 6, 2021

stammw Jan 6, 2021 •

edited

Loading