Rework Rc and Arc APIs for FCP #27837

Gankra · 2015-08-14T19:07:15Z

This prepares both for the FCP of #27718

Arc:

Add previously omitted function Arc::try_unwrap(Self) -> Result<T, Self>
Move arc.downgrade() to Arc::downgrade(&Self) per conventions.
Deprecate Arc::weak_count and Arc::strong_count for raciness. It is almost
impossible to correctly act on these results without a CAS loop on the actual
fields.
Rename Arc::make_unique to Arc::make_mut to avoid uniqueness terminology
and to clarify relation to Arc::get_mut.

Rc:

Add Rc::would_unwrap(&Self) -> bool to introspect whether try_unwrap would succeed,
because it's destructive (unlike get_mut).
Move rc.downgrade() to Rc::downgrade(&Self) per conventions.
Deprecate Rc::weak_count and Rc::strong_count for questionable utility.
Deprecate Rc::is_unique for questionable semantics (there are two kinds of
uniqueness with Weak pointers in play).
Rename rc.make_unique() to Rc::make_mut(&mut Self) per conventions, to
avoid uniqueness terminology, and to clarify the relation to Rc::get_mut.

Notable omission:

Arc::would_unwrap is not added due to the fact that it's racy, and therefore doesn't
provide much actionable information.

(note: rc_would_unwrap is not proposed for FCP as it is truly experimental)

r? @aturon (careful attention needs to be taken for the new Arc::try_unwrap, but intuitively it should "just work" by virtue of being semantically equivalent to Drop).

eefriedman · 2015-08-14T20:13:38Z

src/liballoc/rc.rs

+    #[unstable(feature = "rc_unique", reason = "needs FCP", issue = "27718")]
+    pub fn make_mut(this: &mut Self) -> &mut T {
+        if !Rc::is_unique(this) {
+            *this = Rc::new((**this).clone())


This calls clone() in some cases where it isn't necessary.

Can you elaborate?

In the case where self is the only strong pointer, but there are other weak pointers, you can just ptr::read the value out of the old Rc.

If there's only one strong pointer (this one) but many weak pointers then you can move ownership from the original Rc to a new one, thereby destroying the original one (decrementing its strong count to 0).

I believe Arc takes this strategy currently.

SimonSapin · 2015-08-14T22:24:24Z

src/liballoc/rc.rs

-            *self = Rc::new((**self).clone())
+    #[unstable(feature = "rc_unique", reason = "needs FCP", issue = "27718")]
+    pub fn make_mut(this: &mut Self) -> &mut T {
+        if Rc::strong_count(this) != 0 {


Shouldn’t this be 1 rather than 0?

I should probably focus on this patch at all

Pleased to announce the tests caught this

Good tests :)

Err, no they didn't, try_unwrap just had an out of date test.

This is actually technically correct. Just weird to say. (The check is really > 1, where 0 is a nonsense value) -- it's needlessly ineffecient though.

bors · 2015-08-16T06:58:48Z

☔ The latest upstream changes (presumably #27818) made this pull request unmergeable. Please resolve the merge conflicts.

glaebhoerl · 2015-08-16T12:57:31Z

Just out of curiosity, why is it preferable to have rc::Rc::downgrade and arc::Arc::downgrade instead of rc::downgrade and arc::downgrade?

alexcrichton · 2015-08-16T16:05:11Z

@glaebhoerl when working with Rc you've probably already got that imported, so scoping on the type means you only ever need to import the type rather than both the type and the module. Beyond that though there's no real strong reason.

aturon · 2015-08-19T22:22:13Z

src/liballoc/arc.rs

+    #[unstable(feature = "arc_unique", reason = "needs FCP", issue = "27718")]
+    pub fn try_unwrap(this: Self) -> Result<T, Self> {
+        // See `drop` for why all these atomics are like this
+        if this.inner().strong.compare_and_swap(1, 0, Release) != 1 { return Err(this) }


I believe this can be replaced with an Acquire on the CAS and no fence, because this operation will only succeed if we're the last one out. (Similar to what we do for make_mut.)

Normally we fence after only if we see we're the last one, but doing the Acquire as part of the operation is slightly more efficient in some architectures.

Since this only succeeds if we're the last strong ref, the Release isn't needed; there's no one who would do a corresponding Acquire.

(That said, this is fine as-is; you could treat these as optimizations to be done later.)

aturon · 2015-08-19T22:23:41Z

Hey, sorry for the delay reviewing this.

Overall, this seems like a pretty good direction for the API (e.g., would_unwrap is nicer than working directly with the counts). A couple thoughts:

Functional deprecations (like removing the counts, as opposed to just renaming) should go through the FCP process now, so I'd split that out from this PR. (Looks like you already did that)
try_unwrap impl looks fine; I left a comment about improving the atomics but that can be done separately.
FWIW, I like having would_unwrap as a convenience, but think we may still want the counts in the long run. I also think that providing these methods on Arc may be good in the long run, to cover the cases where you have sufficient external sync to ensure that the values are useful.

Anyway, basically, r=me.

Gankra · 2015-08-19T22:51:18Z

Punting on fixing up atomics more, @aturon wants to do a more comprehensive sweep later anyway.

@bors r=aturon

bors · 2015-08-19T22:51:19Z

📌 Commit 9a0db5b has been approved by aturon

* Add `Rc::would_unwrap(&Self) -> bool` to introspect whether try_unwrap would succeed, because it's destructive (unlike get_mut). * Move `rc.downgrade()` to `Rc::downgrade(&Self)` per conventions. * Deprecate `Rc::weak_count` and `Rc::strong_count` for questionable utility. * Deprecate `Rc::is_unique` for questionable semantics (there are two kinds of uniqueness with Weak pointers in play). * Rename `rc.make_unique()` to `Rc::make_mut(&mut Self)` per conventions, to avoid uniqueness terminology, and to clarify the relation to `Rc::get_mut`.

* Add previously omitted function `Arc::try_unwrap(Self) -> Result<T, Self>` * Move `arc.downgrade()` to `Arc::downgrade(&Self)` per conventions. * Deprecate `Arc::weak_count` and `Arc::strong_count` for raciness. It is almost impossible to correctly act on these results without a CAS loop on the actual fields. * Rename `Arc::make_unique` to `Arc::make_mut` to avoid uniqueness terminology and to clarify relation to `Arc::get_mut`.

Gankra · 2015-08-19T23:07:28Z

@bors r=aturon

(rebasing error)

bors · 2015-08-19T23:07:29Z

📌 Commit 4c8d75f has been approved by aturon

SimonSapin · 2015-08-20T07:43:22Z

would_unwrap is nicer than working directly with the counts

That reminds me, do the docs mention the get_mut().is_some() pattern?

Gankra · 2015-08-20T15:37:33Z

@SimonSapin Nope, definitely a good follow up.

bors · 2015-08-21T05:04:33Z

⌛ Testing commit 4c8d75f with merge b1d07bb...

@aturon

This prepares both for the FCP of #27718 Arc: * Add previously omitted function `Arc::try_unwrap(Self) -> Result<T, Self>` * Move `arc.downgrade()` to `Arc::downgrade(&Self)` per conventions. * Deprecate `Arc::weak_count` and `Arc::strong_count` for raciness. It is almost impossible to correctly act on these results without a CAS loop on the actual fields. * Rename `Arc::make_unique` to `Arc::make_mut` to avoid uniqueness terminology and to clarify relation to `Arc::get_mut`. Rc: * Add `Rc::would_unwrap(&Self) -> bool` to introspect whether try_unwrap would succeed, because it's destructive (unlike get_mut). * Move `rc.downgrade()` to `Rc::downgrade(&Self)` per conventions. * Deprecate `Rc::weak_count` and `Rc::strong_count` for questionable utility. * Deprecate `Rc::is_unique` for questionable semantics (there are two kinds of uniqueness with Weak pointers in play). * Rename `rc.make_unique()` to `Rc::make_mut(&mut Self)` per conventions, to avoid uniqueness terminology, and to clarify the relation to `Rc::get_mut`. Notable omission: * Arc::would_unwrap is not added due to the fact that it's racy, and therefore doesn't provide much actionable information. (note: rc_would_unwrap is not proposed for FCP as it is truly experimental) r? @aturon (careful attention needs to be taken for the new Arc::try_unwrap, but intuitively it should "just work" by virtue of being semantically equivalent to Drop).

alexcrichton · 2015-08-21T17:41:30Z

Hm looks like our android slave never came back after I tried last time, but this passed all tests otherwise so merging manually

rust-lang/rust#27837

rust-highfive assigned aturon Aug 14, 2015

eefriedman reviewed Aug 14, 2015
View reviewed changes

Gankra force-pushed the weaknique branch 2 times, most recently from 9c3cf0c to 86bbc3d Compare August 14, 2015 22:10

SimonSapin reviewed Aug 14, 2015
View reviewed changes

Gankra force-pushed the weaknique branch 4 times, most recently from ebd3c6e to 777dd7c Compare August 15, 2015 00:49

alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 16, 2015

aturon reviewed Aug 19, 2015
View reviewed changes

Gankra force-pushed the weaknique branch from 0065a22 to 9a0db5b Compare August 19, 2015 22:46

Gankra added 4 commits August 19, 2015 15:52

fallout of reworking rc and arc APIs

5bbaa3c

don't do deprecations yet

4c8d75f

Gankra force-pushed the weaknique branch from 9a0db5b to 4c8d75f Compare August 19, 2015 22:52

alexcrichton merged commit 4c8d75f into rust-lang:master Aug 21, 2015

This was referenced Aug 22, 2015

std::rc::try_unwrap() should only consider the strong refcount #24789

Closed

Tracking issue for Rc/Arc stabilization #27718

Closed

mokus0 added a commit to mokus0/zinc that referenced this pull request Aug 24, 2015

Fixed platformtree after nightly change in Rc::downgrade

5b9204c

rust-lang/rust#27837

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework Rc and Arc APIs for FCP #27837

Rework Rc and Arc APIs for FCP #27837

Gankra commented Aug 14, 2015

eefriedman Aug 14, 2015

Gankra Aug 14, 2015

eefriedman Aug 14, 2015

alexcrichton Aug 14, 2015

SimonSapin Aug 14, 2015

Gankra Aug 14, 2015

Gankra Aug 14, 2015

Gankra Aug 14, 2015

SimonSapin Aug 14, 2015

Gankra Aug 14, 2015

bors commented Aug 16, 2015

glaebhoerl commented Aug 16, 2015

alexcrichton commented Aug 16, 2015

aturon Aug 19, 2015

aturon commented Aug 19, 2015

Gankra commented Aug 19, 2015

bors commented Aug 19, 2015

Gankra commented Aug 19, 2015

bors commented Aug 19, 2015

SimonSapin commented Aug 20, 2015

Gankra commented Aug 20, 2015

bors commented Aug 21, 2015

alexcrichton commented Aug 21, 2015

Rework Rc and Arc APIs for FCP #27837

Rework Rc and Arc APIs for FCP #27837

Conversation

Gankra commented Aug 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Aug 16, 2015

glaebhoerl commented Aug 16, 2015

alexcrichton commented Aug 16, 2015

Choose a reason for hiding this comment

aturon commented Aug 19, 2015

Gankra commented Aug 19, 2015

bors commented Aug 19, 2015

Gankra commented Aug 19, 2015

bors commented Aug 19, 2015

SimonSapin commented Aug 20, 2015

Gankra commented Aug 20, 2015

bors commented Aug 21, 2015

alexcrichton commented Aug 21, 2015