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

Provide safe wrappers for single-threaded memory fence intrinsics #41091

Closed
jonhoo opened this issue Apr 5, 2017 · 8 comments
Closed

Provide safe wrappers for single-threaded memory fence intrinsics #41091

jonhoo opened this issue Apr 5, 2017 · 8 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

@jonhoo
Copy link
Contributor

jonhoo commented Apr 5, 2017

RFC #888 introduced single-threaded (i.e., compiler-only) memory fence intrinsics, and was implemented in #24833.

The RFC explicitly does not add safe wrappers for these new barriers, and states

The existing fence intrinsics are exported in libstd with safe wrappers, but this design does not export safe wrappers for the new intrinsics. The existing fence functions will still perform correctly if used where a single-threaded fence is called for, but with a slight reduction in efficiency. Not exposing these new intrinsics through a safe wrapper reduces the possibility for confusion on which fences are appropriate in a given situation, while still providing the capability for users to opt in to a single-threaded fence when appropriate.

While the argument is sound, authors of low-level concurrency libraries are often very concerned about the performance overhead of full-blown memory fences when a simple compiler barrier will do. The current design requires authors of such libraries to either require a nightly compiler (to use intrinsics), or to fall back to the old

asm!("" ::: "memory" : "volatile")

workaround (which for the time being also requires nightly due to feature(asm)).

I propose adding in a safe wrapper around these compiler barrier intrinsics alongside fence after all, with name and documentation that clearly indicates how that function differs from what fence provides. PR incoming shortly.

bors added a commit that referenced this issue Apr 8, 2017
Add safe wrapper for atomic_compilerfence intrinsics

This PR adds a proposed safe wrapper for the `atomic_singlethreadfence_*` intrinsics introduced by [RFC #888](rust-lang/rfcs#888). See #41091 for further discussion.
@Mark-Simulacrum Mark-Simulacrum 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 Jun 20, 2017
@Mark-Simulacrum
Copy link
Member

This is the tracking issue for https://doc.rust-lang.org/nightly/std/sync/atomic/fn.compiler_fence.html.

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

@rfcbot fcp merge

Hasn't changed in quite awhile and seems straightforward to stabilize!

@rfcbot
Copy link

rfcbot commented Aug 29, 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 Aug 29, 2017
@sfackler
Copy link
Member

It seems mildly strange to me that these would live in sync::atomic since they don't deal with synchronization or atomicity in the traditional sense. I'm not sure where they should live though.

@dtolnay
Copy link
Member

dtolnay commented Aug 30, 2017

I am okay with the placement. As a data point, C++ has their equivalent std::atomic_signal_fence in #include <atomic> next to their equivalent of everything we have in std::sync::atomic.

@jonhoo
Copy link
Contributor Author

jonhoo commented Aug 30, 2017

@sfackler: I completely agree, and had the same reservation when I first implemented this. But as you say yourself, it's unclear where else this somewhat weird function should go.

@sfackler
Copy link
Member

Yeah, given that C++ sticks it in atomic std::sync::atomic seems like the least bad place.

@rfcbot
Copy link

rfcbot commented Sep 10, 2017

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

@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 Sep 10, 2017
TimNN added a commit to TimNN/rust that referenced this issue Sep 17, 2017
…=alexcrichton

stabilized compiler_fences (fixes rust-lang#41091)

I did not know what to proceed with "unstable-book" entry. The feature would no longer be unstable so I have deleted it. If it was the wrong call I'll revert it (unfortunately his case is not described in the CONTRIBUTING.md).
TimNN added a commit to TimNN/rust that referenced this issue Sep 17, 2017
…=alexcrichton

stabilized compiler_fences (fixes rust-lang#41091)

I did not know what to proceed with "unstable-book" entry. The feature would no longer be unstable so I have deleted it. If it was the wrong call I'll revert it (unfortunately his case is not described in the CONTRIBUTING.md).
TimNN added a commit to TimNN/rust that referenced this issue Sep 17, 2017
…=alexcrichton

stabilized compiler_fences (fixes rust-lang#41091)

I did not know what to proceed with "unstable-book" entry. The feature would no longer be unstable so I have deleted it. If it was the wrong call I'll revert it (unfortunately his case is not described in the CONTRIBUTING.md).
@bors bors closed this as completed in 8a11172 Sep 17, 2017
dtolnay pushed a commit to dtolnay/rust that referenced this issue Sep 25, 2017
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

6 participants