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

Introduce tokio-sync crate containing synchronization primitives. #839

Merged
merged 57 commits into from Jan 22, 2019

Conversation

carllerche
Copy link
Member

Introduce a tokio-sync crate containing useful synchronization primitives for programs written using Tokio.

The initial release contains:

  • An mpsc channel
  • A oneshot channel
  • A semaphore implementation
  • An AtomicTask primitive.

The oneshot and mpsc channels are new implementations providing improved performance characteristics. In some benchmarks, the new mpsc channel shows up to 7x improvement over the version provided by the futures crate. Unfortunately, the oneshot implementation only provides a slight performance improvement as it is mostly limited by the futures 0.1 task system. Once updated to the std version of Future (currently nightly only), much greater performance improvements should be achievable by oneshot.

Additionally, he implementations provided here are checked using Loom, which provides greater confidence of correctness.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

I've not finished reviewing the whole branch, but here are some minor nits and other non-blocking feedback on what I've reviewed to this point.

tokio-sync/src/mpsc/block.rs Show resolved Hide resolved
tokio-sync/src/mpsc/block.rs Show resolved Hide resolved
tokio-sync/src/mpsc/block.rs Outdated Show resolved Hide resolved
tokio-sync/src/mpsc/block.rs Outdated Show resolved Hide resolved
tokio-sync/src/mpsc/block.rs Show resolved Hide resolved
tokio-sync/src/mpsc/chan.rs Show resolved Hide resolved
tokio-sync/src/mpsc/chan.rs Show resolved Hide resolved
tokio-sync/src/mpsc/chan.rs Show resolved Hide resolved
tokio-sync/src/mpsc/chan.rs Show resolved Hide resolved
tokio-sync/src/mpsc/list.rs Outdated Show resolved Hide resolved
@arthurprs
Copy link
Contributor

Shouldn't this kind of thing be futures-sync instead?

@carllerche
Copy link
Member Author

@arthurprs given that it will be maintained by the Tokio team, it makes more sense to have it under the Tokio project IMO.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

A few questions:

  • What's the reason for exporting AtomicTask, i.e. when should one use it over futures::task::AtomicTask?

  • Any reason why the API for acquiring permits in semaphore is the way it is? Why not have Semaphore::acquire(&self) -> Acquire<'_>, where Acquire is a future that returns a Permit on success, and dropping Permit releases it?

  • Would it be better for Sender::poll_cancel() to be named Sender::poll_close()? One of my pet peeves with channels in general is that there are so many words that mean the same thing: close, disconnect, cancel (also see here). :)

.travis.yml Show resolved Hide resolved
tokio-sync/src/semaphore.rs Outdated Show resolved Hide resolved
tokio-sync/src/semaphore.rs Outdated Show resolved Hide resolved
tokio-sync/src/semaphore.rs Outdated Show resolved Hide resolved
tokio-sync/src/semaphore.rs Show resolved Hide resolved
tokio-sync/src/oneshot.rs Outdated Show resolved Hide resolved
tokio-sync/src/oneshot.rs Show resolved Hide resolved
@carllerche
Copy link
Member Author

@stjepang

What's the reason for exporting AtomicTask, i.e. when should one use it over futures::task::AtomicTask?

It makes me feel more comfortable maintaining this infrastructure piece as all of Tokio relies on it. It has been updated to be checked w/ loom. There have been buggy changes merged into the AtomicTask in the futures crate in the past.

Any reason why the API for acquiring permits in semaphore is the way it is? Why not have Semaphore::acquire(&self) -> Acquire<'_>, where Acquire is a future that returns a Permit on success, and dropping Permit releases it?

There could be value in providing an API like that, but it would incur additional run-time cost. The proposed API has little run-time cost and is intended to be used as part of other data structures (like mpsc and blocking in tokio-threadpool. If, at some point, we introduce a Semaphore with the API you propose, we can rename this impl to RawSemaphore or something like that. Thoughts?

Would it be better for Sender::poll_cancel() to be named Sender::poll_close()?

I think that would be a good change. I will apply to the PR.

@ghost
Copy link

ghost commented Jan 16, 2019

It makes me feel more comfortable maintaining this infrastructure piece as all of Tokio relies on it. It has been updated to be checked w/ loom. There have been buggy changes merged into the AtomicTask in the futures crate in the past.

Fair enough, that makes sense. It'd be good to eventually move our changes and loom-verified stuff back into futures, but it's no rush.

If, at some point, we introduce a Semaphore with the API you propose, we can rename this impl to RawSemaphore or something like that. Thoughts?

Fine by me. API design is hard. Let's keep the current semaphore and reevaluate it once we gain experience.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Very nice! I'm happy with the current state of the PR.

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

:shipit: 🔥

@carllerche carllerche merged commit 1308315 into master Jan 22, 2019
jonhoo added a commit to mit-pdos/noria that referenced this pull request Jan 23, 2019
@mauri870
Copy link

There's any plans to add mpmc channels to this library?

@steviezplayyard
Copy link

steviezplayyard commented Jan 24, 2019 via email

@carllerche carllerche deleted the tokio-sync branch January 25, 2019 05:02
@@ -0,0 +1 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for being that late, but what was the reason for this file and tx.rs, if both are empty and not used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah... just forgot to delete them I guess.

@carllerche
Copy link
Member Author

@mauri870 at some point probably. Just have to come up w/ a strategy that works.

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.

None yet

7 participants