Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stream: use VecDeque to store SendBuf data #695

Merged
merged 2 commits into from
Nov 3, 2020
Merged

Conversation

ghedo
Copy link
Member

@ghedo ghedo commented Nov 3, 2020

This changes the internal storage of SendBuf to a VecDeque, but it
doesn't change the underlying logic. In the future this will allow us to
change how SendBuf data is emitted to avoid the allocation and
additional copy we currently perform.

In most cases insertions to the SendBuf happen at the end of the buffer,
however, due to the fact that buffers need to be stored in order, in
case of retransmissions they might happen at any index in the vector.

While at it, the SendBuf::push_slice() method is also renamed to
SendBuf::write() to be consistent with RecvBuf.

ghedo added 2 commits November 3, 2020 13:46
These seem to be better names.
This changes the internal storage of SendBuf to a VecDeque, but it
doesn't change the underlying logic. In the future this will allow us to
change how SendBuf data is emitted to avoid the allocation and
additional copy we currently perform.

In most cases insertions to the SendBuf happen at the end of the buffer,
however, due to the fact that buffers need to be stored in order, in
case of retransmissions they might happen at any index in the vector.

While at it, the SendBuf::push_slice() method is also renamed to
SendBuf::write() to be consistent with RecvBuf.
@ghedo ghedo requested a review from a team as a code owner November 3, 2020 13:51
Comment on lines +1000 to +1005
for i in 0..self.data.len() {
if buf.off < self.data[i].off {
insert_at = Some(i);
break;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a chance to improve this further once binary_search lands? rust-lang/rust#78021

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation looks pretty simple, so we can probably do it without waiting for that PR. Though note that after the next set of changes (once I get them to work) we won't need the slow path anymore as we will always appending to the end, so might end up being wasted work.

@ghedo ghedo merged commit b335378 into master Nov 3, 2020
@ghedo ghedo deleted the stream-sendbuf-vecdeque branch November 3, 2020 17:11
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.

2 participants