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

Stabilize `std::{rc,sync}::Weak::{weak_count, strong_count}` #65778

Open
wants to merge 2 commits into
base: master
from

Conversation

@bdonlan
Copy link

bdonlan commented Oct 24, 2019

Closes: #57977

Supporting comments:

Although these were added for testing, it is occasionally useful to have a way to probe optimistically for whether a weak pointer has become dangling, without actually taking the overhead of manipulating atomics. Are there any plans to stabilize this?

Originally posted by @bdonlan in #57977 (comment)

Having this stabilized would help. Currently, the only way to check if a weak pointer has become dangling is to call upgrade, which is by far expensive.

Originally posted by @glebpom in #57977 (comment)

Not sure if stabilizing these warrants a full RFC, so throwing this out here as a start for now.

Note: per CONTRIBUTING.md, I ran the tidy checks, but they seem to be failing on unchanged files (primarily in src/stdsimd).

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Oct 24, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @rkruppe (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rkruppe

This comment has been minimized.

Copy link
Member

rkruppe commented Oct 25, 2019

The diff looks good to me, but stabilizing requires a @rust-lang/libs FCP and that hasn't happened yet on the tracking issue. I don't think the bot will listen to me, so let's wait for a libs team member to show up.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Oct 28, 2019

@rfcbot fcp merge

Looks like this is stabilizing:

mod rc {
    impl Weak<T> {
        pub fn strong_count(&self) -> usize;
        pub fn weak_count(&self) -> Option<usize>;
    }
}

mod sync {
    impl Weak<T> {
        pub fn strong_count(&self) -> usize;
        pub fn weak_count(&self) -> Option<usize>;
    }
}

where weak_count returns None for Weak::new. There's also some notes for Arc about how the value read can be racy

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Oct 28, 2019

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Oct 29, 2019

I assume this may have already been discussed, but why would a Weak created via new not just return 1 from weak_count?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Oct 29, 2019

I believe it's "technically a lie" because you can clone a Weak::new but you can't ever know how many clones were created

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Oct 29, 2019

Ah right, that makes sense.

@KodrAus

This comment has been minimized.

Copy link
Contributor

KodrAus commented Oct 31, 2019

@rfcbot concern is this the API we want?

The motivation for this seems to be entirely focused around cheaply determining if Weak::upgrade could possibly return Some, and since sync::Weak::weak_count behaves oddly should we just expose some kind of fn is_dangling() -> bool method on Weak instead (something like @stepancheg suggested) that doesn't give the illusion of offering any more information?

If we do want this API as-is then I think the docs need a little more attention. sync::Weak::new().weak_count() suggests you'll get Some(0), but you actually get None, like rc::Weak::new().weak_count(). sync::Weak should probably also carry the same safety notes as sync::Arc.

@jonas-schievink

This comment has been minimized.

Copy link
Member

jonas-schievink commented Oct 31, 2019

I agree that having an additional is_dangling or has_strong method makes sense, but I think these methods are still useful by themselves (eg. to write tests where a specific number of references is expected).

@bdonlan

This comment has been minimized.

Copy link
Author

bdonlan commented Nov 1, 2019

Perhaps an is_upgradable() function would be useful? Since we don't actually care about the count, we might be able to get away with relaxed ordering when reading the strong count in this case as well.

@KodrAus

This comment has been minimized.

Copy link
Contributor

KodrAus commented Nov 4, 2019

@jonas-schievink Do you have any examples of such a datastructure that needs to assert on the count through a Weak that isn't just trying to determine whether or not it's upgradable? I did a quick scan on GitHub and didn't turn up much, but there's lots of Rust that isn't publicly available on GitHub!

If not, I think I'd err towards stabilizing the semantically tighter is_ugradable() API.

@jonas-schievink

This comment has been minimized.

Copy link
Member

jonas-schievink commented Nov 4, 2019

My original use case for adding these methods was for unit tests in https://github.com/jonas-schievink/rcgc, which would've benefitted from the exact counts

@KodrAus

This comment has been minimized.

Copy link
Contributor

KodrAus commented Nov 4, 2019

@rfcbot resolved is this the API we want?

Ok, thanks for the follow-up @jonas-schievink! Modulo docs I'll call this concern resolved and not block stabilization.

@stepancheg

This comment has been minimized.

Copy link
Contributor

stepancheg commented Nov 4, 2019

One of the problems with the current API is that it exposes certain internals which we might not keep forever.

For example this program:

    let a = Rc::new(1);
    let c = Rc::downgrade(&a);
    drop(a);
    let d = c.clone();
    println!("{:?}", d.weak_count());

currently prints Some(2), and not None.

However, value is dropped, weak will never become strong. In particular, in clone it might be more convenient to create empty Weak instead of incremeting a refcounter when strong_count == 0 (to potentially save memory).

However, when we intentionally specify that weak_count returns Option<usize> instead of usize, we make this possible change harder in the future.

If we really need to expose counters (instead of is_upgradable), how about returning usize in weak_count instead of Option<usize>?

@bdonlan

This comment has been minimized.

Copy link
Author

bdonlan commented Nov 4, 2019

An option would be to specify that weak_count eventually will return zero once the strong count drops to zero (implementation-wise, we would perform a relaxed read on the strong count, and force the weak count to zero if the strong count is zero). This should provide consistent behavior in unit tests that have sufficient memory barriers to have reproducible results in the first place.

@bdonlan

This comment has been minimized.

Copy link
Author

bdonlan commented Nov 6, 2019

@stepancheg any thoughts on the suggestion above - whether weak_count should return zero or the true weak count for the case where all strong references have been dropped?

@stepancheg

This comment has been minimized.

Copy link
Contributor

stepancheg commented Nov 8, 2019

@bdonlan Good question. IMHO returning zero from weak count when strong is zero would be consistent. For the same reason: allow future optimizations without breaking compatibility.

@bdonlan

This comment has been minimized.

Copy link
Author

bdonlan commented Nov 8, 2019

I'd be happy to update the semantics of weak_count to reflect that if it's desirable - not sure what the process would be for this though. Should I open a separate PR for that, or just add a commit here? Or do we need more consensus from the T-libs team members first?

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Nov 21, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@bdonlan bdonlan force-pushed the bdonlan:stable_weak_count branch from 5ce5265 to 0d0b283 Nov 21, 2019
@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Dec 1, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@jonas-schievink

This comment has been minimized.

Copy link
Member

jonas-schievink commented Dec 1, 2019

Note that #65778 (comment) was not registered as a concern, and the libs team hasn't weighted in on it yet, so it's unclear to me whether this should be stabilized with or without that change.

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Dec 2, 2019

#65778 (comment) is a good statement of what still needs to be decided.

@rfcbot concern return 0 or the true weak count when all strong references have been dropped?

I am on board with either approach. #65778 (comment) points out that adjusting the implementation of Rc in the future to stop tracking weak counts once all strong counts are gone allows us to save memory; that's compelling to me. It looks like the PR has been updated to that implementation -- some of @rust-lang/libs checked boxes before that change was made and will need to weigh in on that change.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Dec 2, 2019

Looking at at least the Rc code, there's unsafe code in is_unique (and presumably elsewhere) that depends on weak_count's return value -- if we're returning 0 in more cases, then we may need to update that code. We should do the same for Arc, but the code there looks more complicated, so I'm uncertain if it needs to change.

I don't think this is a blocker stabilization wise, just needs to be done before we land this PR.

@bdonlan

This comment has been minimized.

Copy link
Author

bdonlan commented Dec 2, 2019

The change in the PR affects the implementation of std::{sync,rc}::Weak::weak_count, but does not impact Rc::weak_count or Arc::weak_count. This is because if we're coming from an Rc or Arc the strong count is guaranteed to be at least one, otherwise we could not have called the function in question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.