From 9bc59865f1042b8c6f3a7c69f0d2643b37a41d75 Mon Sep 17 00:00:00 2001 From: Shaun Steenkamp Date: Tue, 6 Feb 2018 08:56:27 +0000 Subject: [PATCH 1/8] 38880 don't compute hash when searching an empty HashMap This addresses issue #38880 --- src/libstd/collections/hash/map.rs | 40 ++++++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/src/libstd/collections/hash/map.rs b/src/libstd/collections/hash/map.rs index 82a687ae5e493..74c4382f16a5d 100644 --- a/src/libstd/collections/hash/map.rs +++ b/src/libstd/collections/hash/map.rs @@ -397,9 +397,21 @@ pub struct HashMap { resize_policy: DefaultResizePolicy, } +/// Search for a pre-hashed key when the hash map is known to be non-empty. +#[inline] +fn search_hashed_nonempty(table: M, hash: SafeHash, is_match: F) + -> InternalEntry + where M: Deref>, + 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(table: M, hash: SafeHash, mut is_match: F) -> InternalEntry +fn search_hashed(table: M, hash: SafeHash, is_match: F) -> InternalEntry where M: Deref>, F: FnMut(&K) -> bool { @@ -410,6 +422,16 @@ fn search_hashed(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(table: M, hash: SafeHash, mut is_match: F) + -> InternalEntry + where M: Deref>, + F: FnMut(&K) -> bool +{ let size = table.size(); let mut probe = Bucket::new(table, hash); let mut displacement = 0; @@ -550,8 +572,12 @@ impl HashMap where K: Borrow, Q: Eq + Hash { - let hash = self.make_hash(q); - search_hashed(&self.table, hash, |k| q.eq(k.borrow())) + if self.table.capacity() != 0 { + let hash = self.make_hash(q); + search_hashed_nonempty(&self.table, hash, |k| q.eq(k.borrow())) + } else { + InternalEntry::TableIsEmpty + } } #[inline] @@ -559,8 +585,12 @@ impl HashMap where K: Borrow, 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 From dcdd2c42d3c7f6125583bcdf0c5ed7e1ef7086db Mon Sep 17 00:00:00 2001 From: Shaun Steenkamp Date: Tue, 6 Feb 2018 14:16:54 +0000 Subject: [PATCH 2/8] 38880 use search_mut function rather than search_hashed --- src/libstd/collections/hash/map.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/libstd/collections/hash/map.rs b/src/libstd/collections/hash/map.rs index 74c4382f16a5d..04c9f617d019f 100644 --- a/src/libstd/collections/hash/map.rs +++ b/src/libstd/collections/hash/map.rs @@ -1039,9 +1039,7 @@ impl HashMap pub fn entry(&mut self, key: K) -> Entry { // 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. From 29f71488bc50843b65660867ab41e6ebf1101e6e Mon Sep 17 00:00:00 2001 From: Shaun Steenkamp Date: Mon, 12 Feb 2018 14:00:08 +0000 Subject: [PATCH 3/8] 38880 remove redundant extra function --- src/libstd/collections/hash/map.rs | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/libstd/collections/hash/map.rs b/src/libstd/collections/hash/map.rs index 04c9f617d019f..d798854927d32 100644 --- a/src/libstd/collections/hash/map.rs +++ b/src/libstd/collections/hash/map.rs @@ -397,17 +397,6 @@ pub struct HashMap { resize_policy: DefaultResizePolicy, } -/// Search for a pre-hashed key when the hash map is known to be non-empty. -#[inline] -fn search_hashed_nonempty(table: M, hash: SafeHash, is_match: F) - -> InternalEntry - where M: Deref>, - 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] @@ -422,16 +411,19 @@ fn search_hashed(table: M, hash: SafeHash, is_match: F) -> InternalE return InternalEntry::TableIsEmpty; } - search_hashed_body(table, hash, is_match) + search_hashed_nonempty(table, hash, is_match) } -/// The body of the search_hashed[_nonempty] functions + +/// Search for a pre-hashed key when the hash map is known to be non-empty. #[inline] -fn search_hashed_body(table: M, hash: SafeHash, mut is_match: F) +fn search_hashed_nonempty(table: M, hash: SafeHash, is_match: F) -> InternalEntry where M: Deref>, F: FnMut(&K) -> bool { + // Do not check the capacity as an extra branch could slow the lookup. + let size = table.size(); let mut probe = Bucket::new(table, hash); let mut displacement = 0; From fd78621717e4f9f73b41256627bfe3a83aa5e660 Mon Sep 17 00:00:00 2001 From: Shaun Steenkamp Date: Mon, 12 Feb 2018 14:53:09 +0000 Subject: [PATCH 4/8] 38880 fixup add missing mut --- src/libstd/collections/hash/map.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstd/collections/hash/map.rs b/src/libstd/collections/hash/map.rs index d798854927d32..c4f3fdc283ef3 100644 --- a/src/libstd/collections/hash/map.rs +++ b/src/libstd/collections/hash/map.rs @@ -417,7 +417,7 @@ fn search_hashed(table: M, hash: SafeHash, is_match: F) -> InternalE /// Search for a pre-hashed key when the hash map is known to be non-empty. #[inline] -fn search_hashed_nonempty(table: M, hash: SafeHash, is_match: F) +fn search_hashed_nonempty(table: M, hash: SafeHash, mut is_match: F) -> InternalEntry where M: Deref>, F: FnMut(&K) -> bool From a295ec1ec9d7616474a620a8acd15944aaf0c638 Mon Sep 17 00:00:00 2001 From: Shaun Steenkamp Date: Tue, 13 Feb 2018 16:32:35 +0000 Subject: [PATCH 5/8] 38880 restore original entry(key) method --- src/libstd/collections/hash/map.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libstd/collections/hash/map.rs b/src/libstd/collections/hash/map.rs index c4f3fdc283ef3..23a932c7aa360 100644 --- a/src/libstd/collections/hash/map.rs +++ b/src/libstd/collections/hash/map.rs @@ -1031,7 +1031,9 @@ impl HashMap pub fn entry(&mut self, key: K) -> Entry { // Gotta resize now. self.reserve(1); - self.search_mut(&key).into_entry(key).expect("unreachable") + let hash = self.make_hash(&key); + search_hashed(&mut self.table, hash, |q| q.eq(&key)) + .into_entry(key).expect("unreachable") } /// Returns the number of elements in the map. From 94c3c84b6a9c382862b1f750f782c33256fa58bd Mon Sep 17 00:00:00 2001 From: Shaun Steenkamp Date: Tue, 13 Feb 2018 16:33:00 +0000 Subject: [PATCH 6/8] 38880 hashmap check size=0, not just capacity=0 --- src/libstd/collections/hash/map.rs | 54 +++++++++++++----------------- 1 file changed, 24 insertions(+), 30 deletions(-) diff --git a/src/libstd/collections/hash/map.rs b/src/libstd/collections/hash/map.rs index 23a932c7aa360..fdc62be3dd960 100644 --- a/src/libstd/collections/hash/map.rs +++ b/src/libstd/collections/hash/map.rs @@ -414,7 +414,6 @@ fn search_hashed(table: M, hash: SafeHash, is_match: F) -> InternalE search_hashed_nonempty(table, hash, is_match) } - /// Search for a pre-hashed key when the hash map is known to be non-empty. #[inline] fn search_hashed_nonempty(table: M, hash: SafeHash, mut is_match: F) @@ -557,32 +556,36 @@ impl HashMap } /// Search for a key, yielding the index if it's found in the hashtable. - /// If you already have the hash for the key lying around, use - /// search_hashed. + /// If you already have the hash for the key lying around, or if you need an + /// InternalEntry, use search_hashed or search_hashed_nonempty. #[inline] - fn search<'a, Q: ?Sized>(&'a self, q: &Q) -> InternalEntry> + fn search<'a, Q: ?Sized>(&'a self, q: &Q) + -> Option>> where K: Borrow, Q: Eq + Hash { - if self.table.capacity() != 0 { - let hash = self.make_hash(q); - search_hashed_nonempty(&self.table, hash, |k| q.eq(k.borrow())) - } else { - InternalEntry::TableIsEmpty + if !self.is_empty() { + return None; } + + let hash = self.make_hash(q); + search_hashed_nonempty(&self.table, hash, |k| q.eq(k.borrow())) + .into_occupied_bucket() } #[inline] - fn search_mut<'a, Q: ?Sized>(&'a mut self, q: &Q) -> InternalEntry> + fn search_mut<'a, Q: ?Sized>(&'a mut self, q: &Q) + -> Option>> where K: Borrow, Q: Eq + Hash { - 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 + if self.is_empty() { + return None; } + + let hash = self.make_hash(q); + search_hashed_nonempty(&mut self.table, hash, |k| q.eq(k.borrow())) + .into_occupied_bucket() } // The caller should ensure that invariants by Robin Hood Hashing hold @@ -1140,7 +1143,7 @@ impl HashMap where K: Borrow, Q: Hash + Eq { - self.search(k).into_occupied_bucket().map(|bucket| bucket.into_refs().1) + self.search(k).map(|bucket| bucket.into_refs().1) } /// Returns true if the map contains a value for the specified key. @@ -1167,7 +1170,7 @@ impl HashMap where K: Borrow, Q: Hash + Eq { - self.search(k).into_occupied_bucket().is_some() + self.search(k).is_some() } /// Returns a mutable reference to the value corresponding to the key. @@ -1196,7 +1199,7 @@ impl HashMap where K: Borrow, Q: Hash + Eq { - self.search_mut(k).into_occupied_bucket().map(|bucket| bucket.into_mut_refs().1) + self.search_mut(k).map(|bucket| bucket.into_mut_refs().1) } /// Inserts a key-value pair into the map. @@ -1256,11 +1259,7 @@ impl HashMap where K: Borrow, Q: Hash + Eq { - if self.table.size() == 0 { - return None; - } - - self.search_mut(k).into_occupied_bucket().map(|bucket| pop_internal(bucket).1) + self.search_mut(k).map(|bucket| pop_internal(bucket).1) } /// Removes a key from the map, returning the stored key and value if the @@ -1296,7 +1295,6 @@ impl HashMap } self.search_mut(k) - .into_occupied_bucket() .map(|bucket| { let (k, v, _) = pop_internal(bucket); (k, v) @@ -2654,15 +2652,11 @@ impl super::Recover for HashMap #[inline] fn get(&self, key: &Q) -> Option<&K> { - self.search(key).into_occupied_bucket().map(|bucket| bucket.into_refs().0) + self.search(key).map(|bucket| bucket.into_refs().0) } fn take(&mut self, key: &Q) -> Option { - if self.table.size() == 0 { - return None; - } - - self.search_mut(key).into_occupied_bucket().map(|bucket| pop_internal(bucket).0) + self.search_mut(key).map(|bucket| pop_internal(bucket).0) } #[inline] From f3330cea7f43288362fec9b010b04e22dfbf45a2 Mon Sep 17 00:00:00 2001 From: Shaun Steenkamp Date: Tue, 13 Feb 2018 17:15:58 +0000 Subject: [PATCH 7/8] 38880 fix incorrect negation --- src/libstd/collections/hash/map.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstd/collections/hash/map.rs b/src/libstd/collections/hash/map.rs index fdc62be3dd960..a8f419d6c6c80 100644 --- a/src/libstd/collections/hash/map.rs +++ b/src/libstd/collections/hash/map.rs @@ -564,7 +564,7 @@ impl HashMap where K: Borrow, Q: Eq + Hash { - if !self.is_empty() { + if self.is_empty() { return None; } From e034dddb32cd9814d9f71bb2b444f9863fba2dfc Mon Sep 17 00:00:00 2001 From: Shaun Steenkamp Date: Tue, 13 Feb 2018 20:25:10 +0000 Subject: [PATCH 8/8] 38880 remove unnecessary self.table.size check --- src/libstd/collections/hash/map.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/libstd/collections/hash/map.rs b/src/libstd/collections/hash/map.rs index a8f419d6c6c80..a82ff915093c6 100644 --- a/src/libstd/collections/hash/map.rs +++ b/src/libstd/collections/hash/map.rs @@ -1290,10 +1290,6 @@ impl HashMap where K: Borrow, Q: Hash + Eq { - if self.table.size() == 0 { - return None; - } - self.search_mut(k) .map(|bucket| { let (k, v, _) = pop_internal(bucket);