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

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

Merged
merged 8 commits into from
Feb 15, 2018
Merged

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

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9bc5986
38880 don't compute hash when searching an empty HashMap
technicalguy Feb 6, 2018
dcdd2c4
38880 use search_mut function rather than search_hashed
technicalguy Feb 6, 2018
29f7148
38880 remove redundant extra function
technicalguy Feb 12, 2018
fd78621
38880 fixup add missing mut
technicalguy Feb 12, 2018
a295ec1
38880 restore original entry(key) method
technicalguy Feb 13, 2018
94c3c84
38880 hashmap check size=0, not just capacity=0
technicalguy Feb 13, 2018
f3330ce
38880 fix incorrect negation
technicalguy Feb 13, 2018
e034ddd
38880 remove unnecessary self.table.size check
technicalguy Feb 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 36 additions & 8 deletions src/libstd/collections/hash/map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,9 +397,21 @@ pub struct HashMap<K, V, S = RandomState> {
resize_policy: DefaultResizePolicy,
}

/// Search for a pre-hashed key when the hash map is known to be non-empty.
#[inline]
fn search_hashed_nonempty<K, V, M, F>(table: M, hash: SafeHash, is_match: F)
-> InternalEntry<K, V, M>
where M: Deref<Target = RawTable<K, V>>,
F: FnMut(&K) -> bool
{
// Do not check the capacity as an extra branch could slow the lookup.
search_hashed_body(table, hash, is_match)
}

/// Search for a pre-hashed key.
/// If you don't already know the hash, use search or search_mut instead
#[inline]
fn search_hashed<K, V, M, F>(table: M, hash: SafeHash, mut is_match: F) -> InternalEntry<K, V, M>
fn search_hashed<K, V, M, F>(table: M, hash: SafeHash, is_match: F) -> InternalEntry<K, V, M>
where M: Deref<Target = RawTable<K, V>>,
F: FnMut(&K) -> bool
{
Expand All @@ -410,6 +422,16 @@ fn search_hashed<K, V, M, F>(table: M, hash: SafeHash, mut is_match: F) -> Inter
return InternalEntry::TableIsEmpty;
}

search_hashed_body(table, hash, is_match)
}

/// 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this search_hashed_nonempty? Avoiding the extra function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right

-> InternalEntry<K, V, M>
where M: Deref<Target = RawTable<K, V>>,
F: FnMut(&K) -> bool
{
let size = table.size();
let mut probe = Bucket::new(table, hash);
let mut displacement = 0;
Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@technicalguy technicalguy Feb 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor Author

@technicalguy technicalguy Feb 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. HashMaps are hard 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@arthurprs arthurprs Feb 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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()))
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

let hash = self.make_hash(q);
search_hashed_nonempty(&self.table, hash, |k| q.eq(k.borrow()))
} else {
InternalEntry::TableIsEmpty
}
}

#[inline]
fn search_mut<'a, Q: ?Sized>(&'a mut self, q: &Q) -> InternalEntry<K, V, &'a mut RawTable<K, V>>
where K: Borrow<Q>,
Q: Eq + Hash
{
let hash = self.make_hash(q);
search_hashed(&mut self.table, hash, |k| q.eq(k.borrow()))
if self.table.capacity() != 0 {
let hash = self.make_hash(q);
search_hashed_nonempty(&mut self.table, hash, |k| q.eq(k.borrow()))
} else {
InternalEntry::TableIsEmpty
}
}

// The caller should ensure that invariants by Robin Hood Hashing hold
Expand Down Expand Up @@ -1009,9 +1039,7 @@ impl<K, V, S> HashMap<K, V, S>
pub fn entry(&mut self, key: K) -> Entry<K, V> {
// Gotta resize now.
self.reserve(1);
let hash = self.make_hash(&key);
search_hashed(&mut self.table, hash, |q| q.eq(&key))
.into_entry(key).expect("unreachable")
self.search_mut(&key).into_entry(key).expect("unreachable")
}

/// Returns the number of elements in the map.
Expand Down