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.

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

perf: increase min buckets on very small types #524

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
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
69 changes: 54 additions & 15 deletions src/raw/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -192,14 +192,39 @@ impl ProbeSeq {
// Workaround for emscripten bug emscripten-core/emscripten-fastcomp#258
#[cfg_attr(target_os = "emscripten", inline(never))]
#[cfg_attr(not(target_os = "emscripten"), inline)]
fn capacity_to_buckets(cap: usize) -> Option<usize> {
fn capacity_to_buckets(cap: usize, table_layout: TableLayout) -> Option<usize> {
debug_assert_ne!(cap, 0);

// For small tables we require at least 1 empty bucket so that lookups are
// Consider a small TableLayout like { size: 1, ctrl_align: 16 } on a
// platform with Group::WIDTH of 16 (like x86_64 with SSE2). For small
// bucket sizes, this ends up wasting quite a few bytes just to pad to the
// relatively larger ctrl_align:
//
// | capacity | buckets | bytes allocated | bytes per item |
// | -------- | ------- | --------------- | -------------- |
// | 3 | 4 | 36 | (Yikes!) 12.0 |
// | 7 | 8 | 40 | (Poor) 5.7 |
// | 14 | 16 | 48 | 3.4 |
// | 28 | 32 | 80 | 3.3 |
//
// In general, buckets * table_layout.size >= table_layout.ctrl_align must
// be true to avoid these edges. This is implemented by adjusting the
// minimum capacity upwards for small items. This code only needs to handle
// ctrl_align which are less than or equal to Group::WIDTH, because valid
// layout sizes are always a multiple of the alignment, so anything with
// alignment over the Group::WIDTH won't hit this edge case.
let cap = cap.max(match (Group::WIDTH, table_layout.size) {
(16, 0..=1) => 14,
(16, 2..=3) => 7,
(8, 0..=1) => 7,
_ => 3,
});
Comment on lines +216 to +221
Copy link
Author

@morrisonlevi morrisonlevi Jun 24, 2024

Choose a reason for hiding this comment

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

It could also be written this way. Not sure what is better:

Suggested change
let cap = cap.max(match (Group::WIDTH, table_layout.size) {
(16, 0..=1) => 14,
(16, 2..=3) => 7,
(8, 0..=1) => 7,
_ => 3,
});
let cap = cap.max(match table_layout.size {
0..=1 if Group::WIDTH == 16 => 14,
2..=3 if Group::WIDTH == 16 => 7,
0..=1 if Group::WIDTH == 8 => 7,
_ => 3,
});

Copy link
Member

Choose a reason for hiding this comment

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

What I had in mind was to move the max check inside the if cap < 8 block entirely (and changing the bound to 15). Then just have hard-coded selection depending on the requested capacity and the group width/item size.


// For small tables, require at least 1 empty bucket so that lookups are
// guaranteed to terminate if an element doesn't exist in the table.
if cap < 8 {
// We don't bother with a table size of 2 buckets since that can only
// hold a single element. Instead we skip directly to a 4 bucket table
// Don't bother with a table size of 2 buckets since that can only
// hold a single element. Instead, skip directly to a 4 bucket table
// which can hold 3 elements.
return Some(if cap < 4 { 4 } else { 8 });
}
Expand Down Expand Up @@ -1126,7 +1151,7 @@ impl<T, A: Allocator> RawTable<T, A> {
// elements. If the calculation overflows then the requested bucket
// count must be larger than what we have right and nothing needs to be
// done.
let min_buckets = match capacity_to_buckets(min_size) {
let min_buckets = match capacity_to_buckets(min_size, Self::TABLE_LAYOUT) {
Some(buckets) => buckets,
None => return,
};
Expand Down Expand Up @@ -1257,14 +1282,8 @@ impl<T, A: Allocator> RawTable<T, A> {
/// * If `self.table.items != 0`, calling of this function with `capacity`
/// equal to 0 (`capacity == 0`) results in [`undefined behavior`].
///
/// * If `capacity_to_buckets(capacity) < Group::WIDTH` and
/// `self.table.items > capacity_to_buckets(capacity)`
/// calling this function results in [`undefined behavior`].
///
/// * If `capacity_to_buckets(capacity) >= Group::WIDTH` and
/// `self.table.items > capacity_to_buckets(capacity)`
/// calling this function are never return (will go into an
/// infinite loop).
/// * If `self.table.items > capacity_to_buckets(capacity, Self::TABLE_LAYOUT)`
/// calling this function are never return (will loop infinitely).
///
/// See [`RawTableInner::find_insert_slot`] for more information.
///
Expand Down Expand Up @@ -1782,8 +1801,8 @@ impl RawTableInner {
// SAFETY: We checked that we could successfully allocate the new table, and then
// initialized all control bytes with the constant `EMPTY` byte.
unsafe {
let buckets =
capacity_to_buckets(capacity).ok_or_else(|| fallibility.capacity_overflow())?;
let buckets = capacity_to_buckets(capacity, table_layout)
.ok_or_else(|| fallibility.capacity_overflow())?;

let result = Self::new_uninitialized(alloc, table_layout, buckets, fallibility)?;
// SAFETY: We checked that the table is allocated and therefore the table already has
Expand Down Expand Up @@ -4571,6 +4590,26 @@ impl<T, A: Allocator> RawExtractIf<'_, T, A> {
mod test_map {
use super::*;

#[test]
fn test_minimum_capacity_for_small_types() {
#[track_caller]
fn test_t<T>() {
let raw_table: RawTable<T> = RawTable::with_capacity(1);
let actual_buckets = raw_table.buckets();
let min_buckets = Group::WIDTH / core::mem::size_of::<T>();
assert!(
actual_buckets >= min_buckets,
"expected at least {min_buckets} buckets, got {actual_buckets} buckets"
);
}

test_t::<u8>();

// This is only "small" for some platforms, like x86_64 with SSE2, but
// there's no harm in running it on other platforms.
test_t::<u16>();
}

fn rehash_in_place<T>(table: &mut RawTable<T>, hasher: impl Fn(&T) -> u64) {
unsafe {
table.table.rehash_in_place(
Expand Down
Loading