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

Fix SplitSink performance problem #1969

Merged
merged 3 commits into from
Nov 13, 2019

Conversation

doyoubi
Copy link
Contributor

@doyoubi doyoubi commented Nov 9, 2019

Please refer to the related issue for details.

Maybe I should add tests for this "buffering guarantee"?

@Nemo157
Copy link
Member

Nemo157 commented Nov 11, 2019

It looks like both poll_flush and poll_close could be changed to call poll_flush_slot instead of having copies of the code each.

@doyoubi doyoubi changed the title Fix SplitSink performance problem [WIP] Fix SplitSink performance problem Nov 13, 2019
@doyoubi doyoubi changed the title [WIP] Fix SplitSink performance problem Fix SplitSink performance problem Nov 13, 2019
ready!(inner.as_pin_mut().poll_ready(cx))?;
inner.as_pin_mut().start_send(this.slot.take().unwrap())?;
}
ready!(Self::poll_flush_slot(inner.as_pin_mut(), &mut this.slot, cx))?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Move the this.lock.poll_lock(cx) out of poll_flush_slot to avoid the need to poll_lock again to get the inner.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yeah, I missed that it needs the lock across both operations.

@doyoubi
Copy link
Contributor Author

doyoubi commented Nov 13, 2019

Please take a look. Thanks

Copy link
Member

@Nemo157 Nemo157 left a comment

Choose a reason for hiding this comment

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

LGTM

ready!(inner.as_pin_mut().poll_ready(cx))?;
inner.as_pin_mut().start_send(this.slot.take().unwrap())?;
}
ready!(Self::poll_flush_slot(inner.as_pin_mut(), &mut this.slot, cx))?;
Copy link
Member

Choose a reason for hiding this comment

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

Ah, yeah, I missed that it needs the lock across both operations.

@doyoubi
Copy link
Contributor Author

doyoubi commented Nov 13, 2019

Oh, I just merge master to this branch to update it. Shall I revert it and use rebase instead?

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.

3 participants