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.

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

Use .reserve() in HashMap and HashSet's extend methods #36580

Closed
wants to merge 1 commit into from

Conversation

bluss
Copy link
Member

@bluss bluss commented Sep 19, 2016

It was revealed that hashmaps perform very poorly in this example code:

let first_map: HashMap<u64, _> = (0..900000).map(|i| (i, ())).collect();
let second_map: HashMap<u64, _> = (900000..1800000).map(|i| (i, ())).collect();

let mut merged = first_map;
merged.extend(second_map);

The explanation is that the iterators visit the keys in their bucket
order, and if both maps have the same hash function (including key),
that means that during .extend() the early buckets are overpopulated
long before the whole hashmap's fill level increases to the point that it
triggers it to grow its allocation.

Fixes #36579

It was revealed that hashmaps perform very poorly in this example code:

```rust
let first_map: HashMap<u64, _> = (0..900000).map(|i| (i, ())).collect();
let second_map: HashMap<u64, _> = (900000..1800000).map(|i| (i, ())).collect();

let mut merged = first_map;
merged.extend(second_map);
```

The explanation is that the iterators visit the keys in their bucket
order, and if both maps have the same hash function (including key),
that means that during .extend() the early buckets are overpopulated
long before the whole hashmap's fill level increases to the point that it
triggers it to grow its allocation.
@rust-highfive
Copy link
Collaborator

r? @aturon

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

@bluss
Copy link
Member Author

bluss commented Sep 19, 2016

I have not built & benchmarked the difference yet, but I can't see a reason not to grow the hash map (and set) up front. So it remains to see what the effect is on the extend case.

@jfager
Copy link
Contributor

jfager commented Sep 19, 2016

Isn't this going to hurt performance for maps that have high overlap?

@bluss
Copy link
Member Author

bluss commented Sep 19, 2016

@jfager That is a good point

@bluss bluss closed this Sep 19, 2016
@bluss
Copy link
Member Author

bluss commented Sep 19, 2016

I retract this. Thanks for the review, it makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants