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

Extended Rc uniqueness methods #14908

Closed
wants to merge 1 commit into from
Closed

Extended Rc uniqueness methods #14908

wants to merge 1 commit into from

Conversation

kvark
Copy link
Contributor

@kvark kvark commented Jun 15, 2014

maybe_unique() allows to mutate the referenced data if the pointer is guaranteed to be the only one.
is_last_standing() allows to check if this pointer is the last strong one.

One possible application of these methods is a shared resource factory. The resource doesn't know how it is built or destroyed, it may even be method-less. The factory knows how to create a resource and how to destroy it. Upon creation, the factory stores Rc<T> internally and returns a cloned reference. Later, at a designated (by the user) moment, the factory can sweep over known references and delete only those resources that are no longer needed by anyone else (here comes maybe_unique or is_last_standing).

/// count is 1 (this pointer is unique).
#[inline]
#[experimental]
pub fn maybe_unique<'a>(&'a self) -> Option<&'a mut T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't safe because aliasing references can be created before or after calling maybe_unique. It would need to take &'a mut self and it would also need to prevent doing this if any Weak<T> reference exists because it could be upgraded. There have been implementations of this before and I think the previously proposed approaches need to be considered too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, fixed now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please note that maybe_unique does check for weak() count to be 1.

@eddyb
Copy link
Member

eddyb commented Jun 15, 2014

I'd suggest rewriting make_unique to use maybe_unique (and remove the bit of unsafety that isn't required anymore).

@kvark
Copy link
Contributor Author

kvark commented Jun 15, 2014

Thanks @eddyb. It's fixed now. I was hesitating to do that because maybe_unique() has to be called twice. I also removed the experimental tags, otherwise was getting a warning (of course) of using experimentals.

// additional reference of either kind.
if self.strong() != 1 || self.weak() != 1 {
*self = Rc::new(self.deref().clone())
if self.maybe_unique().is_none() {
Copy link
Member

Choose a reason for hiding this comment

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

This is not what I meant. You should be able to match on the result of maybe_unique.
(though, in retrospective, I may have underestimated the complexity of this change)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I initially tried, but Rust doesn't allow reassigning *self in the the match arm of maybe_unique() because the latter freezes self.

Copy link
Member

Choose a reason for hiding this comment

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

You have to return early in the other branch and mutate outside of the match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted make_unique() back to the original implementation after a brief discussion with @eddyb. The unsafe{} can be replaced by 2 calls to maybe_unique(), but that may not be as efficient.

@aturon
Copy link
Member

aturon commented Jun 23, 2014

@kvark I think I understand the use-case for is_last_standing() you outlined: basically, as a factory, you want to know when it would be advantageous to drop an Rc (to free up resources).

Can you spell out how maybe_unique() plays into this -- why isn't it enough to drop the Rc (and therefore the underlying resource)? Or do you envision a different use-case for make_unique()?

@kvark
Copy link
Contributor Author

kvark commented Jun 23, 2014

@aturon Thanks for having a look at this PR. I don't have a maybe_unique() use-case in mind. It's just something @eddyb proposed to have with an idea that make_unique() could be safely implemented through it. However, it turned out that doing make_unique() the old (unsafe) way is much more efficient, so I left the implementation unchanged.

@lilyball
Copy link
Contributor

These should not be methods on Rc; that interferes with using Rc to wrap any type T that has methods of the same name. They should be free functions.

The current state of my PR #15399 actually provides the same functionality under different names, along with a 3rd function to unwrap a uniquely-owned Rc<T>. You can see this at lilyball/rust@5033b2445. I was thinking of removing this because I believe my pending rewrite of the PR doesn't need that functionality any more, but the existence of this PR here suggests that it is more generally useful.

If you have no objection, I'm inclined to re-submit just that one commit as a PR (that would obsolete this PR).

@kvark
Copy link
Contributor Author

kvark commented Jul 21, 2014

@kballard Thanks, I trust you to take care of this. Besides, I'm long past the point where I needed it.

@kvark kvark closed this Jul 21, 2014
@kvark kvark deleted the t-rc branch August 17, 2014 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants