-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
RFC: remove weak pointers #1232
Conversation
# Alternatives | ||
|
||
Accept the tiny overhead and stabilize Weak. This involves answering the | ||
question of what uniqueness means in the context of Weak pointers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative not mentioned: Provide both weakless and weakable Rc and Arc in rust. For example using default type parameters, Arc<T> = Arc<T, NoWeak>
vs. Arc<T, AllowWeak>
. This alternative is also pay for what you use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest also designing this parameter to be able to configure atomicity and ref-count size, as well, if at all desired in the future, to avoid type parameter bloat.
👍 the weak support feels somewhat niche to me, though admittedly the cost is small |
Weak pointers are essential when reference cycles are expected, for example in a tree with parent node references. This is not all use cases, but it happens. Yes, Rc-with-Weak can be defined outside of the standard library, but so can Rc-without-Weak. What justifies inclusion in To clarify:
The uniqueness questions can be worked out (and are independent of inclusion or not in
|
👎 Do external crates already exist for this? These are used in Servo, and we were trying to move more things to libstd types that have them ( |
@metajack to clarify this RFC intends to move the support for weak pointers into an external crate on crates.io. Support for something like |
@alexcrichton I agree with @SimonSapin that if Rc is in libstd, then Weak probably should be. However, my issues with external crates is mostly around whether they will be created and maintained. If the answer is that the Rust team is committed to maintaining those, I agree this shouldn't impact Servo all that much. |
I must be missing something obvious, but I don't see why the weak_count is On Fri, Jul 31, 2015, 5:39 PM Alex Crichton notifications@github.com
|
@seanmonstar you need the weak count for a weak pointer to know if there are more weak pointers -- if it thinks it's the last it will free the memory causing all other Weaks to use-after-free. |
@metajack it could certainly be a rust-lang crate. |
@gankro couldn't the drop fn of let ptr = *self._ptr;
if !(*(&ptr as *const _ as *const *const ())).is_null() &&
ptr as *const () as usize != mem::POST_DROP_USIZE {
if self.strong() == 0 {
deallocate(ptr as *mut u8, size_of_val(&*ptr),
align_of_val(&*ptr))
}
} When the last |
@seanmonstar You can make arbitrarily many weak and strong pointers. Also the strong pointers need to know if there's any weak pointers when deciding whether to free. |
@gankro I know you can make many. Once the pointer is null, the other destructors should just check I may be wrongly assuming that a |
@seanmonstar How does anyone know to free the RcBox? Sorry to be clear, there's two distinct steps: |
In drop of Rc, if strong count becomes 0, free RcBox. On Fri, Jul 31, 2015, 10:12 PM Alexis Beingessner notifications@github.com
|
@seanmonstar This will make every Weak pointer use-after-free. |
conditions regardless. | ||
|
||
Note that we have *never* received complaints about this, to my knowledge. Servo | ||
and rustc happily use Rc without Weak. This isn't so much a "this is tragedy" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: Servo uses arc::Weak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Servo has both some data in Arc
s that may have weak references, and some other data in Arc
s that never do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the RFC it sounds like rustc and Servo don't use any weak pointers and are happy like that. I just don't want that we take decisions based on incorrect information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it’s a bit misleading. But there are also some cases where we have unused weak counters where the extra memory usage hasn’t bothered us yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rustc uses Weak
in at least one place, as a parent pointer in the rustc_resolve
custom tree of Module
s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So stop lieing in the RFC please!
(seanmonstar and I hashed it out on IRC -- they failed to heed my warning about the implementation details being a bit confusing and looked at them anyway -- Filling drop and NonZero are a hell of a drug) |
The choice of which So I don't think it's a good idea to get people to use |
@dgrunwald I think that's a reasonably convincing argument for why Weak should be opt-out, not opt-in. |
Proposal:
(Like |
What the Result/Option in return value cannot do that |
|
Funny coincidence, I just opened an issue on stabilizing I even tried to reimplement it outside Given all that the idea presented in this RFC does not feel right, especially as it is being considered for philosophical reasons. Altogether that sounds like premature optimization. Note that you want to remove functionality that people actually use. I disagree with the dismissal of the drawbacks in the RFC. Everybody using In my opinion we should at least obtain solid data, that there is a tangible benefit to removing it, to even consider this course of action. Of course, I admit that I have a biased perspective on this as a user of |
Perhaps following the argument of @dgrunwald it is inadvisable to use a ref-counted pointer without weak-ref support in APIs and such. As an alternative proposal, maybe a strong-only pointer type could be added to |
I really like @bluss' parametrization alternative, allowing the std to expose both only-strong references and pairs of strong/weak reference types, without violating DRY or zero-cost principles. |
Crates on crates.io that use
|
I agree that some evidence of the negative impact on space and perf would be very good to have. My feeling is that the RC we have is violating the pay-for-what-you-use principle. I'd like to remind people that when weak pointers were originally proposed they were a completely separate RC type, but we merged them because we didn't want a proliferation of RC types. I think this was a mistake we made in the spirit of compromise and we should not have merged weak pointers at all. |
On the topic of pay-for-what-you-use: C++ is generally regarded as the poster-child for this, and yet
Crates.io is much, much more flexible: (breaking) changes can be made independently of the language version, unlike things in |
|
Maybe we should only consider that factor on |
@huonw C++ also has no non-atomic ref-counted pointers, it's in a really bad spot, actually, when it comes to pay-for-what-you-use. EDIT: it appears that @steveklabnik made the same point while I was writing my comment. |
This is kinda my point ;) C++'s core version of this abstraction isn't so great, and yet it is still the only version offered. Sure, other people will write their own weak-less/non-atomic versions, but apparently not enough to get into the standard (maybe tracking down the C++ standards committee's/boost's discussion/proposals for |
I liked the suggestion by @bluss (and @eddyb's reply) so I scrambled together a quick half implementation that seems to work. https://play.rust-lang.org/?gist=0d53766a9a224fe27fe4&version=nightly Basically I don't really understand atomic operations though, and testing for race conditions is hard, so it's probably full of holes. Not sure of the overhead in calling closures either. All of the tests from I kind of like that it's just one type this way but it seems to cause issues with type inference (e.g. The |
@whataloadofwhat The issues with inference will be solved once #213 is ungated (if you enable the feature, you should be able to have |
I still don't understand how the trait version is supposed to be a better api than 4 types. It's the exact same combinatoric shuffling as having a function take an enum, but at the type level. It will just make everything more complicated and require describing everything 6 times instead of 4. |
@gankro It should simplify implementation by avoiding copy-pasting code. The API result should be exactly the same or better, otherwise it's not working correctly. The better part would be simpler ways to switch from one configuration to another. |
It's hardly shocking that people aren't using |
My opinion is starting to waver. There seems to be a balance between "pay for what you use" and "full featured". I think C++ goes too far with It's true that I've never personally had much use for weak pointers, but I know that Cocoa programs make use of them to represent pointers "up the tree" (and also for some specific situations, like a backpointer from delegate to UI node). We know Servo is using them. Hence, I can imagine that it comes up pretty regularly when programs grow to a certain size and rely extensively on Rc. And I guess it's one of those features that, when you need it, you really need it. I am not sure how I feel about using generics. It's a neat trick, and it seems better than having two totally distinct types, but in the end it's still two types (or 1.5, at least), and hence carries similar downsides (e.g., the point @dgrunwald made that you may not be in a good position to choose which flavor of |
I think parameters alleviates part of @dgrunwald's point, because we can write generic code on such parameters today, but not with totally separate types until HKT. |
I agree with @aepsil0n that this feels like premature optimization. I would consider this proposal compelling only if there were benchmarks of heavy real-world users of |
I'm going to propose the exact opposite of this RFC (but without is_unique). |
Sorry to comment on a closed thread, but for future reference, this really needs data - someone needs to sit down and find out the cost of the extra pointer in benchmarks before we can make an informed decision here. |
Well, we know the cost, right? It’s Whether that’s significant entirely depends on how big is |
@SimonSapin it incurs extra branches/logic in some places. For instance when strong count hits 0 you need to branch on the weak count to decide if you also free the memory. Probably negligible -- but not free. |
Like I said before, the size is the least of your worries: the main significant cost is that of atomic operations in the case of In the case of |
@eddyb it understands jemalloc enough to elide some useless allocations per rust-lang/rust#22159 But rust-lang/rust#24194 suggests it's maybe not great at that. |
Was there ever any update on this? It seems like it is impossible to build a "weak-less" Rc outside of std with the same performance because the standard Rc depends on unstable features. |
Weak is stable now. But I can't think of any unstable features that would prevent implementing a custom Rc. What are you referring to? |
unsized coerce and access to the memory allocator is missing (The latter could be possible to overcome with just using box, but I'm not 100% certain of the details). NonZero is missing. |
Seems like you can get most of a regular weakless Rc working on stable (code link). It's missing the unsized coerce and |
Given that the motivation for removing the weak count is to improve performance, I think it's counter productive to remove the nullable optimization. Giving up the rest is tolerable, but IMO, NonZero is essential. |
I think that is an argument for NonZero outside of std rust-lang/rust#27730 (which is desirable anyway), not for weakless Rc in std. |
Abandon support for Weak on Rc and Arc in favour of external crates that
provide the functionality. This resolves some open API questions, while making
them genuinely "pay for what you use".
Also stabilize the uniqueness stuff on them at the same time.
rendered