Skip to content

Randomize Stream::merge to improve the throughput. #503

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

Merged
merged 6 commits into from
Nov 19, 2019
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 29 additions & 9 deletions src/stream/stream/merge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use pin_project_lite::pin_project;

use crate::prelude::*;
use crate::stream::Fuse;
use crate::utils;

pin_project! {
/// A stream that merges two other streams into a single stream.
Expand All @@ -27,7 +28,10 @@ pin_project! {

impl<L: Stream, R: Stream> Merge<L, R> {
pub(crate) fn new(left: L, right: R) -> Self {
Self { left: left.fuse(), right: right.fuse() }
Self {
left: left.fuse(),
right: right.fuse(),
}
}
}

Expand All @@ -40,14 +44,30 @@ where

fn poll_next(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> {
let this = self.project();
match this.left.poll_next(cx) {
Poll::Ready(Some(item)) => Poll::Ready(Some(item)),
Poll::Ready(None) => this.right.poll_next(cx),
Poll::Pending => match this.right.poll_next(cx) {
Poll::Ready(Some(item)) => Poll::Ready(Some(item)),
Poll::Ready(None) => Poll::Pending,
Poll::Pending => Poll::Pending,
}
if utils::random(1) == 1 {
poll_next_in_order(this.left, this.right, cx)
} else {
poll_next_in_order(this.right, this.left, cx)
}
}
}

fn poll_next_in_order<F, S, I>(
first: Pin<&mut F>,
second: Pin<&mut S>,
cx: &mut Context<'_>,
) -> Poll<Option<I>>
where
F: Stream<Item = I>,
S: Stream<Item = I>,
{
match first.poll_next(cx) {
Poll::Ready(Some(item)) => Poll::Ready(Some(item)),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Poll::Ready(Some(item)) => Poll::Ready(Some(item)),
Poll::Ready(item) => Poll::Ready(item),

I think either is fine, but I thought it was a little redundant.
However, the above changes will not match None, so the order must be changed.
Which do you think is better?

Poll::Ready(None) => second.poll_next(cx),
Poll::Ready(item) => Poll::Ready(item),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm yes, I will review this, better if we remove redundancy. I just used the code that was already in place, but this change looks better.

Poll::Ready(None) => second.poll_next(cx),
Poll::Pending => match second.poll_next(cx) {
Poll::Ready(Some(item)) => Poll::Ready(Some(item)),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Poll::Ready(Some(item)) => Poll::Ready(Some(item)),
Poll::Ready(item) => Poll::Ready(item),

The same applies here

Poll::Ready(None) => Poll::Pending,
Poll::Pending => Poll::Pending,
},
}
}
11 changes: 7 additions & 4 deletions src/stream/stream/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1670,11 +1670,14 @@ extension_trait! {
let c = stream::once(3u8);

let mut s = a.merge(b).merge(c);
let mut lst = Vec::new();

assert_eq!(s.next().await, Some(1u8));
assert_eq!(s.next().await, Some(2u8));
assert_eq!(s.next().await, Some(3u8));
assert_eq!(s.next().await, None);
while let Some(n) = s.next().await {
lst.push(n)
}
Copy link
Member

Choose a reason for hiding this comment

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

I think I can write like this.

use async_std::stream::FromStream;
let v = Vec::from_stream(stream).await;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this makes it more clear :)


lst.sort_unstable();
assert_eq!(&lst, &[1u8, 2u8, 3u8]);
Comment on lines +1675 to +1676
Copy link
Member

Choose a reason for hiding this comment

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

👍

# });
```
"#]
Expand Down