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 PR 40537 (hint_core_should_pause) #41196

Closed
mstewartgallus opened this issue Apr 10, 2017 · 24 comments
Closed

Tracking issue for PR 40537 (hint_core_should_pause) #41196

mstewartgallus opened this issue Apr 10, 2017 · 24 comments
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. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@mstewartgallus
Copy link
Contributor

mstewartgallus commented Apr 10, 2017

Tracking #40537 (comment)

@mstewartgallus mstewartgallus changed the title Tracking issue for PR https://github.com/rust-lang/rust/pull/40537#issuecomment-292972023 Tracking issue for PR 40537 (hint_core_should_pause) Apr 10, 2017
@alexcrichton alexcrichton added B-unstable Blocker: Implemented in the nightly compiler and unstable. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Apr 20, 2017
@clarfonthey
Copy link
Contributor

Reposting my comment from #41207: the spin crate calls this function cpu_relax; maybe we should do something similar?

I'd much prefer spin_loop_hint or something similar to the current function name. We could even potentially have a warning that suggests using this function, or even better, have the compiler automatically add these instructions inside busy loops.

@Mark-Simulacrum Mark-Simulacrum added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Jul 22, 2017
@ghost
Copy link

ghost commented Oct 5, 2017

I like the spin_loop_hint name, but would be okay with spin_wait as well.
Java calls this onSpinWait, in .NET it's SpinWait, and Linux kernel has cpu_relax.

Would it be a good time to propose stabilization after some name bikeshedding?
I've wanted this function in stable Rust for quite a while. :)

@alexcrichton
Copy link
Member

alexcrichton commented Oct 5, 2017

@rfcbot fcp merge

Sounds like a great issue to consider for stabilization!

Proposed name

spin_loop_hint

@rfcbot
Copy link

rfcbot commented Oct 5, 2017

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 these reviewers reach consensus, 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 the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Oct 5, 2017
@alexcrichton
Copy link
Member

To be clear I also don't have much of a preference on names, @stjepang's preference of spin_loop_hint also seems fine by me

@sfackler
Copy link
Member

sfackler commented Oct 5, 2017

spin_loop_hint or spin_wait both seem reasonable.

@dtolnay
Copy link
Member

dtolnay commented Oct 8, 2017

I am on board with stabilizing a function with this behavior. But in general, the more a function's behavior may be confusing, the more careful we need to be in selecting a name.

  • 👎 spin_wait because the spin loop code would read as though this call internally does a spin wait, rather than identifies the spin wait containing the call.
  • 👎 cpu_relax because the word "relax" is already claimed in the context of atomics for Ordering::Relaxed.
  • 👎 hint_core_should_pause because "pause" means do nothing for a while, where doing something else for a while may be more what you want.
  • 👍 spin_loop_hint which is what the Intel® 64 and IA-32 Architectures Software Developer’s Manual names the pause instruction.

I also sort of like the naming that WebKit uses -- pause is called YIELD_PROCESSOR and thread::yield_now is called YIELD_THREAD. This frames them as a pair of lower/higher level instructions. Yield processor is a hint to the processor, and yield thread is a hint to the OS's scheduler. Go uses the same naming -- pause is called runtime.procyield. So I'll add another name:

  • 👍 yield_processor.

@clarfonthey
Copy link
Contributor

👍 to spin_loop_hint

@dtolnay
Copy link
Member

dtolnay commented Oct 18, 2017

Looking at this again, I prefer yield_processor over spin_loop_hint. It seems like both would need to be documented equally carefully, but yield_processor results in code that is more pleasant to read. Unless yield_processor mischaracterizes how people conceptualize this instruction, I would go with that one. Also, precedent in Go and WebKit is encouraging.

It could also work as std::sync::atomic::yield. Is everything that runs Rust a "processor"?

@petrochenkov
Copy link
Contributor

petrochenkov commented Oct 18, 2017

Is everything that runs Rust a "processor"?

"Processor" sounds kinda wrong in this context IMO, since the word is so often used for the whole package with multiple cores, caches, etc, while the instruction applies to a "core" or "thread".

@SimonSapin
Copy link
Contributor

SimonSapin commented Oct 18, 2017

std::sync::yield or std::thread::yield sounds fine to me. (It’s not really about atomic instructions, more about thread scheduling.)

@kennytm
Copy link
Member

kennytm commented Oct 18, 2017

Isn't yield a keyword?

@SimonSapin
Copy link
Contributor

Oh, right.

@clarfonthey
Copy link
Contributor

I think that we should be cognisant of the case where someone imports both the processor and the thread yield functions at the same time. How would someone differentiate these functions?

Additionally, what this function does isn't quite a "yield;" on x86 specifically, it simply delays the execution of the next instruction by a small amount to avoid a memory order violation, reducing power consumption but not really reducing execution time. It's less the program "yielding" to the CPU and more the program telling the CPU "I'm just going to loop again, so, take your time."

Additionally, the thread version of a yield will definitely yield, whereas this will simply do nothing on architectures which have no such function. It makes a lot more sense to call this a hint than a yield.

@clarfonthey
Copy link
Contributor

Also this is a general question, but how reasonable would it be for the compiler itself to notice these cases and simply insert the instruction into busy loops? For example, something like loop {} should also contain this instruction. I wonder if LLVM has an optimisation like that.

@ghost
Copy link

ghost commented Oct 21, 2017

@clarcharr

Also this is a general question, but how reasonable would it be for the compiler itself to notice these cases and simply insert the instruction into busy loops?

I don't think the compiler can automatically decide when and where to insert the instruction into a loop. This is something only the programmer knows, really.

Inserting the instruction into a loop is a very delicate matter. Consider how parking_lot does it:

if self.counter <= 10 {
    cpu_relax(4 << self.counter);
} else {
    thread_yield();
}

Here, cpu_relax(t) will execute the instruction t times. Each successive step of the loop the instruction is executed twice as many times. There's a lot going on in that snippet - and is something that's very hard for the compiler to do correctly.

There's no recipe for how to properly use the instruction. You just have to carefully tweak the parameters, profile the program, and see what works best. There's no other way. :)

@Ixrec
Copy link
Contributor

Ixrec commented Oct 24, 2017

I'm not familiar with this function, but after spending a few minutes reading up on it, I believe spin_loop_hint is the only name suggested so far that would not be misleading to me. All of the others contain a word like yield, pause, wait, relax, etc that implies significantly more impact on execution semantics than a mere hint to the CPU.

@main--
Copy link
Contributor

main-- commented Oct 25, 2017

From the Intel Optimization Manual, section 8.4.7:

The PAUSE instruction is typically used with software threads executing on two logical processors located in the same processor core, waiting for a lock to be released. Such short wait loops tend to last between tens and a few hundreds of cycles, so performance-wise it is more beneficial to wait while occupying the CPU than yielding to the OS. When the wait loop is expected to last for thousands of cycles or more, it is preferable to yield to the operating system by calling one of the OS synchronization API functions, such as WaitForSingleObject on Windows* OS.

The PAUSE instruction is intended to:

  • Temporarily provide the sibling logical processor (ready to make forward progress exiting the spin loop) with competitively shared hardware resources. [...]
  • Save power consumed by the processor core compared to executing equivalent spin loop instruction sequence [...]

The latency of PAUSE instruction in prior generation microarchitecture is about 10 cycles, whereas on Skylake microarchitecture it has been extended to as many as 140 cycles.

The increased latency (allowing more effective utilization of competitively-shared microarchitectural resources to the logical processor ready to make forward progress) has a small positive performance impact of 1-2% on highly threaded applications. [...]

As the PAUSE latency has been increased significantly, workloads that are sensitive to PAUSE latency will suffer some performance loss.

The instruction's original function (hint to avoid memory order violation) has been extended quite a bit. So yes, it does yield resources.

@alexcrichton
Copy link
Member

I'm going to go ahead an check @BurntSushi's checkbox as it's been awhile and I think he's quite busy!

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Oct 31, 2017
@rfcbot
Copy link

rfcbot commented Oct 31, 2017

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

@rfcbot
Copy link

rfcbot commented Nov 10, 2017

The final comment period is now complete.

@bstrie
Copy link
Contributor

bstrie commented Nov 17, 2017

I marginally prefer spin_loop_hint over yield_processor, if only because it has zero chance of confusion with, er, whatever the yield keyword ends up doing.

@ghost ghost mentioned this issue Nov 21, 2017
kennytm added a commit to kennytm/rust that referenced this issue Nov 27, 2017
…sfackler

Stabilize spin_loop_hint

Stabilize `spin_loop_hint` in release `1.23.0`.
I've also renamed feature `hint_core_should_pause` to `spin_loop_hint`.

cc rust-lang#41196
@ghost
Copy link

ghost commented Jan 11, 2018

In #43751, there has been some talk of stabilizing module std::intrinsics or introducing std::hints. I wonder if such a module would be a better fit for spin_loop_hint than std::sync::atomic - what does everyone think?

(just a reminder: this feature is currently in beta, scheduled for stabilization in 1.24)

@ghost
Copy link

ghost commented Apr 12, 2018

This feature was stabilized. Let's close the issue.

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. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests