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

Remove unnecessary release from Arc::try_unwrap #74025

Merged
merged 1 commit into from
Jul 6, 2020

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Jul 4, 2020

The thread that recovers the unique access to Arc inner value (e.g., drop
when ref-count strong reaches zero, successful try_unwrap), ensures that
other operations on Arc inner value happened before by synchronizing
with release operations performed when decrementing the reference counter.

When try_unwrap succeeds, the current thread recovers the unique access
to Arc inner value, so release is unnecessary.

r? @Amanieu

The thread that recovers the unique access to Arc inner value (e.g., drop
when ref-count strong reaches zero, successful try_unwrap), ensures that
other operations on Arc inner value happened before by synchronizing
with release operations performed when decrementing the reference counter.

When try_unwrap succeeds, the current thread recovers the unique access
to Arc inner value, so release is unnecessary.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 4, 2020
@Amanieu
Copy link
Member

Amanieu commented Jul 5, 2020

This is correct, Release is necessary because there are no other threads to synchronize with. The Acquire is still needed to prevent the drop of the value from being reordered before the compare_exchange.

@bors r+

@bors
Copy link
Contributor

bors commented Jul 5, 2020

📌 Commit 8900502 has been approved by Amanieu

@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 5, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 5, 2020
…arth

Rollup of 12 pull requests

Successful merges:

 - rust-lang#72688 (added .collect() into String from Box<str>)
 - rust-lang#73787 (Add unstable docs for rustc_attrs)
 - rust-lang#73834 (Some refactoring around intrinsic type checking)
 - rust-lang#73871 (Fix try_print_visible_def_path for Rust 2018)
 - rust-lang#73937 (Explain exhaustive matching on {usize,isize} maximum values)
 - rust-lang#73973 (Use `Span`s to identify unreachable subpatterns in or-patterns)
 - rust-lang#74000 (add `lazy_normalization_consts` feature gate)
 - rust-lang#74025 (Remove unnecessary release from Arc::try_unwrap)
 - rust-lang#74027 (Convert more `DefId`s to `LocalDefId`s)
 - rust-lang#74055 (Fix spacing in Iterator fold doc)
 - rust-lang#74057 (expected_found `&T` -> `T`)
 - rust-lang#74064 (variant_count: avoid incorrect dummy implementation)

Failed merges:

r? @ghost
@bors bors merged commit aef2ca6 into rust-lang:master Jul 6, 2020
@tmiasko tmiasko deleted the try-unwrap branch July 6, 2020 06:39
@RalfJung
Copy link
Member

RalfJung commented Jul 9, 2020

@Amanieu

This is correct, Release is necessary because there are no other threads to synchronize with.

I assume you mean unnecessary?

The Acquire is still needed to prevent the drop of the value from being reordered before the compare_exchange.

There is no acquire in try_unwrap?

@Amanieu
Copy link
Member

Amanieu commented Jul 9, 2020

I assume you mean unnecessary?

Yes, that was a typo.

There is no acquire in try_unwrap?

It's in the acquire! macro.

@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
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants