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

impl Arc::unwrap_or_clone #91589

Merged
merged 2 commits into from
Feb 5, 2022
Merged

impl Arc::unwrap_or_clone #91589

merged 2 commits into from
Feb 5, 2022

Conversation

richard-uk1
Copy link
Contributor

@richard-uk1 richard-uk1 commented Dec 6, 2021 •
edited
Loading

The function gets the inner value, cloning only if necessary. The conversation started on irlo. If the reviewer think the PR has potential to be merged, and does not need an RFC, then I will create the corresponding tracking issues and update the PR.

Alternative names

  • into_inner
  • make_owned
  • make_unique
  • take_* (take_inner?)

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 6, 2021
@apiraino apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Dec 9, 2021
@dtolnay dtolnay added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 10, 2021
@camelid
Copy link
Member

camelid commented Jan 25, 2022

r? rust-lang/libs

@rust-highfive rust-highfive assigned yaahc and unassigned kennytm Jan 25, 2022
@joshtriplett joshtriplett added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Feb 2, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Feb 2, 2022 •
edited
Loading

This seems reasonable to me, though I wonder if we should (also) have a more generic unwrap_or(|..| ..) function. (Not sure what the exact signature should be though.) The existing try_unwrap function comes close, but it doesn't reduce the reference count if ownership wasn't unique, leaving that to the point where the Err(Arc) gets dropped afterwards, resulting in more operations on the atomic counter than necessary. unwrap_or_clone or a more generic unwrap_or(..) could always decrement the counter unconditionally, since they don't return the Arc in any case. (Edit: Not true, see comment below. Only true for unwrap() -> Option<T>.)

For this pr:

  • Please also add the same function to Rc, to keep Arc and Rc in sync.
  • You can optimize the Arc implementation by using a single fetch_sub operation. Check the Drop implementation for Arc for what that looks like. (And check the try_unwrap implementation for how to take the T out of the Arc and cleaning it up.)
  • Please open a tracking issue for the issue number in the #[unstable] attributes.

@m-ou-se m-ou-se removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Feb 2, 2022
@m-ou-se m-ou-se assigned m-ou-se and unassigned yaahc Feb 2, 2022
@m-ou-se m-ou-se added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 2, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Feb 2, 2022

  • You can optimize the Arc implementation by using a single fetch_sub operation. Check the Drop implementation for Arc for what that looks like. (And check the try_unwrap implementation for how to take the T out of the Arc and cleaning it up.)

Actually, that wouldn't work. Sorry. That'd introduce a race where the object gets dropped while you're cloning it. This would only work for unwrap() -> Option<T>, but not for unwrap_or_clone(). So ignore that point. :)

@richard-uk1
impl Arc::unwrap_or_clone
0602fb0 
The function gets the inner value, cloning only if necessary.
@richard-uk1 richard-uk1 mentioned this pull request Feb 3, 2022
Closed
3 tasks
@richard-uk1
Copy link
Contributor Author

OK I've added a tracking issue, and duplicated the code into Rc.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@m-ou-se
Copy link
Member

m-ou-se commented Feb 4, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Feb 4, 2022

📌 Commit f5e6d16 has been approved by m-ou-se

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 4, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 4, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2022
@bors
Auto merge of rust-lang#93655 - matthiaskrgr:rollup-dm88b02, r=matthi…
4c55c83 
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#90132 (Stabilize `-Z instrument-coverage` as `-C instrument-coverage`)
 - rust-lang#91589 (impl `Arc::unwrap_or_clone`)
 - rust-lang#93495 (kmc-solid: Fix off-by-one error in `SystemTime::now`)
 - rust-lang#93576 (Emit more valid HTML from rustdoc)
 - rust-lang#93608 (Clean up `find_library_crate`)
 - rust-lang#93612 (doc: use U+2212 for minus sign in integer MIN/MAX text)
 - rust-lang#93615 (Fix `isize` optimization in `StableHasher` for big-endian architectures)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6f03bd0 into rust-lang:master Feb 5, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 5, 2022
@richard-uk1 richard-uk1 deleted the arc_unwrap_or_clone branch February 5, 2022 09:20
@lopopolo lopopolo mentioned this pull request Apr 2, 2022
Open
facebook-github-bot pushed a commit to facebook/relay that referenced this pull request May 3, 2024
@evanyeung @facebook-github-bot
Fix GitHub Cargo CI by replacing unstable Arc::unwrap_or_clone
cd9246e 
Summary:
Fixes the failure in T187825473 caused by D56339821.

This diff fixes the failing github action in https://github.com/facebook/relay/actions/runs/8932140521/job/24535481318 which was caused due to the addition of an unstable API in Arc (f7b030e). I've replaced the function with it's internal implemention from the PR to ARC at rust-lang/rust#91589.

Reviewed By: tyao1

Differential Revision: D56920263

fbshipit-source-id: e2d3f04921dd1e4b554ef075e258854b59764891
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@m-ou-se m-ou-se

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
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.