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

Channel refactor #985

Merged
merged 1 commit into from Apr 30, 2018

Conversation

Projects
None yet
3 participants
@cramertj
Copy link
Collaborator

cramertj commented Apr 24, 2018

Refactors channels to use "waker" terminology rather than "parking"

Splits unbounded channels into a separate, simplified underlying type

Calls poll_ready in poll_flush impl for bounded channels in order to
provide backpressure on ::send.

Replacement for #984.

Fix #800
Fix rustasync/team#11

cc @danburkert, @seanmonstar, @carllerche

@cramertj cramertj requested a review from aturon Apr 24, 2018

@@ -401,7 +401,8 @@ fn stress_close_receiver_iter() {
Some(r) => assert!(i == r),
None => {
let unwritten = unwritten_rx.recv().expect("unwritten_rx");
assert_eq!(unwritten, i);
assert!(unwritten <= i + 1);

This comment has been minimized.

@cramertj

cramertj Apr 24, 2018

Author Collaborator

This is necessary in order to prevent this test from flaking due to the race condition when writing into a closed channel.

This comment has been minimized.

@carllerche

carllerche Apr 24, 2018

Contributor

I assume that this is the race that you mentioned in gitter and you are punting on a fix? If so, could you add a TODO referencing the issue?

This comment has been minimized.

@carllerche

carllerche Apr 24, 2018

Contributor

Skimming over it looks like the unbounded variant also has the same issue?

This comment has been minimized.

@cramertj

cramertj Apr 24, 2018

Author Collaborator

Yes, they both have this issue.

@carllerche

This comment has been minimized.

Copy link
Contributor

carllerche commented Apr 24, 2018

Could you explain the reasoning behind splitting up the unbounded & bounded implementations?

@carllerche

This comment has been minimized.

Copy link
Contributor

carllerche commented Apr 24, 2018

It's a pretty large diff and hard for me to reason about due to how terminology changes are mixed with code and behavior changes.

@cramertj

This comment has been minimized.

Copy link
Collaborator Author

cramertj commented Apr 24, 2018

@carllerche

Could you explain the reasoning behind splitting up the unbounded & bounded implementations?

Sure! I thought it was clearer and easier to read with them split up this way, and seemed like it might be more performant since there are fewer queues, atomics, and other checks needed to supply simple receiver wakeups than the comparatively complex sender wakeup semantics.

@cramertj cramertj force-pushed the cramertj:channel-backpressure branch from 8fcfc9c to 822061b Apr 24, 2018

@cramertj

This comment has been minimized.

Copy link
Collaborator Author

cramertj commented Apr 24, 2018

Before this PR:

test bounded_100_tx        ... bench:     290,574 ns/iter (+/- 2,578)
test bounded_1_tx          ... bench:     278,866 ns/iter (+/- 11,897)
test unbounded_100_tx      ... bench:     150,284 ns/iter (+/- 7,048)
test unbounded_1_tx        ... bench:     134,440 ns/iter (+/- 3,558)
test unbounded_uncontended ... bench:      66,844 ns/iter (+/- 5,163)

After this PR:

test bounded_100_tx        ... bench:     281,201 ns/iter (+/- 1,249)
test bounded_1_tx          ... bench:     278,706 ns/iter (+/- 36,851)
test unbounded_100_tx      ... bench:     105,909 ns/iter (+/- 22,730)
test unbounded_1_tx        ... bench:      98,740 ns/iter (+/- 2,073)
test unbounded_uncontended ... bench:      59,876 ns/iter (+/- 5,405)
@carllerche

This comment has been minimized.

Copy link
Contributor

carllerche commented Apr 24, 2018

The main argument against duplicating code is that it introduces more locations for bugs. For example, the close race bug is now duplicated in two code locations.

I would suggest that, if performance is the driver, at the very least benchmarks illustrating the performance difference should be present and probably at least a basic tuning pass of the bounded mpsc should be done (given that none have been done to date).

@carllerche

This comment has been minimized.

Copy link
Contributor

carllerche commented Apr 24, 2018

At the end of the day, you are the ones who have to maintain it, so feel free to do whatever.

@aturon

aturon approved these changes Apr 25, 2018

Copy link
Collaborator

aturon left a comment

LGTM modulo nit.

}
};
// Return the message
// Try to read a message off of the message queue.

This comment has been minimized.

@aturon

aturon Apr 25, 2018

Collaborator

Seems worth factoring this out into its own method, since the code is repeated verbatim below.

This comment has been minimized.

@cramertj

cramertj Apr 30, 2018

Author Collaborator

Done

@cramertj cramertj force-pushed the cramertj:channel-backpressure branch from 822061b to 4d2509f Apr 30, 2018

Channel refactor
Refactors channels to use "waker" terminology rather than "parking"

Splits unbounded channels into a separate, simplified underlying type

Calls poll_ready in poll_flush impl for bounded channels in order to
provide backpressure on <Sender as Sink>::send.

@cramertj cramertj force-pushed the cramertj:channel-backpressure branch from 4d2509f to c542a62 Apr 30, 2018

@cramertj cramertj merged commit 62827f1 into rust-lang-nursery:master Apr 30, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@cramertj cramertj deleted the cramertj:channel-backpressure branch Apr 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.