refactor: Introduce a TransmitBuf for poll_transmit

[flub]

Hi, I've been looking around in poll_receive recently and started wondering if it could be simplified. This is my first attempt at a step in this direction, it pulls together the state about datagrams and their boundaries in the write buffer. I think this helps understand the interactions between a bunch of state changes that were previously spread across many lines. See the commit message for a detailed description.

If the general direction is considered useful I think I'd like to continue this further. The next thing I'd probably look at is the buffer management for a packet. I expect a lot of places don't need to know much about the full transmit buffer they're writing into but rather need to know the (start, len, end) space they can write their frames. Probably reducing the need to do everything with offsets from the transmit buffer.

Anyway, my aim is the make things clearer and easier to manage. Let's first see if this is step in that direction and then see what the future steps turn out to be. I think any change like this should be able to stand on its own rather than rely on a future change, because the future is always uncertain and might never happen :)

[djc]

Initial feedback from skimming:

• I like the direction!
• In order to make it easy to review this, I'd want the change to be split into smaller commits. I'd suggest by isolating all the state (fields) into TransmitBuf without doing anything else in the first commit, then add the trait(s) in a separate commit and start moving over methods/adding abstractions.
• The BufMut + BufOffset bounds like repetitive, I guess BufOffset could have BufMut as a supertrait probably?
• (Nit: let's start moving towards using impl Trait instead of T: Trait in argument position.)

[flub]

I've attempted to split this up, hopefully the commits are more or less what you had in mind. Still makes the same change essentially.

[gretchenfrage]

Thank you for working on this! I think this should be considered to close #2121

[gretchenfrage]

Thanks for your patience--I have ambitions of trying to take a closer look at this next weekend.

[flub]

Yes, I think this is a good direction! Some thoughts so far...

Naming

First a minor thing: Perhaps this should be called TransmitBuilder rather than TransmitBuf? That way it matches PacketBuilder, and I feel that more accurately represents that that is tracking more complex state than just a byte sequence.

I don't have very strong feelings about the naming, this was really just a working name I thought would change but then just stuck as it did end up making sense. I think both work, but indeed TransmitBuilder would match PacketBuilder (which retains it's name in #2195). The current naming does match the fact it implements BufMut directly. I guess the variable name would also have to change if this was renamed, probably to transmit or so.

impl BufMut

With regard to this:

unsafe impl BufMut for TransmitBuf<'_>

I'm not sure that I like that. It adds an unsafe statement, as well as a handful of LOC, for the sake of being able to write buf instead of buf.buf in perhaps a dozen places. Plus, it's a slightly surprising thing to keep in mind.

What would you think of:

impl TransmitBuf<'_> {
    /// Returns a buffer into which the datagram can be written
    pub(super) fn datagram_mut(&mut self) -> bytes::buf::Limit<&mut Vec<u8>> {
        self.buf.limit(self.buf_capacity)
    }
}

This is very similar to what I did in #2195 which has a PacketBuilder::frame_space_mut method which I do rather like how it reads. And it gives protection against writing too many bytes into the buffer. Doing this would indeed alight well with renaming TransmitBuf to TransmitBuilder.

(I'd like to return return a generic &mut impl BufMut there, but that doesn't currently work without use-lifetime annotations which are too recent a rust feature. Also that doesn't let me implement BufLen.)

trait BufLen

You're introducing this new trait for the sake of being able to pass around &mut (impl BufMut + BufLen) instead of &mut TransmitBuf. I understand that your motivation for this is to abstract things, but in this circumstance I think that directly passing around &mut TransmitBuf or &mut Vec<u8> to various functions would be a net-win for it being simple to think about.

I did this trait for two reasons:

• A number of APIs expect this "I get large buffer with a max write offset (and sometimes need a start offset)" notion, which is what I'm trying to move away from. But to keep the scope of this PR smaller I can't change all of that at once, that would get out of hand. So this BufLen is a stopgap measure.
• I assumed that the functions which took traits rather than the &Vec<u8> wanted to keep doing this. There is some logic to this, because a number of these functions really don't have any business being able to change the state of the datagrams. They only need to write some stuff.

So I think if you receive an explicit &TransmitBuf (or whatever it is called) you need to have some business with datagrams. Only writing some frames is not that.

I think it might be possible to remove BufLen entirely and instead switch to BufMut::remaining_mut for these functions. But:

• I'm not really statisfied from the BufMut::remaining_mut docs that this can be reliably used for this. I think the implementations we use, and certainly Limit, do. But it still leaves me a bit worried.
• It would switch around a lot more logic. Using this BufLen keeps the changes to places where this stop-gap measure is needed much simpler and easier to review for correctness.

After #2195 there is only one BufLen remaining. I think it's very reasonable to also remove that and I'd be happy to look at doing that in another PR. Though I'd probably wait to figure out a nice way here until that has all been merged so I know what the final shape of things is and have more of a feel for what everyone agrees on for abstraction style.

Getter methods

I agree with djc here--I'm pretty hesitant about having these getter methods on TransmitBuf: segment_size, num_datagrams, max_datagrams, datagram_start_offset, and datagram_max_offset.

These variables aren't really getting abstracted away if the users of TransmitBuf still have to know which of them exist and care about their specific values. That leaves the only remaining motivation of using getters like this as making sure we don't accidentally mutate them wrongly from outside the module. But it's pretty easy to check where we're writing to these variables and I don't think making these variables read-only from outside this module really gets at the bottleneck of us being able to make the code correct.

This last sentence I am really not that sure about. Abstractions the compiler provides are great at ensuring correctness, having to manually check on each PR if something is being mutated that shouldn't be is very fallible. Having explicit contracts like this will make it clear if that is being changed.

And it has to be weighed against the cons of increased LOC, and us remembering which ones of these functions are simply reading a field versus which ones are doing something more transformative.

I was experimenting with an alternative way of trying to abstract some of these things, and I noticed that the num_datagrams value is used in similar ways in a few places repeatedly. So I tried to see if I could factor some of them away into more transformative methods on TransmitBuf until the num_datagrams value no longer needed to be accessed outside of TransmitBuf at all and I came up with this:

Yes, I agree there may be more scope here. And I was unsure whether to go further or not. For the sake of being more incremental with changes I stopped here for this PR though.

For example I think the bytes_to_send is probably a good one to add. But others I expect to be easier to remove in the future entirely. E.g. datagram_max_offset is very tempting to remove after #2195 where it is only 3 uses remain.

So, from what I understand maybe next steps for this PR could be:

• Rename TransmitBuf to TransmitBuilder
• Add TranmitBuiilder::datagram_mut function
• See which helpers might easily disappear with this, e.g. .len(), .is_empty() etc.

If you think that's helpful I'll give those changes a go.

[flub]

@gretchenfrage I've pushed some new commits. Mainly this finally introduces a buffer abstraction that I've long wanted but resisted creating: the BufSlice (I'm not attached to this name, it could be better). To get access to the datagram being written into you need TransmitBuilder::datagram_mut() (or TransmitBuilder::datagram(), but in return you get a nice access to the buffer.

This means the PacketBuilder now needs to keep track of sizes relative to the datagram buffer, a change I've also long wanted. In turn this means some more of the questionable methods on TransmitBuilder can be removed, which is probably a good gain.

The main reason I was trying to avoid these changes so far was to try and keep the changes smaller. The abstractions are a fair number of lines of code again, but have a straight forward interface.

[flub]

FWIW, TransmitBuilder::datagram_mut() is similar to the approach I already took in #2195 where I used PacketBuilder::frame_space_mut() to access the buffer. Give that model also works well there it gives me confidence this is a good approach.

It does leave a manual BufMut impl around, which I agree is unfortunate due to the unsafe. Though as before it mostly forwards calls to the the underlying buffer implementation. So I don't think this is so bad to have, and Quinn does not seem to have a history of avoiding unsafe.

[flub]

Also note that BufSlice can probably completely remove the need for the BufLen trait from this PR. I'll do that once the BufSlice/DatagramBuffer idea has been accepted and possibly reshaped to reduce churn.

[gretchenfrage]

I'm still looking at this, but for what it's worth:

(I'd like to return return a generic &mut impl BufMut there, but that doesn't currently work without use-lifetime annotations which are too recent a rust feature. Also that doesn't let me implement BufLen.)

I don't believe that's true. Maybe I misunderstood? But checking out f2ecc7640e35936d18480b51beb12ca59d432b02 and applying this diff seems to work just fine:

diff --git a/quinn-proto/src/connection/transmit_buf.rs b/quinn-proto/src/connection/transmit_buf.rs
index 31fe7e00..b429073d 100644
--- a/quinn-proto/src/connection/transmit_buf.rs
+++ b/quinn-proto/src/connection/transmit_buf.rs
@@ -141,7 +141,7 @@ impl<'a> TransmitBuf<'a> {
     }
 
     /// Returns a buffer into which the current datagram can be written
-    pub(super) fn datagram_mut(&mut self) -> bytes::buf::Limit<&mut Vec<u8>> {
+    pub(super) fn datagram_mut<'s>(&'s mut self) -> impl BufMut + super::BufLen + 's {
         self.buf.limit(self.buf_capacity)
     }

[gretchenfrage]

I'm not really statisfied from the BufMut::remaining_mut docs that this can be reliably used for this. I think the implementations we use, and certainly Limit, do. But it still leaves me a bit worried.

Can you expand on why not? From looking at it, it actually seems like it might be fine.

BufMut::remaining_mut:

Returns the number of bytes that can be written from the current position until the end of the buffer is reached.

This value is greater than or equal to the length of the slice returned by chunk_mut().

Implementer notes

Implementations of remaining_mut should ensure that the return value does not change unless a call is made to advance_mut or any other function that is documented to change the BufMut’s current position.

Note

remaining_mut may return value smaller than actual available space.

Notable source code bits exemplifying the expected usage of this:

• BufMut::put_slice panics if remaining_mut() < slice.len()
• <Vec<u8> as BufMut>::remaining_mut returns isize::MAX - vec.len()

The comment "This value is greater than or equal to the length of the slice returned by chunk_mut()" clarifies that it's not merely meant as the remaining length of the next chunk, and the put_slice implementation establishes that it's expected to indicate how many more bytes can be written. The Vec implementation seems to further reinforce this understanding. Given this, the comment of "remaining_mut may return value smaller than actual available space" seems to be a pretty low-information note that it can be lower than, say, the extent of the underlying heap allocation or equivalent.

If remaining_mut can be used instead of introducing a new BufLen trait, that sounds quite preferable.

[flub]

I'm still looking at this, but for what it's worth:

(I'd like to return return a generic &mut impl BufMut there, but that doesn't currently work without use-lifetime annotations which are too recent a rust feature. Also that doesn't let me implement BufLen.)

I don't believe that's true. Maybe I misunderstood? But checking out f2ecc76 and applying this diff seems to work just fine:

[...] fn datagram_mut<'s>(&'s mut self) -> impl BufMut + BufLen + 's

Oh, interesting. Thanks for pointing this out.

Though in 4b0dcbf70e74dfe8e52a7adf8003f60f505de834 I now change that return type to DatagramBuffer. Which, while I failed to remove that comment, I do think can be a rather nice type to return rather than the trait. Because it clearly describes to any user what it is.

[flub]

I'm not really statisfied from the BufMut::remaining_mut docs that this can be reliably used for this. I think the implementations we use, and certainly Limit, do. But it still leaves me a bit worried.

Can you expand on why not? From looking at it, it actually seems like it might be fine.

BufMut::remaining_mut:

Returns the number of bytes that can be written from the current position until the end of the buffer is reached. This value is greater than or equal to the length of the slice returned by chunk_mut().

Implementer notes

Implementations of remaining_mut should ensure that the return value does not change unless a call is made to advance_mut or any other function that is documented to change the BufMut’s current position.

Note

remaining_mut may return value smaller than actual available space.

Notable source code bits exemplifying the expected usage of this:

* [`BufMut::put_slice`](https://docs.rs/bytes/latest/src/bytes/buf/buf_mut.rs.html#246-264) panics if `remaining_mut() < slice.len()`

* [`<Vec<u8> as BufMut>::remaining_mut`](https://docs.rs/bytes/latest/src/bytes/buf/buf_mut.rs.html#1599-1667) returns `isize::MAX - vec.len()`

The comment "This value is greater than or equal to the length of the slice returned by chunk_mut()" clarifies that it's not merely meant as the remaining length of the next chunk, and the put_slice implementation establishes that it's expected to indicate how many more bytes can be written. The Vec implementation seems to further reinforce this understanding. Given this, the comment of "remaining_mut may return value smaller than actual available space" seems to be a pretty low-information note that it can be lower than, say, the extent of the underlying heap allocation or equivalent.

If remaining_mut can be used instead of introducing a new BufLen trait, that sounds quite preferable.

My worry is with the Implementer notes: it says remaining_mut is allowed to change whenever advance_mut is called. But it does not say that it must strictly decrease with the amount that was advanced with. So I imagine it would be legal to re-allocate inside advance_mut so that remaining_mut is now larger then before the call to advance_mut. I'm not sure if this was intentional or not though.

What do you think about the BufSlice/DatagramBuffer abstraction though? That is another way of avoiding this, and it's kind of an abstraction that fits nicely to write fixed-sized datagrams (and maybe packets). Though I was worried about the amount of code added for it, now that it's there I do rather like it.

[flub]

What do you think about the BufSlice/DatagramBuffer abstraction though? That is another way of avoiding this, and it's kind of an abstraction that fits nicely to write fixed-sized datagrams (and maybe packets). Though I was worried about the amount of code added for it, now that it's there I do rather like it.

To clarify this: while BufLen still exists in this PR, in #2195 Header::encode is the only place that still has BufLen in it. Since the BufSlice trait here removes the need for BufLen in Header::encode BufLen will be gone entirely once #2195 is updated to be based on this.

[gretchenfrage]

To clarify this: while BufLen still exists in this PR, in https://github.com/quinn-rs/quinn/pull/2195 Header::encode is the only place that still has BufLen in it. Since the BufSlice trait here removes the need for BufLen in Header::encode BufLen will be gone entirely once https://github.com/quinn-rs/quinn/pull/2195 is updated to be based on this.

One thing to keep in mind is that I think to the extent practical we want to avoid approving something based on the assumption that future work will be approved in any very specific form. This sort of plan is highly likely to be transformed through peer review. Not to mention that you could get busy / etc.

[gretchenfrage]

My worry is with the Implementer notes: it says remaining_mut is allowed to change whenever advance_mut is called. But it does not say that it must strictly decrease with the amount that was advanced with. So I imagine it would be legal to re-allocate inside advance_mut so that remaining_mut is now larger then before the call to advance_mut. I'm not sure if this was intentional or not though.

I feel pretty good at this point about just considering that to be less-than-perfect documentation of the bytes crate and assuming that advance_mut is supposed to reduce remaining_mut by exactly cnt. Especially since it's something that's only being used internally.

[gretchenfrage]

What do you think about the BufSlice/DatagramBuffer abstraction though? That is another way of avoiding this, and it's kind of an abstraction that fits nicely to write fixed-sized datagrams (and maybe packets). Though I was worried about the amount of code added for it, now that it's there I do rather like it.

I quite like the DatagramBuffer struct! It seems like a good new and simple abstraction that aligns well with what a lot of this code is expecting. "Growable vector of bytes, with a maximum length, that allocation-wise is actually a suffix of a Vec<u8> that may already have stuff in it." Moreover, if it becomes useful for more things than just datagrams, it could easily be renamed to something more generic.

I don't think it warrants all these traits though, and I think the traits are over-complicated. AFAICT this is the situation that's been created:

flowchart TD
    DatagramBuffer --> BufMut
    DatagramBuffer --> BufSlice
    DatagramBuffer --> BufLen
    V["Vec<u8>"] --> BufMut
    V["Vec<u8>"] --> BufSlice
    V["Vec<u8>"] --> BufLen
    BufSlice --> Deref
    BufSlice --> DerefMut

And, from checking out bce96531fa640ab7153667e6b84eaefa08081478, I can just remove the BufSlice: Deref + DerefMut requirement and it compiles fine:

diff --git a/quinn-proto/src/connection/transmit_builder.rs b/quinn-proto/src/connection/transmit_builder.rs
index dac79621..f79da0ef 100644
--- a/quinn-proto/src/connection/transmit_builder.rs
+++ b/quinn-proto/src/connection/transmit_builder.rs
@@ -203,7 +203,7 @@ impl<'a> TransmitBuilder<'a> {
 ///
 /// The [`Deref`] impl must return the already written bytes as a slice.  [`DerefMut`] must
 /// allow modification of these already written bytes.
-pub(crate) trait BufSlice: BufMut + Deref<Target = [u8]> + DerefMut {
+pub(crate) trait BufSlice: BufMut {
     /// Returns the number of already written bytes in the buffer
     fn len(&self) -> usize;

Also the DerefMut implementation overall seems overkill since it's only used in 2 places for minor things, it seems a fair bit simpler to just give it a fn written(&mut self) -> &mut [u8]:

Diff

diff --git a/quinn-proto/src/connection/packet_builder.rs b/quinn-proto/src/connection/packet_builder.rs
index 3052efb2..b1e3484d 100644
--- a/quinn-proto/src/connection/packet_builder.rs
+++ b/quinn-proto/src/connection/packet_builder.rs
@@ -122,7 +122,7 @@ impl PacketBuilder {
         };
         let partial_encode = header.encode(&mut transmits.datagram_mut());
         if conn.peer_params.grease_quic_bit && conn.rng.random() {
-            transmits.datagram_mut()[partial_encode.start] ^= FIXED_BIT;
+            transmits.datagram_mut().written()[partial_encode.start] ^= FIXED_BIT;
         }
 
         let (sample_size, tag_len) = if let Some(ref crypto) = space.crypto {
@@ -255,7 +255,7 @@ impl PacketBuilder {
             .put_bytes(0, packet_crypto.tag_len());
         let encode_start = self.partial_encode.start;
         let mut datagram_buf = transmits.datagram_mut();
-        let packet_buf = &mut datagram_buf[encode_start..];
+        let packet_buf = &mut datagram_buf.written()[encode_start..];
         self.partial_encode.finish(
             packet_buf,
             header_crypto,
diff --git a/quinn-proto/src/connection/transmit_builder.rs b/quinn-proto/src/connection/transmit_builder.rs
index dac79621..968cfaed 100644
--- a/quinn-proto/src/connection/transmit_builder.rs
+++ b/quinn-proto/src/connection/transmit_builder.rs
@@ -1,5 +1,3 @@
-use std::ops::{Deref, DerefMut};
-
 use bytes::BufMut;
 
 use super::BufLen;
@@ -203,7 +201,7 @@ impl<'a> TransmitBuilder<'a> {
 ///
 /// The [`Deref`] impl must return the already written bytes as a slice.  [`DerefMut`] must
 /// allow modification of these already written bytes.
-pub(crate) trait BufSlice: BufMut + Deref<Target = [u8]> + DerefMut {
+pub(crate) trait BufSlice: BufMut {
     /// Returns the number of already written bytes in the buffer
     fn len(&self) -> usize;
 
@@ -242,6 +240,10 @@ impl<'a> DatagramBuffer<'a> {
             max_offset,
         }
     }
+
+    pub(crate) fn written(&mut self) -> &mut [u8] {
+        &mut self.buf[self.start_offset..]
+    }
 }
 
 unsafe impl BufMut for DatagramBuffer<'_> {
@@ -258,20 +260,6 @@ unsafe impl BufMut for DatagramBuffer<'_> {
     }
 }
 
-impl Deref for DatagramBuffer<'_> {
-    type Target = [u8];
-
-    fn deref(&self) -> &Self::Target {
-        &self.buf[self.start_offset..]
-    }
-}
-
-impl DerefMut for DatagramBuffer<'_> {
-    fn deref_mut(&mut self) -> &mut Self::Target {
-        &mut self.buf[self.start_offset..]
-    }
-}
-
 impl BufSlice for DatagramBuffer<'_> {
     /// Returns the number of already written bytes in the buffer
     fn len(&self) -> usize {

From there I started trying to remove the need to have BufSlice::len / BufLen::len by just being able to take .written().len(), but the way the various traits are integrated together made it more laborious than I'm probably willing to do in the course of writing this comment.

This is really the only reason the BufSlice trait exists for now. It is of course possible to make this w: &mut DatagramBuffer, the cost is a bunch of callers on the receive path would have to be adapted to construct a DatagramBuffer where they currently pass a &mut Vec.

That doesn't seem that hard. But also, could you clarify why that would be necessary on the receive path?

Overall I quite like where the DatagramBuffer abstraction is going. But I think the system of traits surrounding it is over-complicated.

[flub]

9da0da3 continues the DatagramBuffer but removes the BufSlice trait. As you can see in the commit it affects a number of calls on the receive side that pass in a Vec<u8> for a possible response. These calls have been updated, but the DatagramBuffer type is kept internal in quinn-proto, so the public APIs still use Vec<u8> for this.

Regarding the Deref and DerefMut: I think these traits are very natural for this kind of buffer. Removing them now results in 48 failures, because a few more places use the buffer like that now. I now also use Deref<Target = [u8]> for the len implemenation which is needed. If you manually add a DatagramBuffer::len instead of relying on the deref there are only 20 places that use the (mutable) slice access. Still, I think that's enough to show this is a natural API for this so I'm leaning towards keeping this.

f7110c7 removes the BufLen trait again, since no one seems to like this. I did this by essentially moving one of the commits from #2195 here. It uses BufMut::remaining_mut, in a way that even avoids the ambiguity that was discussed above :) (that edge case discussed is avoided thanks to DatagramBuffer being used where that problem occurred). Anyway, I like this result and was only worried about that change making this PR larger than needed.

[flub]

Hi, I would love some feedback on the current state. If this is about right I think the next step would be to tidy up the commits, e.g. there's need to have the TransmitBuf -> TransmitBuilder rename in this and we can probably also avoid the existance of the not-much-loved BufLen in the history. Though to see the incremental changes based on feedback I thought it was better to only add commits for now.

Though if the design needs to evolve more first let me know.

[gretchenfrage]

This is high on my list on things to look at! Just been a bit busy.

[djc]

Feedback from skimming the current state:

• It looks mostly pretty good
• I think the trait hierarchy looks okay
• I don't like the trivial getters on TransmitBuilder, IMO we can just make the fields pub(super) instead
• The start_with_datagram_size() methods and related naming seems a bit unwieldy

I think it's definitely worth getting the commit history in shape here.

[flub]

* I don't like the trivial getters on `TransmitBuilder`, IMO we can just make the fields `pub(super)` instead

I really would like to keep the access to fields read-only, and rust only allows this by using getter methods. When unfamiliar with the code, having everything access everything and mutate from various places make it rather hard to understand what is going on at times. It also results in logic about fields of structs being encoded in surprising places because everywhere can access it. I understand that originally this was convenient when first writing Quinn, but also think that it is fine to be more restrictive with the abstractions now.

* The `start_with_datagram_size()` methods and related naming seems a bit unwieldy

Yes, I was exploring this a bit more to see what can be done. This is basically an artifact of how the code was structured before. Having this code abstracted in one place makes it more obvious that this is a bit of a soar point. I think it would totally make sense to further refactor this. E.g. I can easily imagine inverting the api calls from "start new datagram" to "finish datagram", it would probably remove the need to keep track of variables like num_datagrams as well. But I do think this comes up because inserting this abstraction makes is more easy to recognise this. I would prefer to not to add more to the current PR, I think it already is an improvement on what was there before.

[djc]

I really would like to keep the access to fields read-only, and rust only allows this by using getter methods. When unfamiliar with the code, having everything access everything and mutate from various places make it rather hard to understand what is going on at times. It also results in logic about fields of structs being encoded in surprising places because everywhere can access it.

I understand this, but I think we can rely on callers in the limited visibility area to respect this if you just document it. Getters are just not worth it in this case.

[flub]

Hi, just letting you know I'm not forgetting about this PR. Am still waiting for a round of feedback from @gretchenfrage before doing followup changes.

[Ralith]

it would be helpful for you to craft the PR commit history in a way that incorporates changes to commits into those commits, rather than by changing them with additional commits added to the end, because doing so meshes better with the git usage style of this project.

Just a quick drive-by note -- it's not (just) that this is the style, but concretely it makes reviewing larger PRs much easier, since each commit can be reviewed in isolation.

[gretchenfrage]

it would be helpful for you to craft the PR commit history in a way that incorporates changes to commits into those commits, rather than by changing them with additional commits added to the end, because doing so meshes better with the git usage style of this project.

Just a quick drive-by note -- it's not (just) that this is the style, but concretely it makes reviewing larger PRs much easier, since each commit can be reviewed in isolation.

Well, if a project utilizes a "never rebase" workflow, that can make it easier to see what has changed about the PR since last review

[Ralith]

Which is great for the brief period where you can exactly remember what happened last review :grin:

[flub]

(Still massaging commits to take feedback into account, these newly pushed commits are not yet ready for a round of review. Will re-request review once ready again.)