Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use Vec::with_capacity(1) in a couple of places. #50762

Closed
wants to merge 1 commit into from
Closed

Use Vec::with_capacity(1) in a couple of places. #50762

wants to merge 1 commit into from

Conversation

nnethercote
Copy link
Contributor

Because Vec::new() + push() results in a capacity of 4, and these
particular vectors almost never grow past a length of 1.

This change reduces the number of bytes of heap allocation
significantly. (For serde it's a 15% reduction for a debug build.) This
didn't give noticeable speed-ups on my machine but it's a trivial change
and certainly can't hurt.

@rust-highfive
Copy link
Collaborator

r? @eddyb

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 15, 2018
arthurprs
arthurprs reviewed May 15, 2018
View reviewed changes
src/librustc/middle/dataflow.rs Outdated
@@ -181,7 +181,7 @@ fn build_local_id_to_index(body: Option<&hir::Body>,

cfg.graph.each_node(|node_idx, node| {
if let cfg::CFGNodeData::AST(id) = node.data {
index.entry(id).or_insert(vec![]).push(node_idx);
index.entry(id).or_insert(Vec::with_capacity(1)).push(node_idx);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably make the allocation lazy with .or_insert_with(|| Vec::with_capacity(1))

eddyb
eddyb reviewed May 15, 2018
View reviewed changes
src/librustc/middle/dataflow.rs Outdated
@@ -209,7 +209,8 @@ fn build_local_id_to_index(body: Option<&hir::Body>,
}

fn visit_pat(&mut self, p: &hir::Pat) {
self.index.entry(p.hir_id.local_id).or_insert(vec![]).push(self.entry);
self.index.entry(p.hir_id.local_id)
.or_insert(Vec::with_capacity(1)).push(self.entry);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arthurprs' comment about making or_insert lazy also applies here.

@Mark-Simulacrum
Copy link
Member

Is there a reason these aren't being changed to SmallVec<[T; 1]>? That would seem like it probably would net similar wins...

@nnethercote
Copy link
Contributor Author

Is there a reason these aren't being changed to SmallVec<[T; 1]>? That would seem like it probably would net similar wins...

For the ty.obligations case -- I tried SmallVec, and I got up to 10% slowdowns. It reduced the number of allocations, but the number of calls to memcpy jumped dramatically. This is because Vec<PredicateObligation> is 24 bytes and SmallVec<[PredicateObligation; 1]> is much bigger, something like 152 bytes or more, and we have quite a lot of these vectors and they get copied around a lot, and I guess once they get bigger than a certain size the compiler inserts memcpy calls to do the copying.

So the improvement for ty.obligations is to produce vectors with length=1 and capacity=1 instead of length=1 and capacity=4.

(BTW, it's interesting that these vectors get copied so much. I considered changing them to boxed slices to make them smaller, but that didn't work out because many of them need to be modifiable. I'd be interested to hear other ideas on this front.)

For the cfg case I didn't try using SmallVec. It would increase the size of the hash table entries, but they are just indices and so small. I'll try it out.

@nnethercote
Use Vec::with_capacity(1) in a couple of places.
4123f80 
Because Vec::new() + push() results in a capacity of 4, and these
particular vectors almost never grow past a length of 1.

This change reduces the number of bytes of heap allocation
significantly. (For serde it's a 15% reduction for a debug build.) This
didn't give noticeable speed-ups on my machine but it's a trivial change
and certainly can't hurt.
@nnethercote
Copy link
Contributor Author

I tried SmallVec for cfg. Those allocations aren't hot enough to make a noticeable difference... I'll stick with the with_capacity change because it's a much smaller change, and so arguably justifiable.

@eddyb
Copy link
Member

eddyb commented May 16, 2018

@bors r+

@bors
Copy link
Contributor

bors commented May 16, 2018

📌 Commit 4123f80 has been approved by eddyb

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 16, 2018
@nnethercote
Copy link
Contributor Author

Actually, I've worked out a better way of doing things. @eddyb, can you please unapprove this PR?

@hanna-kruppe
Copy link
Contributor

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 16, 2018
@nnethercote
Copy link
Contributor Author

#50818 is the much-improved version.

@nnethercote nnethercote deleted the with_cap-1 branch December 12, 2018 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddyb eddyb eddyb left review comments

@arthurprs arthurprs arthurprs left review comments

Assignees

@eddyb eddyb

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@nnethercote @rust-highfive @Mark-Simulacrum @eddyb @bors @hanna-kruppe @arthurprs