Type walker small vector #37760
Type walker small vector #37760
Conversation
|
r? @arielb1 (rust_highfive has picked a reviewer for you, use r? to override) |
| @@ -127,6 +127,11 @@ pub enum DepNode<D: Clone + Debug> { | |||
| TraitItems(D), | |||
| ReprHints(D), | |||
| TraitSelect(Vec<D>), | |||
There was a problem hiding this comment.
Is there a reason we add a TraitSelectSingle instead of making TraitSelect SmallVec<[D; 1]>?
There was a problem hiding this comment.
I did it that way because using SmallVec would likely increase the size of DepNode. But I haven't measured and I'm not sure how much that would matter.
There was a problem hiding this comment.
Seems unlikely, but it's possible. SmallVec has no size overhead so long as size_of(D) + size_of(usize) <= size_of(Vec<D>), and I don't know if that's true. DepNode mostly stores either D, Vec<D>, or Arc<WorkProductId>.
There was a problem hiding this comment.
For at least one case, it increases the siz of DepNode from 32 bytes to 40 bytes on 64-bit. DepNode is used in HashMaps and Graphs so I'm leery about increasing its size.
| while len < self.len() { | ||
| // Decrement len before the drop_in_place(), so a panic on Drop | ||
| // doesn't re-drop the just-failed value. | ||
| let len = self.len() - 1; |
There was a problem hiding this comment.
this particular shadowing confuses more than it helps; it might as well use a different variable name.
There was a problem hiding this comment.
I can change it, but it's also worth noting that I copied this code (with minor modifications) from vec.rs.
|
Please don't approve this without further review. Adding new kinds of DepNodes should be done carefully. cc @nikomatsakis |
|
|
||
| // An optional alternative to `TraitSelect` that avoids a heap allocation | ||
| // in the case where there is a single D. (Note that `TraitSelect` is still | ||
| // allowed to contain a Vec with a single D.) |
There was a problem hiding this comment.
It may be better to avoid vecs altogether and change to
TraitSelect { trait_def_id: DefId, self_def_id: Option<DefId> }Or:
TraitSelect { trait_def_id: DefId },
TraitSelectNominal { trait_def_id: DefId, self_def_id: DefId },The idea would be that we just extract the first def-id we find in the self type for the trait, rather than extracting all def-ids that we find in any of the input types. (And, if there is no def-id in the self-type, then we go to the trait-only variant.)
I've not had a lot of coffee yet this morning but off hand that seems fine.
There was a problem hiding this comment.
(Obviously it will affect incremental resolution, but probably not in any major way -- it's still the case that we can distinguish vec.clone() from string.clone())
There was a problem hiding this comment.
Sure. I don't think the "type parameter" case is that much important here, and eventually we probably want to rethink how we handle this anyway.
There was a problem hiding this comment.
@nikomatsakis: I'm having trouble seeing how the Ds in the old TraitSelect variant would map to the Ds in the new one. The two places where a TraitSelect variant is created are TraitPredicate::dep_node and ProjectionCache::to_dep_node. The former is guaranteed to produce a vector of length 1 or more due to the iter::once, but the latter isn't.
There was a problem hiding this comment.
Isn't the "correct" thing to use here some form of the fast_reject "type skeletons"?
There was a problem hiding this comment.
I think I tried them and had problems, but now I'm not 100% sure why. I guess because you can't "map" them as easily. You'd have to refactor, and I'm not sure I see the point. The main thing is to ensure that Vec: Clone doesn't interfere with String: Clone and the like.
There was a problem hiding this comment.
Btw, looks like ProjectionCache::to_dep_node really wants multiple nodes, could the API be changed to be an "internal iterator" i.e. each_dep_node?
brson
added
the
relnotes
The change also adds the missing `SmallVec::truncate` method.
nnethercote
force-pushed
the
TypeWalker-SmallVector
branch
from
6db12d4
to
f72685f
Compare
|
I updated the PR to cover just the first commit. @nikomatsakis is working on the r? @arielb1 |
|
@bors r+ rollup |
|
📌 Commit f72685f has been approved by |
… r=arielb1 Type walker small vector These two changes avoid allocations on some hot paths and speed up a few workloads (some from rustc-benchmarks, as well as the workload from rust-lang#36799) by 1--2%.
These two changes avoid allocations on some hot paths and speed up a few workloads (some from rustc-benchmarks, as well as the workload from #36799) by 1--2%.