Arc: remove unused allocation from Weak::new() #50357
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
In order to not lose the |
|
@jonas-schievink I suppose it could be, would such trickery be worth it though? The only thing you do with |
This comment has been minimized.
This comment has been minimized.
|
I think what matters most is how |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
It seems potentially common to have |
|
So, I could change it back to holding a |
|
I think |
kennytm
added
T-libs-api
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
kennytm
reviewed
May 2, 2018
| weak: atomic::AtomicUsize::new(1), | ||
| data: uninitialized(), | ||
| }), | ||
| ptr: NonNull::new_unchecked(WEAK_EMPTY as *mut _), |
There was a problem hiding this comment.
😕 Just use NonNull::dangling() here.
There was a problem hiding this comment.
That picks a value based on the alignment size of T. While RawVec can check cap if its pointer is actually null, Weak doesn't have that. So, to check, dangling would need to be called each time.
It's also not clear to me what having a never-used pointer to 8, or 16, etc would provide.
There was a problem hiding this comment.
I don't think calling dangling every time is a problem.
There was a problem hiding this comment.
It shouldn't be a problem, the value is constant for the input type.
It may avoid memory access errors in debuggers, or avoid tripping unaligned access warnings in tools like Valgrind (if that's a thing).
There was a problem hiding this comment.
In #41064 we changed all dangling pointers to be aligned rather than 1. There is no reason for this case to be different.
dangling() optimizes to a constant, there is not issue with calling it many times.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Ping from triage, @seanmonstar and @kennytm ! What's the status here? |
|
@shepmaster looks like the CI failure is spurious. It's tagged waiting on review, is that correct, or is that blocked on me? |
|
@shepmaster @seanmonstar This is blocked by the lack of a reviewer 😐 |
haha, I thought you were the reviewer! random says... r? @KodrAus |
|
I can resolve the conflicts. I no longer have the benchmark code setup, is it needed to see |
|
Thanks for your patience with this @seanmonstar. I figured since we had the little bit of extra logic in #![feature(test)]
extern crate test;
use std::sync::{Arc, Weak};
#[bench]
fn weak_new(b: &mut test::Bencher) {
b.iter(|| test::black_box(Weak::<()>::new()));
}
#[bench]
fn weak_upgrade_none(b: &mut test::Bencher) {
let w = Weak::<i32>::new();
b.iter(|| test::black_box(w.upgrade()));
}
#[bench]
fn weak_upgrade_some(b: &mut test::Bencher) {
let a = Arc::new(42);
let w = Arc::downgrade(&a);
b.iter(|| test::black_box(w.upgrade()));
}On my machine I get: The difference looks negligible to me. I'm happy to r+ again after we resolve the conflicts. |
seanmonstar
force-pushed
the
arc-weak-null
branch
from
df72c4c to
24ce259
Compare
|
It looks like we're get the benefits of not allocating @bors r+ |
|
📌 Commit 24ce259 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Jun 29, 2018
Arc: remove unused allocation from Weak::new() It seems non-obvious that calling `Weak::new()` actually allocates space for the entire size of `T`, even though you can **never** access that data from such a constructed weak pointer. Besides that, if someone were to create many `Weak:new()`s, they could be unknowingly wasting a bunch of memory. This change makes it so that `Weak::new()` allocates no memory at all. Instead, it is created with a null pointer. The only things done with a `Weak` are trying to upgrade, cloning, and dropping, meaning there are very few places that the code actually needs to check if the pointer is null.
|
☀️ Test successful - status-appveyor, status-travis |
This was referenced Jun 29, 2018
|
Why do this for |
|
Same for Rc: #51901 |
SimonSapin
added a commit
that referenced
this pull request
Jun 29, 2018
SimonSapin
added a commit
that referenced
this pull request
Jul 4, 2018
SimonSapin
added a commit
that referenced
this pull request
Jul 6, 2018
Mark-Simulacrum
added a commit
to Mark-Simulacrum/rust
that referenced
this pull request
Jul 7, 2018
…hton Rc: remove unused allocation and fix segfault in Weak::new() Same as rust-lang#50357 did for `Arc`. Fixes rust-lang#48493
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 16, 2021
Correct outdated documentation for rc::Weak This was overlooked in ~~rust-lang#50357~~ rust-lang#51901
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It seems non-obvious that calling
Weak::new()actually allocates space for the entire size ofT, even though you can never access that data from such a constructed weak pointer. Besides that, if someone were to create manyWeak:new()s, they could be unknowingly wasting a bunch of memory.This change makes it so that
Weak::new()allocates no memory at all. Instead, it is created with a null pointer. The only things done with aWeakare trying to upgrade, cloning, and dropping, meaning there are very few places that the code actually needs to check if the pointer is null.