Smarter HashMap/HashSet pre-allocation for extend/from_iter #38017

Merged
merged 1 commit into from Dec 7, 2016

Projects

None yet

8 participants

@arthurprs
Contributor
arthurprs commented Nov 26, 2016 edited

HashMap/HashSet from_iter and extend are making totally different assumptions.

A more balanced decision may allocate half the lower hint (rounding up). For "well defined" iterators this effectively limits the worst case to two resizes (the initial reserve + one resize).

cc #36579
cc @bluss

@rust-highfive
Collaborator

r? @aturon

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

src/libstd/collections/hash/set.rs
+ let iter = iter.into_iter();
+ let hint = iter.size_hint().0;
+ self.reserve((hint / 2) + (hint & 1));
+ for (k, v) in iter {
@sfackler
sfackler Nov 26, 2016 Member

This will not compile :P

Might be simpler to just call down to self.map.extend(iter.into_iter().map(|k| (k, ())).

@arthurprs
arthurprs Nov 26, 2016 Contributor

I totally butchered the set part... will amend in a bit.

@arthurprs
Contributor

I wonder if it should try to take into consideration the upper bound.

src/libstd/collections/hash/map.rs
+ // will only resize twice in the worst case.
+ let iter = iter.into_iter();
+ let hint = iter.size_hint().0;
+ self.reserve((hint / 2) + (hint & 1));
@ranma42
ranma42 Nov 27, 2016 Contributor

Usually to round up you add (denom-1) before the division, as in (hint + 1) / 2.
(Note that the current code is correct, it is just a little unusual and it would require nontrivial changes if the divisor was changed)

@arthurprs
arthurprs Nov 27, 2016 Contributor

That makes much more sense, will fix.

@alexcrichton alexcrichton added the T-libs label Nov 28, 2016
@bluss
Contributor
bluss commented Nov 29, 2016

What I wanted to do here was to try this on our favourite hashmap workload (rustc) and see what the effect is. Haven't had time to do it yet.

@arthurprs
Contributor

That's a great idea, thanks!

@bluss
Contributor
bluss commented Nov 30, 2016

It's noisy, results are a bit all over the place (So they are about the same).

./compare.py ~/src/rust/build-before-hashmap/x86_64-unknown-linux-gnu/stage1/bin/rustc ~/src/rust/build/x86_64-unknown-linux-gnu/stage1/bin/rustc
futures-rs-test  5.152s vs  5.029s --> 1.024x faster (variance: 1.006x, 1.021x)
helloworld       0.251s vs  0.279s --> 0.899x faster (variance: 1.645x, 1.277x)
html5ever-2016-  6.668s vs  6.813s --> 0.979x faster (variance: 1.029x, 1.009x)
hyper.0.5.0      5.995s vs  5.935s --> 1.010x faster (variance: 1.015x, 1.034x)
inflate-0.1.0    4.583s vs  4.560s --> 1.005x faster (variance: 1.032x, 1.008x)
issue-32062-equ  0.341s vs  0.373s --> 0.913x faster (variance: 1.182x, 1.242x)
issue-32278-big  2.052s vs  2.076s --> 0.988x faster (variance: 1.054x, 1.033x)
jld-day15-parse  1.654s vs  1.624s --> 1.018x faster (variance: 1.098x, 1.059x)
piston-image-0. 13.449s vs 13.717s --> 0.981x faster (variance: 1.010x, 1.024x)
regex-0.1.80    10.920s vs 10.854s --> 1.006x faster (variance: 1.016x, 1.009x)
regex.0.1.30     2.928s vs  2.862s --> 1.023x faster (variance: 1.014x, 1.020x)
rust-encoding-0  2.359s vs  2.360s --> 1.000x faster (variance: 1.011x, 1.039x)
syntex-0.42.2    0.069s vs  0.077s --> 0.900x faster (variance: 1.312x, 1.156x)

For example, we can retry piston-image and syntex:

piston-image-0. 13.462s vs 13.443s --> 1.001x faster (variance: 1.012x, 1.025x)
syntex-0.42.2    0.071s vs  0.081s --> 0.887x faster (variance: 1.241x, 1.216x)

Why is syntex so quick, what happened with that?

I also looked at memory usage in time-passes for piston-image and it is the same before and after.

@arthurprs
Contributor

Thanks @bluss!

If we want to be extra sure not to regress we can revert the from_iter part, or,

reserve the entire hint on extend if the hashmap is empty, since it's essentially the same runtime cost (except memory of course).

@bluss
Contributor
bluss commented Nov 30, 2016

According to the result, something is regressing in the super small crates (hello world, issue-32062-equ, and syntex) and the rest are as good as unchanged.

@arthurprs arthurprs Smarter HashMap/HashSet extend
2c5d240
@arthurprs
Contributor

I changed it to reserve everything if the map is empty, so if the regression was due to slower from_iter it should now be fixed.

@bluss
Contributor
bluss commented Dec 6, 2016

@bors r+

Let's go with this; extend will reserve the size hint's lower bound by half.

@bors
Contributor
bors commented Dec 6, 2016

📌 Commit 2c5d240 has been approved by bluss

@bluss
Contributor
bluss commented Dec 6, 2016

@bors r-

Oops, I guess T-libs means that the libs team want to discuss it first?

@alexcrichton
Member

@bors: r=bluss

oh I actually just meant that as categorization, this is fine to go through without libs discussion :)

@bors
Contributor
bors commented Dec 6, 2016

📌 Commit 2c5d240 has been approved by bluss

@bluss
Contributor
bluss commented Dec 6, 2016

great, thanks for the info

@bors
Contributor
bors commented Dec 6, 2016

⌛️ Testing commit 2c5d240 with merge 5f128ed...

@bors bors added a commit that referenced this pull request Dec 6, 2016
@bors bors Auto merge of #38017 - arthurprs:hm-extend, r=bluss
Smarter HashMap/HashSet pre-allocation for extend/from_iter

HashMap/HashSet from_iter and extend are making totally different assumptions.

A more balanced decision may allocate half the lower hint (rounding up). For "well defined" iterators this effectively limits the worst case to two resizes (the initial reserve + one resize).

cc #36579
cc @bluss
5f128ed
@bors bors merged commit 2c5d240 into rust-lang:master Dec 7, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment