use more Rc in the part of resolver that gets cloned a lot #5118
Conversation
In a test on rust-lang#4810 (comment) Before we got to 1700000 ticks in ~(82 to 97) sec After we got to 1700000 ticks in ~(63 to 67) sec
|
(rust_highfive has picked a reviewer for you, use r? to override) |
| prev.push(summary.clone()); | ||
| let mut inner: Vec<_> = (**prev).clone(); | ||
| inner.push(summary.clone()); | ||
| *prev = Rc::new(inner); |
There was a problem hiding this comment.
If we're optimizing for clones here, I wonder if this'd be better managed through an RcList perhaps?
There was a problem hiding this comment.
I was thinking of that, but we very often iter over the activated items. Specifically is_active, prev_active, and flag_activated. So it would be a pretty invasive change. I can give it a try if you want.
Also before we cloned every vec for every clone of Context for # clone vecs = O(# activated * ticks). Now we are cloning a vec each time we activate for # clone vecs = O(# activated). If I am miss reading it please correct me.
|
@bors: r+ |
|
📌 Commit af60861 has been approved by |
|
⌛ Testing commit af60861 with merge 30f5f6d7a383cfab54f8d48dd4bec0598d694840... |
|
💔 Test failed - status-travis |
|
@bors: retry |
use more Rc in the part of resolver that gets cloned a lot In a test on #4810 (comment) Before we got to 1700000 ticks in ~(82 to 97) sec After we got to 1700000 ticks in ~(63 to 67) sec
|
☀️ Test successful - status-appveyor, status-travis |
String interning This builds on the work from #5118. This interns the strings in the part of resolver that gets cloned a lot. In a test on #4810 (comment) Before we got to 1700000 ticks in ~(63 to 67) sec from #5118 After we got to 1700000 ticks in ~(42 to 45) sec The interning code itself would be much better with a `leak` function that converts a `String` to a `&'static str`. Something like: ```rust pub fn leek(s: String) -> &'static str { let ptr = s.as_ptr(); let len = s.len(); mem::forget(s); unsafe { let slice = slice::from_raw_parts(ptr, len); str::from_utf8(slice).unwrap() } } ``` but "there is no unsafe in Cargo", and I am not the best at unsafe. So I just `to_string` and lived with the extra copy. Is there a better way to hand out references? I assumed that `InternedString::new` world start appearing in profile result, and that we would want `PackageId`, and `Summary`, Et Al. to store the `InternedString`. That is why I put the interner in a shared folder. So far it is just used in the resolver. It may make sense for a lot more of the Strings to be interned, but with the extra copy... I have not explored it yet.
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
In a test on #4810 (comment)
Before we got to 1700000 ticks in ~(82 to 97) sec
After we got to 1700000 ticks in ~(63 to 67) sec