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

fix: Only instantiate RawTable's reserve functions once #204

Merged
merged 4 commits into from
Sep 30, 2020
Merged

fix: Only instantiate RawTable's reserve functions once #204

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b7830cf
fix: Only instantiate RawTable's reserve functions once
Sep 29, 2020
63832ed
Don't instantiate get functions twice either
Sep 29, 2020
d8463d0
Address review comments
Sep 30, 2020
5e11d32
nits
Sep 30, 2020
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
73 changes: 48 additions & 25 deletions src/map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,29 @@ impl<K: Clone, V: Clone, S: Clone> Clone for HashMap<K, V, S> {
}
}

#[cfg_attr(feature = "inline-more", inline)]
pub(crate) fn make_hasher<K: Hash, V>(
hash_builder: &impl BuildHasher,
) -> impl Fn(&(K, V)) -> u64 + '_ {
move |val| make_hash(hash_builder, &val.0)
}

fn equivalent<Q, K, V>(k: &Q) -> impl Fn(&(K, V)) -> bool + '_
where
K: Borrow<Q>,
Q: ?Sized + Eq,
{
move |x| k.eq(x.0.borrow())
}

fn equivalent_single<Q, K>(k: &Q) -> impl Fn(&K) -> bool + '_
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be called equivalent and the other one should be renamed to equivalent_key.

where
K: Borrow<Q>,
Q: ?Sized + Eq,
{
move |x| k.eq(x.borrow())
}

#[cfg_attr(feature = "inline-more", inline)]
pub(crate) fn make_hash<K: Hash + ?Sized>(hash_builder: &impl BuildHasher, val: &K) -> u64 {
let mut state = hash_builder.build_hasher();
Expand Down Expand Up @@ -664,8 +687,7 @@ where
#[cfg_attr(feature = "inline-more", inline)]
pub fn reserve(&mut self, additional: usize) {
let hash_builder = &self.hash_builder;
self.table
.reserve(additional, |x| make_hash(hash_builder, &x.0));
self.table.reserve(additional, make_hasher(hash_builder));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.table.reserve(additional, make_hasher(hash_builder));
self.table.reserve(additional, make_hasher(&self.hash_builder));

The let above was only needed because the closure would otherwise capture all of self instead of just the hash_builder field.

}

/// Tries to reserve capacity for at least `additional` more elements to be inserted
Expand All @@ -688,7 +710,7 @@ where
pub fn try_reserve(&mut self, additional: usize) -> Result<(), TryReserveError> {
let hash_builder = &self.hash_builder;
self.table
.try_reserve(additional, |x| make_hash(hash_builder, &x.0))
.try_reserve(additional, make_hasher(hash_builder))
}

/// Shrinks the capacity of the map as much as possible. It will drop
Expand All @@ -710,7 +732,7 @@ where
#[cfg_attr(feature = "inline-more", inline)]
pub fn shrink_to_fit(&mut self) {
let hash_builder = &self.hash_builder;
self.table.shrink_to(0, |x| make_hash(hash_builder, &x.0));
self.table.shrink_to(0, make_hasher(hash_builder));
}

/// Shrinks the capacity of the map with a lower limit. It will drop
Expand Down Expand Up @@ -740,7 +762,7 @@ where
pub fn shrink_to(&mut self, min_capacity: usize) {
let hash_builder = &self.hash_builder;
self.table
.shrink_to(min_capacity, |x| make_hash(hash_builder, &x.0));
.shrink_to(min_capacity, make_hasher(hash_builder));
}

/// Gets the given key's corresponding entry in the map for in-place manipulation.
Expand All @@ -765,7 +787,7 @@ where
#[cfg_attr(feature = "inline-more", inline)]
pub fn entry(&mut self, key: K) -> Entry<'_, K, V, S> {
let hash = make_hash(&self.hash_builder, &key);
if let Some(elem) = self.table.find(hash, |q| q.0.eq(&key)) {
if let Some(elem) = self.table.find(hash, equivalent(&key)) {
Entry::Occupied(OccupiedEntry {
hash,
key: Some(key),
Expand Down Expand Up @@ -852,7 +874,7 @@ where
Q: Hash + Eq,
{
let hash = make_hash(&self.hash_builder, k);
self.table.get(hash, |x| k.eq(x.0.borrow()))
self.table.get(hash, equivalent(k))
}

/// Returns the key-value pair corresponding to the supplied key, with a mutable reference to value.
Expand Down Expand Up @@ -960,7 +982,7 @@ where
Q: Hash + Eq,
{
let hash = make_hash(&self.hash_builder, k);
self.table.get_mut(hash, |x| k.eq(x.0.borrow()))
self.table.get_mut(hash, equivalent(k))
}

/// Inserts a key-value pair into the map.
Expand Down Expand Up @@ -991,12 +1013,11 @@ where
#[cfg_attr(feature = "inline-more", inline)]
pub fn insert(&mut self, k: K, v: V) -> Option<V> {
let hash = make_hash(&self.hash_builder, &k);
if let Some((_, item)) = self.table.get_mut(hash, |x| k.eq(&x.0)) {
if let Some((_, item)) = self.table.get_mut(hash, equivalent(&k)) {
Some(mem::replace(item, v))
} else {
let hash_builder = &self.hash_builder;
self.table
.insert(hash, (k, v), |x| make_hash(hash_builder, &x.0));
self.table.insert(hash, (k, v), make_hasher(hash_builder));
None
}
}
Expand Down Expand Up @@ -1061,7 +1082,7 @@ where
Q: Hash + Eq,
{
let hash = make_hash(&self.hash_builder, &k);
self.table.remove_entry(hash, |x| k.eq(x.0.borrow()))
self.table.remove_entry(hash, equivalent(k))
}
}

Expand Down Expand Up @@ -1528,7 +1549,7 @@ impl<'a, K, V, S> RawEntryBuilderMut<'a, K, V, S> {
K: Borrow<Q>,
Q: Eq,
{
self.from_hash(hash, |q| q.borrow().eq(k))
self.from_hash(hash, equivalent_single(k))
}
}

Expand Down Expand Up @@ -1585,7 +1606,7 @@ impl<'a, K, V, S> RawEntryBuilder<'a, K, V, S> {
K: Borrow<Q>,
Q: Eq,
{
self.from_hash(hash, |q| q.borrow().eq(k))
self.from_hash(hash, equivalent_single(k))
}

#[cfg_attr(feature = "inline-more", inline)]
Expand Down Expand Up @@ -1946,7 +1967,10 @@ impl<'a, K, V, S> RawVacantEntryMut<'a, K, V, S> {
S: BuildHasher,
{
let hash_builder = self.hash_builder;
self.insert_with_hasher(hash, key, value, |k| make_hash(hash_builder, k))
let &mut (ref mut k, ref mut v) =
self.table
.insert_entry(hash, (key, value), make_hasher(hash_builder));
(k, v)
}

/// Set the value of an entry with a custom hasher function.
Expand Down Expand Up @@ -1977,9 +2001,9 @@ impl<'a, K, V, S> RawVacantEntryMut<'a, K, V, S> {
let mut hasher = self.hash_builder.build_hasher();
key.hash(&mut hasher);

let elem = self.table.insert(hasher.finish(), (key, value), |k| {
make_hash(hash_builder, &k.0)
});
let elem = self
.table
.insert(hasher.finish(), (key, value), make_hasher(hash_builder));
RawOccupiedEntryMut {
elem,
table: self.table,
Expand Down Expand Up @@ -2974,9 +2998,7 @@ impl<'a, K, V, S> VacantEntry<'a, K, V, S> {
{
let hash_builder = &self.table.hash_builder;
let table = &mut self.table.table;
let entry = table.insert_entry(self.hash, (self.key, value), |x| {
make_hash(hash_builder, &x.0)
});
let entry = table.insert_entry(self.hash, (self.key, value), make_hasher(hash_builder));
&mut entry.1
}

Expand All @@ -2987,9 +3009,10 @@ impl<'a, K, V, S> VacantEntry<'a, K, V, S> {
S: BuildHasher,
{
let hash_builder = &self.table.hash_builder;
let elem = self.table.table.insert(self.hash, (self.key, value), |x| {
make_hash(hash_builder, &x.0)
});
let elem = self
.table
.table
.insert(self.hash, (self.key, value), make_hasher(hash_builder));
OccupiedEntry {
hash: self.hash,
key: None,
Expand Down Expand Up @@ -4470,7 +4493,7 @@ mod test_map {
assert!(removed.contains(&(i, 2 * i)), "{} not in {:?}", i, removed);
let e = m
.table
.insert(hash, (i, 2 * i), |x| super::make_hash(&hasher, &x.0));
.insert(hash, (i, 2 * i), super::make_hasher(&hasher));
it.reflect_insert(&e);
if let Some(p) = removed.iter().position(|e| e == &(i, 2 * i)) {
removed.swap_remove(p);
Expand Down