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

Remove sync::Once::call_once 'static bound #52239

Merged
merged 1 commit into from Jul 11, 2018

Conversation

Projects
None yet
9 participants
@CAD97
Contributor

CAD97 commented Jul 11, 2018

See https://internals.rust-lang.org/t/sync-once-per-instance/7918 for more context.

Suggested r is @alexcrichton, the one who added the 'static bound back in 2014. I don't want to officially r? though, if the system would even let me. I'd rather let the system choose the appropriate member since it knows more than I do.

git blame history for sync::Once::call_once's signature:

  • std: Second pass stabilization of sync (Dec 2014)

    -    pub fn doit<F>(&'static self, f: F) where F: FnOnce() {
    +    #[stable]
    +    pub fn call_once<F>(&'static self, f: F) where F: FnOnce() {
  • libstd: use unboxed closures (Dec 2014)

    -    pub fn doit(&'static self, f: ||) {
    +    pub fn doit<F>(&'static self, f: F) where F: FnOnce() {
  • std: Rewrite the sync module (Nov 2014)

    -    pub fn doit(&self, f: ||) {
    +    pub fn doit(&'static self, f: ||) {
     The second layer is the layer provided by `std::sync` which is intended to be
     the thinnest possible layer on top of `sys_common` which is entirely safe to
     use. There are a few concerns which need to be addressed when making these
     system primitives safe:
    
       * Once used, the OS primitives can never be **moved**. This means that they
         essentially need to have a stable address. The static primitives use
         `&'static self` to enforce this, and the non-static primitives all use a
         `Box` to provide this guarantee.
    

The author of this diff is @alexcrichton. sync::Once now contains only a pointer to (privately hidden) Waiters, which are all stack-allocated. The 'static bound to sync::Once is thus unnecessary to guarantee that any OS primitives are non-relocatable.

As the 'static bound is not required for sync::Once's operation, removing it is strictly more useful. As an example, it allows attaching a one-time operation to instances rather than only to global singletons.

remove sync::Once::call_once 'static
- [std: Rewrite the `sync` modulehttps://github.com/rust-lang/rust/commit/71d4e77db8ad4b6d821da7e5d5300134ac95974e) (Nov 2014)

    ```diff
    -    pub fn doit(&self, f: ||) {
    +    pub fn doit(&'static self, f: ||) {
    ```

    > ```text
    >  The second layer is the layer provided by `std::sync` which is intended to be
    >  the thinnest possible layer on top of `sys_common` which is entirely safe to
    >  use. There are a few concerns which need to be addressed when making these
    >  system primitives safe:
    >
    >    * Once used, the OS primitives can never be **moved**. This means that they
    >      essentially need to have a stable address. The static primitives use
    >      `&'static self` to enforce this, and the non-static primitives all use a
    >      `Box` to provide this guarantee.
    > ```

The author of this diff is @alexcrichton. `sync::Once` contains only a pointer to (privately hidden) `Waiter`s, which are all stack-allocated. The `'static` bound to `sync::Once` is thus unnecessary to guarantee that any OS primitives are non-relocatable.

See https://internals.rust-lang.org/t/sync-once-per-instance/7918 for more context.
@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Jul 11, 2018

Collaborator

r? @bluss

(rust_highfive has picked a reviewer for you, use r? to override)

Collaborator

rust-highfive commented Jul 11, 2018

r? @bluss

(rust_highfive has picked a reviewer for you, use r? to override)

@CAD97

This comment has been minimized.

Show comment
Hide comment
@CAD97

CAD97 Jul 11, 2018

Contributor

Note also that parking_lot:0.6's Once takes &self for its Once::call_once implementation: https://docs.rs/parking_lot/0.6/parking_lot/struct.Once.html#method.call_once

Differences from the standard library Once

  • Only requires 1 byte of space, instead of 1 word.
  • Not required to be 'static.
  • Relaxed memory barriers in the fast path, which can significantly improve
    performance on some architectures.
  • Efficient handling of micro-contention using adaptive spinning.
Contributor

CAD97 commented Jul 11, 2018

Note also that parking_lot:0.6's Once takes &self for its Once::call_once implementation: https://docs.rs/parking_lot/0.6/parking_lot/struct.Once.html#method.call_once

Differences from the standard library Once

  • Only requires 1 byte of space, instead of 1 word.
  • Not required to be 'static.
  • Relaxed memory barriers in the fast path, which can significantly improve
    performance on some architectures.
  • Efficient handling of micro-contention using adaptive spinning.
@CAD97

This comment has been minimized.

Show comment
Hide comment
@CAD97

CAD97 Jul 11, 2018

Contributor

The only reason I can see that this would be problematic is if it causes inference issues. I don't know how this might cause inference breakage, but I've long since given up trying to predict what changes to signatures can cause inference breakage at this point.

Contributor

CAD97 commented Jul 11, 2018

The only reason I can see that this would be problematic is if it causes inference issues. I don't know how this might cause inference breakage, but I've long since given up trying to predict what changes to signatures can cause inference breakage at this point.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 11, 2018

Member

Thanks for the PR @CAD97! I think this should be a safe change to apply backwards-compatibly and thanks for doing the digging here to see where the 'static came from! It was indeed needed long ago for safety but nowadays it no longer needs it, so I believe this should all be safe to merge. To be extra sure though let's...

@rfcbot fcp merge

Member

alexcrichton commented Jul 11, 2018

Thanks for the PR @CAD97! I think this should be a safe change to apply backwards-compatibly and thanks for doing the digging here to see where the 'static came from! It was indeed needed long ago for safety but nowadays it no longer needs it, so I believe this should all be safe to merge. To be extra sure though let's...

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Jul 11, 2018

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot commented Jul 11, 2018

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Jul 11, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot commented Jul 11, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 11, 2018

Member

@bors: r+

Alrighty!

Member

alexcrichton commented Jul 11, 2018

@bors: r+

Alrighty!

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 11, 2018

Contributor

📌 Commit 0f3f292 has been approved by alexcrichton

Contributor

bors commented Jul 11, 2018

📌 Commit 0f3f292 has been approved by alexcrichton

@Mark-Simulacrum

This comment has been minimized.

Show comment
Hide comment
@Mark-Simulacrum
Member

Mark-Simulacrum commented Jul 11, 2018

@bors rollup

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jul 11, 2018

Rollup merge of #52239 - CAD97:patch-1, r=alexcrichton
Remove sync::Once::call_once 'static bound

See https://internals.rust-lang.org/t/sync-once-per-instance/7918 for more context.

Suggested r is @alexcrichton, the one who added the `'static` bound back in 2014. I don't want to officially r? though, if the system would even let me. I'd rather let the system choose the appropriate member since it knows more than I do.

`git blame` history for `sync::Once::call_once`'s signature:

- [std: Second pass stabilization of sync](rust-lang@f3a7ec7) (Dec 2014)

    ```diff
    -    pub fn doit<F>(&'static self, f: F) where F: FnOnce() {
    +    #[stable]
    +    pub fn call_once<F>(&'static self, f: F) where F: FnOnce() {
    ```

- [libstd: use unboxed closures](rust-lang@cdbb3ca) (Dec 2014)

    ```diff
    -    pub fn doit(&'static self, f: ||) {
    +    pub fn doit<F>(&'static self, f: F) where F: FnOnce() {
    ```

- [std: Rewrite the `sync` module](rust-lang@71d4e77) (Nov 2014)

    ```diff
    -    pub fn doit(&self, f: ||) {
    +    pub fn doit(&'static self, f: ||) {
    ```

    > ```text
    >  The second layer is the layer provided by `std::sync` which is intended to be
    >  the thinnest possible layer on top of `sys_common` which is entirely safe to
    >  use. There are a few concerns which need to be addressed when making these
    >  system primitives safe:
    >
    >    * Once used, the OS primitives can never be **moved**. This means that they
    >      essentially need to have a stable address. The static primitives use
    >      `&'static self` to enforce this, and the non-static primitives all use a
    >      `Box` to provide this guarantee.
    > ```

The author of this diff is @alexcrichton. `sync::Once` now contains only a pointer to (privately hidden) `Waiter`s, which are all stack-allocated. The `'static` bound to `sync::Once` is thus unnecessary to guarantee that any OS primitives are non-relocatable.

As the `'static` bound is not required for `sync::Once`'s operation, removing it is strictly more useful. As an example, it allows attaching a one-time operation to instances rather than only to global singletons.

bors added a commit that referenced this pull request Jul 11, 2018

Auto merge of #52268 - Mark-Simulacrum:rollup, r=Mark-Simulacrum
Rollup of 14 pull requests

Successful merges:

 - #51614 (Correct suggestion for println)
 - #51952 ( hygiene: Decouple transparencies from expansion IDs)
 - #52193 (step_by: leave time of item skip unspecified)
 - #52207 (improve error message shown for unsafe operations)
 - #52223 (Deny bare trait objects in in src/liballoc)
 - #52224 (Deny bare trait objects in in src/libsyntax)
 - #52239 (Remove sync::Once::call_once 'static bound)
 - #52247 (Deny bare trait objects in in src/librustc)
 - #52248 (Deny bare trait objects in in src/librustc_allocator)
 - #52252 (Deny bare trait objects in in src/librustc_codegen_llvm)
 - #52253 (Deny bare trait objects in in src/librustc_data_structures)
 - #52254 (Deny bare trait objects in in src/librustc_metadata)
 - #52261 (Deny bare trait objects in in src/libpanic_unwind)
 - #52265 (Deny bare trait objects in in src/librustc_codegen_utils)

Failed merges:

r? @ghost

@bors bors merged commit 0f3f292 into rust-lang:master Jul 11, 2018

1 check passed

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

@CAD97 CAD97 deleted the CAD97:patch-1 branch Jul 11, 2018

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Sep 14, 2018

rust: Update to 1.29.0.
Version 1.29.0 (2018-09-13)
==========================

Compiler
--------
- [Bumped minimum LLVM version to 5.0.][51899]
- [Added `powerpc64le-unknown-linux-musl` target.][51619]
- [Added `aarch64-unknown-hermit` and `x86_64-unknown-hermit` targets.][52861]

Libraries
---------
- [`Once::call_once` now no longer requires `Once` to be `'static`.][52239]
- [`BuildHasherDefault` now implements `PartialEq` and `Eq`.][52402]
- [`Box<CStr>`, `Box<OsStr>`, and `Box<Path>` now implement `Clone`.][51912]
- [Implemented `PartialEq<&str>` for `OsString` and `PartialEq<OsString>`
  for `&str`.][51178]
- [`Cell<T>` now allows `T` to be unsized.][50494]
- [`SocketAddr` is now stable on Redox.][52656]

Stabilized APIs
---------------
- [`Arc::downcast`]
- [`Iterator::flatten`]
- [`Rc::downcast`]

Cargo
-----
- [Cargo can silently fix some bad lockfiles ][cargo/5831] You can use
  `--locked` to disable this behaviour.
- [`cargo-install` will now allow you to cross compile an install
  using `--target`][cargo/5614]
- [Added the `cargo-fix` subcommand to automatically move project code from
  2015 edition to 2018.][cargo/5723]

Misc
----
- [`rustdoc` now has the `--cap-lints` option which demotes all lints above
  the specified level to that level.][52354] For example `--cap-lints warn`
  will demote `deny` and `forbid` lints to `warn`.
- [`rustc` and `rustdoc` will now have the exit code of `1` if compilation
  fails, and `101` if there is a panic.][52197]
- [A preview of clippy has been made available through rustup.][51122]
  You can install the preview with `rustup component add clippy-preview`

Compatibility Notes
-------------------
- [`str::{slice_unchecked, slice_unchecked_mut}` are now deprecated.][51807]
  Use `str::get_unchecked(begin..end)` instead.
- [`std::env::home_dir` is now deprecated for its unintuitive behaviour.][51656]
  Consider using the `home_dir` function from
  https://crates.io/crates/dirs instead.
- [`rustc` will no longer silently ignore invalid data in target spec.][52330]

[52861]: rust-lang/rust#52861
[52656]: rust-lang/rust#52656
[52239]: rust-lang/rust#52239
[52330]: rust-lang/rust#52330
[52354]: rust-lang/rust#52354
[52402]: rust-lang/rust#52402
[52103]: rust-lang/rust#52103
[52197]: rust-lang/rust#52197
[51807]: rust-lang/rust#51807
[51899]: rust-lang/rust#51899
[51912]: rust-lang/rust#51912
[51511]: rust-lang/rust#51511
[51619]: rust-lang/rust#51619
[51656]: rust-lang/rust#51656
[51178]: rust-lang/rust#51178
[51122]: rust-lang/rust#51122
[50494]: rust-lang/rust#50494
[cargo/5614]: rust-lang/cargo#5614
[cargo/5723]: rust-lang/cargo#5723
[cargo/5831]: rust-lang/cargo#5831
[`Arc::downcast`]: https://doc.rust-lang.org/std/sync/struct.Arc.html#method.downcast
[`Iterator::flatten`]: https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.flatten
[`Rc::downcast`]: https://doc.rust-lang.org/std/rc/struct.Rc.html#method.downcast
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment