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

Dropping empty map takes time proportional to capacity #170

Closed
jonhoo opened this issue Jul 1, 2020 · 5 comments · Fixed by #182
Closed

Dropping empty map takes time proportional to capacity #170

jonhoo opened this issue Jul 1, 2020 · 5 comments · Fixed by #182

Comments

@jonhoo
Copy link
Contributor

jonhoo commented Jul 1, 2020

We currently iterate over the map on drop:

hashbrown/src/raw/mod.rs

Lines 1194 to 1195 in efbd036

if mem::needs_drop::<T>() {
for item in self.iter() {

And the iterator iterates over all the buckets, even if it knows there are no more items (RawIter::items == 0):

hashbrown/src/raw/mod.rs

Lines 1401 to 1402 in efbd036

fn next(&mut self) -> Option<Bucket<T>> {
if let Some(b) = self.iter.next() {

This means that code like this will unnecessarily take fairly long:

let map: HashMap<String, String> = HashMap::with_capacity(1024 * 1024);
drop(map);

The above is a little contrived, but the same occurs if the caller calls drain or clear, and then drops the map. Not sure if this is something we want to optimize for, but the fix is fairly straightforward.

jonhoo added a commit to jonhoo/hashbrown that referenced this issue Jul 1, 2020
@jonhoo
Copy link
Contributor Author

jonhoo commented Jul 1, 2020

I suppose this is mitigated by the fact that we iterate over the control bytes, not the actual buckets, so we're iterating through many buckets at once. Even so, it seems like it'd be nice to avoid this iteration altogether when we can?

@cuviper
Copy link
Member

cuviper commented Jul 1, 2020

I guess the places that check needs_drop could easily check if it's already empty too.

@jonhoo
Copy link
Contributor Author

jonhoo commented Jul 1, 2020

Yup: jonhoo@8dd0412

@cuviper
Copy link
Member

cuviper commented Aug 3, 2020

@jonhoo Are you going to submit that commit in a new pull request? I see that it was an incidental part of the closed PR #167, but that was not included in the replacement PR #175.

@jonhoo
Copy link
Contributor Author

jonhoo commented Aug 3, 2020

@cuviper Ah, good call! Submitted as #182.

bors added a commit that referenced this issue Aug 4, 2020
Do not iterate to drop if empty

If the table is empty, there is no need to iterate over all the buckets
in order to drop their contents.

Fixes #170.
@bors bors closed this as completed in 284960c Aug 4, 2020
@bors bors closed this as completed in #182 Aug 4, 2020
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 a pull request may close this issue.

2 participants