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

Rename compiler_barrier to compiler_fence #41262

Merged
merged 1 commit into from
Apr 18, 2017

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Apr 12, 2017

This addresses concerns raised following the merge of #41092. Specifically:

The naming of these seems surprising: the multithreaded functions (and both the single and multithreaded intrinsics themselves) are fences, but this is a barrier. It's not incorrect, but the latter is both inconsistent with the existing functions and slightly confusing with another type in std (e.g., Barrier).

compiler_fence carries the same semantic implication that this is a compiler-only operation, while being more in line with the fence/barrier concepts already in use in std.

@rust-highfive
Copy link
Collaborator

r? @GuillaumeGomez

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

@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 12, 2017

r? @parched

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 13, 2017
@jonhoo jonhoo force-pushed the compiler-barrier-rename branch from f3edc1f to f9f3e16 Compare April 13, 2017 14:10
This addresses concerns raised following the merge of rust-lang#41092.
Specifically:

> The naming of these seems surprising: the multithreaded functions (and
> both the single and multithreaded intrinsics themselves) are fences,
> but this is a barrier. It's not incorrect, but the latter is both
> inconsistent with the existing functions and slightly confusing with
> another type in std (e.g., `Barrier`).

`compiler_fence` carries the same semantic implication that this is a
compiler-only operation, while being more in line with the fence/barrier
concepts already in use in `std`.
@jonhoo jonhoo force-pushed the compiler-barrier-rename branch from f9f3e16 to 368d560 Compare April 13, 2017 14:27
@alexcrichton
Copy link
Member

I personally don't mind going either way on this, @jonhoo do you know if there's precedent for naming here we can draw from other languages perhaps?

@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 13, 2017

I've been struggling to find other languages that expose this feature beyond C++ (which exposes it as atomic_signal_fence). D and Julie only have the full atomic fence (and called atomic_fence). Go only has a mark-and-sweep write barrier called gcmarkwb_m as far as I can tell. Java doesn't seem to have anything quite equivalent. The docs say that "each action in a thread happens-before every action in that thread that comes later in the program's order", but I don't know how that interacts with interrupt-like changes of execution, which is what atomic_signal_fence addresses.

I was and am hesitant to go with the C++ name for two reasons: it is not exclusively for signals (interrupts are also an important use-case), and atomic_signal_fence could too easily be misconstrued to also do cross-thread synchronization). compiler_barrier and compiler_fence to me both suggest that the operation is indeed entirely about what the compiler is allowed to do, which was the intention. I do agree with the objection raised in #41092 and quoted in the top post that _barrier could easily be connected (incorrectly) with sync::atomic::Barrier, and that fence is better since it associates more closely with the atomic_signal_fence equivalent in C++.

@bstrie
Copy link
Contributor

bstrie commented Apr 14, 2017

Thanks for researching precedence! I concur with the arguments for compiler_fence over both compiler_barrier and atomic_signal_fence. If you want to get even more literal I might suggest reorder_fence (eh, maybe), but as long as we avoid conflation I'm not too concerned.

@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 14, 2017

@bstrie I see what you're trying to do with reorder_fence, but I think it's more likely to cause confusion since so many things can be "reordered", especially when you also take into account the different Ordering arguments you can give. One alternative might be mem_reorder_fence, but I'm not sure it's really that much clearer than compiler_fence. It also still has the potential issue that users may not immediately recognize that we're only talking about compiler re-ordering.

@parched
Copy link
Contributor

parched commented Apr 14, 2017

I think compiler_fence is good, the other one I would consider is single_thread_fence which probably makes more sense if rust is to ever add single thread atomics too.

@shepmaster shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 14, 2017
@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 14, 2017

@parched I'm actually skeptical of single_thread_fence, because that to me still sounds like it might prevent CPU re-ordering (which, of course, it does not). I think we should merge this change to compiler_fence.

@alexcrichton
Copy link
Member

Thanks for the investigation @jonhoo and others for weighing in! I'm convinced of compiler_fence myself and this is unstable regardless, so let's land this and we can reevaluate during stabilization of course.

@bors: r+

@bors
Copy link
Contributor

bors commented Apr 18, 2017

📌 Commit 368d560 has been approved by alexcrichton

@alexcrichton alexcrichton added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 18, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 18, 2017
…lexcrichton

Rename compiler_barrier to compiler_fence

This addresses concerns raised following the merge of rust-lang#41092. Specifically:

> The naming of these seems surprising: the multithreaded functions (and both the single and multithreaded intrinsics themselves) are fences, but this is a barrier. It's not incorrect, but the latter is both inconsistent with the existing functions and slightly confusing with another type in std (e.g., `Barrier`).

`compiler_fence` carries the same semantic implication that this is a compiler-only operation, while being more in line with the fence/barrier concepts already in use in `std`.
bors added a commit that referenced this pull request Apr 18, 2017
Rollup of 3 pull requests

- Successful merges: #41262, #41310, #41344
- Failed merges:
@bors
Copy link
Contributor

bors commented Apr 18, 2017

⌛ Testing commit 368d560 with merge e621e1c...

@bors bors merged commit 368d560 into rust-lang:master Apr 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants