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

Use hashbrown::HashTable instead of RawTable #343

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Aug 30, 2024

No description provided.

@cuviper
Copy link
Member Author

cuviper commented Aug 30, 2024

I previously considered this in #280, and it didn't seem beneficial at the time, but now hashbrown has removed the "raw" feature for its upcoming 0.15. So we'll either have to make the move, or stay on 0.14 and possibly think about forking.

On the idea of forking -- I wonder if much could be gained by hard-coding that we always use RawTable<usize>, e.g. we never have to worry about dropping items. We could also get fancy and use u32 (or even u16) when the capacity is low enough, reducing memory usage -- as the pre-hashbrown version of this crate did!

@cuviper
Copy link
Member Author

cuviper commented Sep 7, 2024

I wonder if much could be gained by hard-coding that we always use RawTable<usize>,

I tested this in rustc, but it didn't show any noticeable change in compiler performance: rust-lang/rust#129965

@clarfonthey
Copy link

Just popping in to say that since I'm going to be working a lot on moving over hashbrown to use the HashTable API internally and completely get rid of the RawTable API, and would definitely be paying attention to any feedback you have if you shared it here: rust-lang/hashbrown#545

The raw table API is very messy and was designed before MaybeUninit existed, which is why we wanted to redo it.

Comment on lines +40 to +41
/// When using `HashTable::find_entry`, that takes hold of `&mut indices`, so we have to borrow our
/// `&mut entries` separately, and there's no way to go back to a `&mut IndexMapCore`. So this type
Copy link
Member Author

Choose a reason for hiding this comment

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

@clarfonthey Just to highlight, this comment is the part that I find most unfortunate about the change: it makes our OccupiedEntry larger to carry separate &mut references, and also makes the core methods more awkward to call from the entry. This new RefMut recovers some of the ergonomics, but I don't know that it's possible to avoid the size increase at all.

Before, raw::Bucket allowed me to store that with the full &mut IndexMapCore in the entry.

Choose a reason for hiding this comment

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

Hmm, yeah, I should add that one thing I definitely want to improve is all of the wasted space in a lot of the internal types.

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 1, 2024
Test indexmap#343 - using hashbrown 0.15 with HashTable

In indexmap-rs/indexmap#343, I've ported to hashbrown 0.15, which also requires a change from `RawTable` (now private) to `HashTable`. This PR is patching to use that unpublished branch so I can test rustc performance.

r? ghost
@cuviper cuviper added this pull request to the merge queue Oct 1, 2024
Merged via the queue into indexmap-rs:master with commit 7f80229 Oct 1, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants