Skip to content

Make RawTable::insert_no_grow unsafe #254

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

Merged
merged 1 commit into from
Mar 30, 2021
Merged

Make RawTable::insert_no_grow unsafe #254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 9 additions & 11 deletions src/raw/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -872,19 +872,17 @@ impl<T, A: Allocator + Clone> RawTable<T, A> {
/// This does not check if the given element already exists in the table.
#[cfg_attr(feature = "inline-more", inline)]
#[cfg(any(feature = "raw", feature = "rustc-internal-api"))]
pub fn insert_no_grow(&mut self, hash: u64, value: T) -> Bucket<T> {
unsafe {
let (index, old_ctrl) = self.table.prepare_insert_slot(hash);
let bucket = self.table.bucket(index);
pub unsafe fn insert_no_grow(&mut self, hash: u64, value: T) -> Bucket<T> {
let (index, old_ctrl) = self.table.prepare_insert_slot(hash);
let bucket = self.table.bucket(index);

// If we are replacing a DELETED entry then we don't need to update
// the load counter.
self.table.growth_left -= special_is_empty(old_ctrl) as usize;
// If we are replacing a DELETED entry then we don't need to update
// the load counter.
self.table.growth_left -= special_is_empty(old_ctrl) as usize;
Copy link
Member

Choose a reason for hiding this comment

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

Since this function is already unsafe, we could add an extra assume_no_deleted argument which assumes that the table is fresh and does not contain any DELETED entries? This would skip the check on old_ctrl and might improve performance in some cases.

However it might be exposing too much of the hash table internals. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

So assume_no_deleted would still be a condition there, but one that you would hope is constant-propagated?

I think it's too much of a micro-optimization, especially for exposing internals. I ran some indexmap benchmarks that are heavy on rebuild_hash_table, which is a clear followed by a loop of insert_no_grow. About 50% of that function's perf profile is simply the bucket.write, mostly due to cache misses. Most of the remaining time is spread around finding the insert slot, and less than 4% on updating growth_left. I even tried hard-coding that to growth_left -= 1, but the difference was lost in the noise.


bucket.write(value);
self.table.items += 1;
bucket
}
bucket.write(value);
self.table.items += 1;
bucket
}

/// Temporary removes a bucket, applying the given function to the removed
Expand Down
8 changes: 5 additions & 3 deletions src/rustc_entry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,8 +574,10 @@ impl<'a, K, V, A: Allocator + Clone> RustcVacantEntry<'a, K, V, A> {
/// ```
#[cfg_attr(feature = "inline-more", inline)]
pub fn insert(self, value: V) -> &'a mut V {
let bucket = self.table.insert_no_grow(self.hash, (self.key, value));
unsafe { &mut bucket.as_mut().1 }
unsafe {
let bucket = self.table.insert_no_grow(self.hash, (self.key, value));
&mut bucket.as_mut().1
}
}

/// Sets the value of the entry with the RustcVacantEntry's key,
Expand All @@ -596,7 +598,7 @@ impl<'a, K, V, A: Allocator + Clone> RustcVacantEntry<'a, K, V, A> {
/// ```
#[cfg_attr(feature = "inline-more", inline)]
pub fn insert_entry(self, value: V) -> RustcOccupiedEntry<'a, K, V, A> {
let bucket = self.table.insert_no_grow(self.hash, (self.key, value));
let bucket = unsafe { self.table.insert_no_grow(self.hash, (self.key, value)) };
RustcOccupiedEntry {
key: None,
elem: bucket,
Expand Down