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

Add a const-constructors for sync primitives #2790

Merged
merged 11 commits into from Sep 12, 2020

Conversation

mental32
Copy link
Sponsor Contributor

@mental32 mental32 commented Aug 24, 2020

Motivation

#2756 describes that the use of const-constructors with sync primitives and that there are currently no const-constructors for them.

Closes #2756

Solution

I've added a const_new public function to:

  • loom::std::atomic_usize::AtomicUsize
  • loom::std::parking_lot::Mutex
  • sync::batch_semaphore::Semaphore
  • sync::mutex::Mutex
  • util::linked_list::LinkedList

All of the const_new constructors have the following cfg:

    #[cfg(all(
        feature = "parking_lot", 
        not(all(loom, test)),
    ))]

As specified by the concern in this comment

Disclaimer

I have not modified or added any new tests as I'm not sure what needs to be done on the testing side for these changes. also since the cfg specifies that the constructors should not be present when cfg(test)

tokio/src/sync/mutex.rs Outdated Show resolved Hide resolved
@mental32
Copy link
Sponsor Contributor Author

There are some CI tests that I don't believe will work, due to a using feature(const_fn) this feature is however needed since if I try to build it on stable without it I get the following error:

   Compiling tokio v0.3.0 (/home/mental/Desktop/open_projects/tokio/tokio)
error[E0723]: trait bounds other than `Sized` on const fn parameters are unstable
  --> tokio/src/util/linked_list.rs:69:6
   |
69 | impl<T: Link> LinkedList<T> {
   |      ^
   |
   = note: see issue #57563 <https://github.com/rust-lang/rust/issues/57563> for more information
   = help: add `#![feature(const_fn)]` to the crate attributes to enable

error: aborting due to previous error

For more information about this error, try `rustc --explain E0723`.
error: could not compile `tokio`.

To learn more, run the command again with --verbose.

I'm not sure how to specify the cfg this PR so that these changes are only available on nightly.

Co-authored-by: Mikail Bagishov <bagishov.mikail@yandex.ru>
@mental32
Copy link
Sponsor Contributor Author

I'm not sure how to specify the cfg this PR so that these changes are only available on nightly.

never mind, figured it out 😅

they require feature(const_fn) which is not available on stable.
@mental32 mental32 force-pushed the mental/add-mutex-const-constructors branch from a330a94 to 3d53e15 Compare August 24, 2020 14:45
tokio/src/sync/mutex.rs Outdated Show resolved Hide resolved
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 think we'd really prefer not to add a nightly feature flag to Tokio. The problem with using a feature flag for nightly-only code is that another dependency could enable the feature flag, resulting it being enabled in the downstream application's build regardless of whether or not the nightly compiler is actually being used. This would result in a broken build.

For stuff like this, I think we'd prefer to add a RUSTFLAGS cfg. Unlike a feature flag, these must be passed as an environment variable at build-time, and cannot be set in Cargo.toml, so only the top level build can enable them. However, I'd rather not require nightly at all, and I think if we're willing to make a somewhat more substantial change, it's possible to do this without needing nightly.

I'd like to hear what other maintainers think about this, but that's my perspective.

tokio/src/util/linked_list.rs Outdated Show resolved Hide resolved
tokio/src/loom/std/parking_lot.rs Outdated Show resolved Hide resolved
tokio/src/sync/mutex.rs Outdated Show resolved Hide resolved
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
@mental32
Copy link
Sponsor Contributor Author

I'm not married to using a nightly feature flag or nightly at all for that matter so currently I'm open to any kind change :)

As for moving forward with this, I'm not quite sure what the next steps are.. Currently I'm watching this thread waiting for other maintainers opinions before doing anything so I feel like this is just blocked still atm.

@Darksonn
Copy link
Contributor

Can you open a new PR that makes the change we described to LinkedList? Then we'll merge that and continue here.

@hawkw
Copy link
Member

hawkw commented Aug 26, 2020

@mental32 btw, feel free to ping me if you get stuck on the LinkedList change. Thanks!

@mental32
Copy link
Sponsor Contributor Author

Sure thing! It may take a little while however so bear with me :)

@Darksonn Darksonn added A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-sync Module: tokio/sync labels Aug 31, 2020
@mental32
Copy link
Sponsor Contributor Author

All ready 👍 only blocked on #2805 for the moment

@Darksonn
Copy link
Contributor

Darksonn commented Sep 2, 2020

You should be able to merge in master now with the new LinkedList. Please prefer merges to rebases.

@mental32 mental32 force-pushed the mental/add-mutex-const-constructors branch from 251d910 to 687c687 Compare September 3, 2020 08:05
@mental32 mental32 force-pushed the mental/add-mutex-const-constructors branch from 687c687 to 39b8a30 Compare September 3, 2020 08:28
@mental32
Copy link
Sponsor Contributor Author

mental32 commented Sep 3, 2020

I think everything's good to go here 😄

tokio/src/loom/std/atomic_usize.rs Outdated Show resolved Hide resolved
tokio/src/sync/batch_semaphore.rs Outdated Show resolved Hide resolved
@mental32 mental32 force-pushed the mental/add-mutex-const-constructors branch from a800dc2 to 92dc9c3 Compare September 9, 2020 09:35
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

I will merge it once CI passes.

@Darksonn Darksonn merged commit 20ef286 into tokio-rs:master Sep 12, 2020
Darksonn pushed a commit that referenced this pull request Sep 12, 2020
* Add const constructors to `RwLock`, `Notify`, and `Semaphore`.

Referring to the types in `tokio::sync`.
Also add `const` to `new` for the remaining atomic integers in `src/loom` and `UnsafeCell`.

Builds upon previous work in #2790
Closes #2756
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-sync Module: tokio/sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Const constructors for Semaphore, Mutex, etc. with parking_lot.
5 participants