Early exit for empty HashMap (issue #38880) #48035

technicalguy · 2018-02-06T14:27:53Z

Addresses issue #38880 by checking if the HashMap is empty before computing the value of the hash.

Before (integer keys)

running 4 tests
test empty_once ... bench:          13 ns/iter (+/- 0)
test empty_100  ... bench:       1,367 ns/iter (+/- 35)
test exist_once ... bench:          14 ns/iter (+/- 0)
test exist_100  ... bench:       1,518 ns/iter (+/- 40)

After

running 4 tests
test empty_once ... bench:           2 ns/iter (+/- 0)
test empty_100  ... bench:         221 ns/iter (+/- 0)
test exist_once ... bench:          15 ns/iter (+/- 0)
test exist_100  ... bench:       1,515 ns/iter (+/- 92)

When the HashMap is not empty, the performance remains the same, and when it is empty the performance is significantly improved.

rust-highfive · 2018-02-06T14:28:06Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

This addresses issue rust-lang#38880

technicalguy · 2018-02-06T17:22:20Z

r? @alexcrichton

alexcrichton · 2018-02-07T02:05:26Z

Thanks!

@bluss would you be ok reviewing this?

arthurprs · 2018-02-09T18:19:42Z

src/libstd/collections/hash/map.rs

@@ -550,17 +572,25 @@ impl<K, V, S> HashMap<K, V, S>
        where K: Borrow<Q>,
              Q: Eq + Hash
    {
-        let hash = self.make_hash(q);
-        search_hashed(&self.table, hash, |k| q.eq(k.borrow()))
+        if self.table.capacity() != 0 {


Shouldn't this be checking for size() for the optimization to apply for any empty map? My understanding is that checking for 0 capacity only works for not yet allocated maps.

This seems to be the explanation for that: #38880 (comment)

As it is written, it's a rearrangement of code that should result in no extra branches.

If I understand correctly, self.table.capacity() != 0 is a precondition. We can simplify this by using .size() instead of it (with a comment), and still have equivalent code — but not in the path that we use to find a VacantEntry in an empty map. It would be good to use the .size() != 0 check where possible. Here's it's also important to keep the cross-method logical dependencies clear.

Shouldn't this be checking for size() for the optimization to apply for any empty map?

This is what I wanted to do, but as @bluss points out, the problem is that when the map is empty but allocated (as opposed to empty and unallocated) and it doesn't find a key, it is supposed to return InternalEntry::Vacant. The crucial problem here is that InternalEntry::Vacant requires the hash. This means that you can't get away from computing the hash when the HashMap is allocated, because otherwise you can't return an InternalEntry::Vacant.

This is also a problem for linear search as suggested in #38880. (I did not fully realise this problem until after writing this pull request.) Since an InternalEntry::Vacant requires the hash value, a linear search cannot avoid computing the hash value, so it is not possible to achieve a speedup by avoiding the computation of a hash value. I think this only way to fix this is to change the semantics of InternalEntry::Vacant. (Which would require a not-insignificant reworking, but would allow for the size() == 0 that @arthurprs is considering.)

N.B. I did actually try the size() == 0 aka is_empty() check first, before I realised about the InternalEntry::Vacant semantics: caaabe

Makes sense. HashMaps are hard 😄

Regarding the linear search, for that to yield a net win the backing table would need to support real fast iteration (packed KVs), otherwise iterating the buckets gets more expensive than the hashing. Potentially of interest for https://github.com/bluss/ordermap

I did some benchmarks for a linear search, and I think it might still be advantageous for some cases even if the map is not densely packed. Here's my reasoning – tell me what you think:

Let m be the allocated size of the hashmap and let n be the number of items it contains. Let S be the cost of accessing the next bucket in the hashmap (regardless of if said bucket is empty), let E be the cost of calculating equality for two elements of a given type T, and let H be the cost of calculating the hash for an element of a given type T. Assume that E and H are constant across all elements of type T (this is not true for all types, e.g. strings).

If mS + nE < H then it is faster to perform a linear search lookup for some key than to compute the hash and perform a direct lookup. The average case of a linear search for a key that exists in the map is half of the worst case.

Now if you have a simple type (e.g. integers) then equality is very cheap and constant time, and computing the hash is (surprisingly) expensive (perhaps because of the DoS resilience?), so a linear search is faster for integer-keyed hashmaps with size <= ~ 20 items.

It gets more complicated for other types, such as strings, because the equality operation doesn't take constant time and/or the hash operation doesn't take constant time. (Although for strings I think it is still the case that the hash operation always takes longer than the equality operation.)

Of course all of this is moot if InternalEntry::Vacant requires returning the hash value.

Maybe, it depends on a lot of variables, specially an expensive hasher.

You can try something like this, but I'm afraid calculating VacantEntryState on misses is too expensive (edit: or not, as you don't need to compare keys anymore).

if self.table.capacity() < CC { if table.capacity() == 0: return InternalEntry::TableIsEmpty; } else { // search_linear returns Option<InternalEntry::Occupied> search_linear(&self.table, |k| q.eq(k.borrow())).unwrap_or_else(|| { self.make_hash(q); InternalEntry::Vacant { hash, VacantEntryState::...{...} } }) } else { let hash = self.make_hash(q); search_hashed_nonempty(&mut self.table, hash, |k| q.eq(k.borrow())) }

So then it would be faster when searching for keys that exist, but slower when searching for keys that don't exist, right? I don't think that optimisation is worth it if it slows down the case when a key is not found.

bluss · 2018-02-09T22:24:36Z

Nice work. Especially that your rearrangement shows that this is a win we don't really have to pay anything for. My suggestions are in the previous comment.

arthurprs · 2018-02-12T12:46:31Z

src/libstd/collections/hash/map.rs

+
+/// The body of the search_hashed[_nonempty] functions
+#[inline]
+fn search_hashed_body<K, V, M, F>(table: M, hash: SafeHash, mut is_match: F)


Can we make this search_hashed_nonempty? Avoiding the extra function.

Yes, you're right

arthurprs

LGTM

bluss · 2018-02-12T18:29:37Z

I think there is a refactoring we can do to get the full .size() != 0 benefit, except when using .entry().

in search(&self, ..) we are never inserting, so we could use the size check?
Lots of search_mut users check the size() up front already (see below); it could be refactored so that all possible search_mut users have the size check (and entry uses the one with just capacity check).

rust/src/libstd/collections/hash/map.rs

Lines 2658 to 2664 in fd78621

    
           fn take(&mut self, key: &Q) -> Option<K> { 
        
               if self.table.size() == 0 { 
        
                   return None; 
        
               } 
        
               self.search_mut(key).into_occupied_bucket().map(|bucket| pop_internal(bucket).0) 
        
           }

technicalguy · 2018-02-13T09:21:30Z

I think I see what you mean – I'll take a look.

technicalguy · 2018-02-13T10:29:09Z

Edit: I'm being silly. Please ignore the last commit. The check can stay inside search/search_mut. And entry can use the unmodified search_hashed. Obviously I haven't woken up yet.

Edit 2: Deleted that commit now, gonna rework it

technicalguy · 2018-02-13T16:37:21Z

@bluss good observation. The entry method is the only one which requires an InternalEntry return type. And in fact it originally never used the search[_mut] method. All the other users of search[_mut] call .into_occupied_bucket(), so they don't require an internal entry, and thus the search[_mut] methods can be updated to return an Option<FullBucket<...>>. This means that the search[_mut] methods can be updated to check if the map is empty (contains no elements), not just that the capacity is zero (is unallocated).

Furthermore, this means that there is hope for a linear search optimisation, since we don't actually need to return an InternalEntry in most cases.

arthurprs · 2018-02-13T17:14:08Z

src/libstd/collections/hash/map.rs

-            search_hashed_nonempty(&self.table, hash, |k| q.eq(k.borrow()))
-        } else {
-            InternalEntry::TableIsEmpty
+        if !self.is_empty() {


Is tis ! correct?

bluss · 2018-02-13T19:42:22Z

src/libstd/collections/hash/map.rs

@@ -1274,7 +1295,6 @@ impl<K, V, S> HashMap<K, V, S>
        }


There's a remaining self.table.size() == 0 here that is now redundant

bluss · 2018-02-13T19:47:48Z

I think there are a few more things that can be streamlined if one goes over all uses of search_mut, search_hashed etc. For example search_hashed checks the capacity == 0 case but its both two callers are from a place where capacity > 0. Removing those checks can be weighed in a different PR — plainly removing them would make the code less defensive w.r.t memory safety bugs.

bluss · 2018-02-13T19:51:23Z

@bors delegate=arthurprs

bors · 2018-02-13T19:51:24Z

✌️ @arthurprs can now approve this pull request

arthurprs

LGTM

arthurprs · 2018-02-13T20:39:55Z

@bors r+

bors · 2018-02-13T20:39:56Z

📌 Commit e034ddd has been approved by arthurprs

arthurprs · 2018-02-13T20:41:23Z

Thanks @bluss @technicalguy

…ap-38880, r=arthurprs Early exit for empty HashMap (issue rust-lang#38880) Addresses issue rust-lang#38880 by checking if the HashMap is empty before computing the value of the hash. Before (integer keys) ``` running 4 tests test empty_once ... bench: 13 ns/iter (+/- 0) test empty_100 ... bench: 1,367 ns/iter (+/- 35) test exist_once ... bench: 14 ns/iter (+/- 0) test exist_100 ... bench: 1,518 ns/iter (+/- 40) ``` After ``` running 4 tests test empty_once ... bench: 2 ns/iter (+/- 0) test empty_100 ... bench: 221 ns/iter (+/- 0) test exist_once ... bench: 15 ns/iter (+/- 0) test exist_100 ... bench: 1,515 ns/iter (+/- 92) ``` When the HashMap is not empty, the performance remains the same, and when it is empty the performance is significantly improved.

Rollup of 22 pull requests - Successful merges: #47784, #47806, #47846, #48005, #48033, #48065, #48087, #48114, #48126, #48130, #48133, #48151, #48154, #48156, #48162, #48163, #48165, #48167, #48181, #48186, #48195, #48035 - Failed merges:

Rollup of 23 pull requests - Successful merges: #47784, #47806, #47846, #48005, #48033, #48065, #48087, #48114, #48126, #48130, #48133, #48151, #48154, #48156, #48162, #48163, #48165, #48167, #48181, #48186, #48195, #48035, #48210 - Failed merges:

rust-highfive assigned sfackler Feb 6, 2018

technicalguy force-pushed the Early-exit-empty-hashmap-38880 branch 2 times, most recently from fc823fd to 6a9dbd2 Compare February 6, 2018 14:54

technicalguy added 2 commits February 6, 2018 15:43

38880 don't compute hash when searching an empty HashMap

9bc5986

This addresses issue rust-lang#38880

38880 use search_mut function rather than search_hashed

dcdd2c4

technicalguy force-pushed the Early-exit-empty-hashmap-38880 branch from 6a9dbd2 to dcdd2c4 Compare February 6, 2018 15:43

rust-highfive assigned alexcrichton and unassigned sfackler Feb 6, 2018

BatmanAoD added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 7, 2018

arthurprs reviewed Feb 9, 2018

View reviewed changes

bluss assigned bluss and unassigned alexcrichton Feb 9, 2018

arthurprs reviewed Feb 12, 2018

View reviewed changes

38880 remove redundant extra function

29f7148

arthurprs approved these changes Feb 12, 2018

View reviewed changes

38880 fixup add missing mut

fd78621

technicalguy force-pushed the Early-exit-empty-hashmap-38880 branch from 9158ce2 to fd78621 Compare February 13, 2018 16:17

38880 restore original entry(key) method

a295ec1

38880 hashmap check size=0, not just capacity=0

94c3c84

technicalguy force-pushed the Early-exit-empty-hashmap-38880 branch from fd72616 to 94c3c84 Compare February 13, 2018 16:40

arthurprs reviewed Feb 13, 2018

View reviewed changes

38880 fix incorrect negation

f3330ce

bluss reviewed Feb 13, 2018

View reviewed changes

38880 remove unnecessary self.table.size check

e034ddd

arthurprs approved these changes Feb 13, 2018

View reviewed changes

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 Feb 13, 2018

kennytm mentioned this pull request Feb 14, 2018

Rollup of 23 pull requests #48203

Merged

kennytm merged commit e034ddd into rust-lang:master Feb 15, 2018

technicalguy deleted the Early-exit-empty-hashmap-38880 branch February 15, 2018 17:00

lucab mentioned this pull request Jan 29, 2019

HashMap starts the search even if the map is empty #38880

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Early exit for empty HashMap (issue #38880) #48035

Early exit for empty HashMap (issue #38880) #48035

technicalguy commented Feb 6, 2018 •

edited

Loading

rust-highfive commented Feb 6, 2018

technicalguy commented Feb 6, 2018

alexcrichton commented Feb 7, 2018

arthurprs Feb 9, 2018

bluss Feb 9, 2018

technicalguy Feb 10, 2018 •

edited

Loading

technicalguy Feb 10, 2018 •

edited

Loading

arthurprs Feb 12, 2018

arthurprs Feb 12, 2018

technicalguy Feb 12, 2018

arthurprs Feb 12, 2018 •

edited

Loading

technicalguy Feb 12, 2018

bluss commented Feb 9, 2018

arthurprs Feb 12, 2018

technicalguy Feb 12, 2018

arthurprs left a comment

bluss commented Feb 12, 2018

technicalguy commented Feb 13, 2018

technicalguy commented Feb 13, 2018 •

edited

Loading

technicalguy commented Feb 13, 2018

arthurprs Feb 13, 2018

technicalguy Feb 13, 2018

bluss Feb 13, 2018

bluss commented Feb 13, 2018

bluss commented Feb 13, 2018

bors commented Feb 13, 2018

arthurprs left a comment

arthurprs commented Feb 13, 2018

bors commented Feb 13, 2018

arthurprs commented Feb 13, 2018

Early exit for empty HashMap (issue #38880) #48035

Early exit for empty HashMap (issue #38880) #48035

Conversation

technicalguy commented Feb 6, 2018 • edited Loading

rust-highfive commented Feb 6, 2018

technicalguy commented Feb 6, 2018

alexcrichton commented Feb 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

technicalguy Feb 10, 2018 • edited Loading

Choose a reason for hiding this comment

technicalguy Feb 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arthurprs Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluss commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arthurprs left a comment

Choose a reason for hiding this comment

bluss commented Feb 12, 2018

technicalguy commented Feb 13, 2018

technicalguy commented Feb 13, 2018 • edited Loading

technicalguy commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluss commented Feb 13, 2018

bluss commented Feb 13, 2018

bors commented Feb 13, 2018

arthurprs left a comment

Choose a reason for hiding this comment

arthurprs commented Feb 13, 2018

bors commented Feb 13, 2018

arthurprs commented Feb 13, 2018

technicalguy commented Feb 6, 2018 •

edited

Loading

technicalguy Feb 10, 2018 •

edited

Loading

technicalguy Feb 10, 2018 •

edited

Loading

arthurprs Feb 12, 2018 •

edited

Loading

technicalguy commented Feb 13, 2018 •

edited

Loading