use more Rc in the part of resolver that gets cloned a lot 2 #5302
Conversation
src/cargo/core/resolver/context.rs
Outdated
| .or_insert_with(HashSet::new); | ||
| .or_insert_with(|| Rc::new(HashSet::new())); | ||
|
|
||
| let mut inner: HashSet<_> = (**set).clone(); |
There was a problem hiding this comment.
Could this perhaps use Rc::make_mut? That way we could have a fast path as well for when the Context hasn't been cloned?
There was a problem hiding this comment.
This is an algorithmic improvement, I think HashSet.clone() is now O(# activated * ticks) and this pr bings it to O(# activated).
I can experiment with that, here and for #5118, but I suspect it will be rare. Not only does the context need not to be cloned, but it needs not to have been cloned since last time that this package was added. Would you like me to do that expremint in this PR or in a follow up?
There was a problem hiding this comment.
I did it in this PR as it made the diff much smaller. New performance numbers on the way.
|
New numbers after using |
|
@bors: r+ |
|
📌 Commit 04fbc5f has been approved by |
use more Rc in the part of resolver that gets cloned a lot 2 This is the same idea as #5118, I was looking at a profile and noted that ~5% of our time was sent dropping `HashMap<PackageId, HashSet<InternedString>>`. A quick rg and I found the culprit, we are cloning the set of features for every new `Context`. With some Rc we are now just cloning for each time we insert. To benchmark I commented out line https://github.com/rust-lang/cargo/blob/b9aa315158fe4d8d63672a49200401922ef7385d/src/cargo/core/resolver/mod.rs#L453 the smallest change to get #4810 (comment) not to solve instantly. Before 17000000 ticks, 90s, 188.889 ticks/ms After 17000000 ticks, 73s, 232.877 ticks/ms
|
☀️ Test successful - status-appveyor, status-travis |
This is the same idea as #5118, I was looking at a profile and noted that ~5% of our time was sent dropping
HashMap<PackageId, HashSet<InternedString>>. A quick rg and I found the culprit, we are cloning the set of features for every newContext. With some Rc we are now just cloning for each time we insert.To benchmark I commented out line
cargo/src/cargo/core/resolver/mod.rs
Line 453 in b9aa315
the smallest change to get #4810 (comment) not to solve instantly.
Before 17000000 ticks, 90s, 188.889 ticks/ms
After 17000000 ticks, 73s, 232.877 ticks/ms