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.

Sign up for GitHub

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

Jump to bottom

Improve comments from #72617, as suggested by RalfJung #73925

Merged
merged 2 commits into from
Jul 4, 2020
Merged

Improve comments from #72617, as suggested by RalfJung #73925

merged 2 commits into from
Jul 4, 2020

Conversation

eduardosm
Copy link
Contributor

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 1, 2020
RalfJung
RalfJung reviewed Jul 2, 2020
View reviewed changes
src/libstd/panicking.rs Outdated
@@ -263,6 +263,11 @@ pub mod panic_count {
// Fast path: if `GLOBAL_PANIC_COUNT` is zero, all threads
// (including the current one) will have `LOCAL_PANIC_COUNT`
// equal to zero, so TLS access can be avoided.
//
// A relaxed atomic load is equivalent to a normal aligned memory read
// (e.g., a `mov` instruction in x86), while a TLS access might require
Copy link
Member

@RalfJung RalfJung Jul 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not equivalent. The compiler can optimize relaxed less than it can optimize normal reads.

Another way to see that they are not equivalent: replacing this relaxed access by a normal aligned read would result in Undefined Behavior. If two things are truly equivalent, then replacing one by the other changes nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about "In terms of performance, a relaxed atomic load is similar to a normal aligned memory read (e.g., a mov instruction in x86), but with some compiler optimization restrictions."?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds great. :)

@RalfJung
Copy link
Member

RalfJung commented Jul 3, 2020

Thanks. :)
@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 3, 2020

📌 Commit 0f1adc8 has been approved by RalfJung

@bors bors 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 Jul 3, 2020
@bors
Copy link
Contributor

bors commented Jul 3, 2020

⌛ Testing commit 0f1adc8 with merge c37547168f1b0b298cbb96aedbbf9fc1b46dc209...

@bors
Copy link
Contributor

bors commented Jul 3, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 3, 2020
@RalfJung
Copy link
Member

RalfJung commented Jul 3, 2020

@bors retry

@bors bors 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 Jul 3, 2020
@RalfJung RalfJung mentioned this pull request Jul 3, 2020
Closed
@Manishearth Manishearth mentioned this pull request Jul 4, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 4, 2020
@bors
Auto merge of rust-lang#74019 - Manishearth:rollup-2st3jsk, r=Manishe…
dbf3ae7 
…arth

Rollup of 12 pull requests

Successful merges:

 - rust-lang#73140 (Fallback to xml.etree.ElementTree)
 - rust-lang#73670 (Add `format_args_capture` feature)
 - rust-lang#73693 (Use exhaustive match in const_prop.rs)
 - rust-lang#73845 (Use &raw in A|Rc::as_ptr)
 - rust-lang#73861 (Create E0768)
 - rust-lang#73881 (Standardize bibliographic citations in rustc API docs)
 - rust-lang#73925 (Improve comments from rust-lang#72617, as suggested by RalfJung)
 - rust-lang#73949 ([mir-opt] Fix mis-optimization and other issues with the SimplifyArmIdentity pass)
 - rust-lang#73984 (Edit docs for rustc_data_structures::graph::scc)
 - rust-lang#73985 (Fix "getting started" link)
 - rust-lang#73997 (fix typo)
 - rust-lang#73999 (Bump mingw-check CI image from Ubuntu 16.04 to 18.04.)

Failed merges:

 - rust-lang#74000 (add `lazy_normalization_consts` feature gate)

r? @ghost
@bors bors merged commit 9d0ca38 into rust-lang:master Jul 4, 2020
@cuviper cuviper added this to the 1.46 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@RalfJung RalfJung

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.46
Development

Successfully merging this pull request may close these issues.
5 participants
@eduardosm @RalfJung @bors @cuviper @rust-highfive