Skip to content

Avoid hacky workaround for with_capacity #5

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

Closed
jonhoo opened this issue Jun 30, 2020 · 4 comments
Closed

Avoid hacky workaround for with_capacity #5

jonhoo opened this issue Jun 30, 2020 · 4 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed question Further information is requested

Comments

@jonhoo
Copy link
Owner

jonhoo commented Jun 30, 2020

When we have to allocate a new table, we want to continue with factor-of-two resizing when possible. Unfortunately, hashbrown::RawTable::with_capacity (I think) allocates more or less the capacity we give it, rather than rounding up. So, we resort to this workaround:

griddle/src/lib.rs

Lines 746 to 749 in c68eb4c

// We want to favor factor-of-two growing, if that's sufficient, so we don't
// use with_capacity (which allocates _exactly_ what we give).
let mut new_table = RawTable::new();
new_table.reserve(need_cap, |_| unreachable!());

It'd be nice to get rid of that. Benchmarks may tell us that with_capacity works just fine..

@jonhoo jonhoo added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers question Further information is requested labels Jun 30, 2020
@Amanieu
Copy link

Amanieu commented Jun 30, 2020

reserve and with_capacity are equivalent: they both take the capacity (number of elements you can insert before resizing) as an argument, not the number of buckets.

@Amanieu
Copy link

Amanieu commented Jun 30, 2020

They also both round up to the next power of two.

@jonhoo
Copy link
Owner Author

jonhoo commented Jun 30, 2020

Hmm, I could be wrong, but that doesn't quite seem like the case from the code? While reserve does eventually call try_with_capacity, it computes the additional capacity to request using a usize::max here, which seems to do some rounding up. Is that logic not relevant/important?

@Amanieu
Copy link

Amanieu commented Jun 30, 2020

new_items may be less than full_capacity at this point if the rehash path was reached because all of our free space is used up by tombstones. The usize::max here is to force us to use the next size in that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants