Type walker small vector#37760

nnethercote · 2016-11-14T03:01:24Z

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%.

rust-highfive · 2016-11-14T03:01:30Z

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

Mark-Simulacrum · 2016-11-14T03:39:15Z

src/librustc/dep_graph/dep_node.rs

@@ -127,6 +127,11 @@ pub enum DepNode<D: Clone + Debug> {
    TraitItems(D),
    ReprHints(D),
    TraitSelect(Vec<D>),


Is there a reason we add a TraitSelectSingle instead of making TraitSelect SmallVec<[D; 1]>?

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.

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>.

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.

bluss · 2016-11-14T08:50:54Z

src/librustc_data_structures/small_vec.rs

+            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;


this particular shadowing confuses more than it helps; it might as well use a different variable name.

I can change it, but it's also worth noting that I copied this code (with minor modifications) from vec.rs.

michaelwoerister · 2016-11-15T16:08:20Z

Please don't approve this without further review. Adding new kinds of DepNodes should be done carefully. cc @nikomatsakis

nikomatsakis · 2016-11-15T16:13:32Z

src/librustc/dep_graph/dep_node.rs

+
+    // 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.)


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.

(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())

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.

@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.

Isn't the "correct" thing to use here some form of the fast_reject "type skeletons"?

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.

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?

The change also adds the missing `SmallVec::truncate` method.

nnethercote · 2016-11-21T23:56:16Z

I updated the PR to cover just the first commit. @nikomatsakis is working on the DepNode code and will post a separate PR for that.

r? @arielb1

arielb1 · 2016-11-22T18:41:08Z

@bors r+ rollup

bors · 2016-11-22T18:41:09Z

📌 Commit f72685f has been approved by arielb1

… 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%.

Rollup of 7 pull requests - Successful merges: #37442, #37760, #37836, #37851, #37859, #37913, #37925 - Failed merges:

rust-highfive assigned arielb1 Nov 14, 2016

Mark-Simulacrum approved these changes Nov 14, 2016

View reviewed changes

bluss reviewed Nov 14, 2016

View reviewed changes

nikomatsakis reviewed Nov 15, 2016

View reviewed changes

brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 16, 2016

Use SmallVec for TypeWalker's stack.

f72685f

The change also adds the missing `SmallVec::truncate` method.

nnethercote force-pushed the TypeWalker-SmallVector branch from 6db12d4 to f72685f Compare November 21, 2016 23:54

GuillaumeGomez mentioned this pull request Nov 22, 2016

Rollup of 7 pull requests #37937

Merged

bors added a commit that referenced this pull request Nov 23, 2016

Auto merge of #37937 - GuillaumeGomez:rollup, r=GuillaumeGomez

127a83d

Rollup of 7 pull requests - Successful merges: #37442, #37760, #37836, #37851, #37859, #37913, #37925 - Failed merges:

bors merged commit f72685f into rust-lang:master Nov 23, 2016

nnethercote deleted the TypeWalker-SmallVector branch November 23, 2016 22:14

Uh oh!

Conversation

nnethercote commented Nov 14, 2016

Uh oh!

rust-highfive commented Nov 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelwoerister commented Nov 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nnethercote commented Nov 21, 2016

Uh oh!

arielb1 commented Nov 22, 2016

Uh oh!

bors commented Nov 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants