-
Notifications
You must be signed in to change notification settings - Fork 154
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
Refactor the map core to its own module #125
Conversation
I've been playing with using |
FYI, I pushed hashbrown here: https://github.com/cuviper/indexmap/tree/shredded-potatoes It will raise the MSRV to 1.32 for hashbrown's sake, unless we go through added effort to make this a conditional feature. |
Really cool that it can be done, nice job!!, if the benchmark gains are significant it seems like we should use hashbrown no doubt. It also resolves our old question.. should we use |
I guess that we unfortunately need clarification on the "experimental"-ness of the RawTable, if we depend on it for indexmap 1.x, even though it doesn't affect our API surface. |
Before we get too far on hashbrown, are you OK with the changes in this PR alone?
So far it's a mixed bag, better on some, worse on others. It also seems highly sensitive to how codegen-units split, such that a seemingly insignificant tweak can cause big changes, even in the standard On a different metric, I found that the memory use (RSS) while running the benchmarks is consistently lower by quite a bit. However, the base
I'm somewhat inclined to give this a lot of weight, myself, even if some performance is lost.
@Amanieu, care to comment? |
Yes, sure. Nothing seems remotely controversial about it, unless I'm missing something (?), so it's just a very nice cleanup.
Of course, as long as we have some tangible gains, like performance, for indexmap users, too. |
I don't think you're missing anything. 🙂 It will probably conflict with #126, but I can yield and rebase if you want to merge that first. |
The |
Sure, it's fine to have API changes with proper semver treatment -- that's a pretty safe "experimental". If we convert |
These are all internal, not affecting public API.
OK, rebase is green, let's go! |
This moves the hash-agnostic
IndexMapCore
to its own module, and then cleans up the abstraction boundary in successive commits.