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

Refactor the map core to its own module #125

Merged
merged 12 commits into from
Jun 9, 2020

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Jun 3, 2020

This moves the hash-agnostic IndexMapCore to its own module, and then cleans up the abstraction boundary in successive commits.

@cuviper
Copy link
Member Author

cuviper commented Jun 3, 2020

I've been playing with using hashbrown's RawTable as the backend for IndexMap, and I already had this kind of refactoring as a result. I'm still not entirely satisfied with that, but I realized that the new organization could be beneficial on its own.

@cuviper
Copy link
Member Author

cuviper commented Jun 5, 2020

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.

@bluss
Copy link
Member

bluss commented Jun 5, 2020

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 unsafe to speed up indexmap? This way we outsource to a well-known and well tested implementation.

@bluss
Copy link
Member

bluss commented Jun 5, 2020

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.

@cuviper
Copy link
Member Author

cuviper commented Jun 5, 2020

Before we get too far on hashbrown, are you OK with the changes in this PR alone?

if the benchmark gains are significant it seems like we should use hashbrown no doubt.

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 HashMap comparision benchmarks -- maybe we should set codegen-units = 1?

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 IndexMapCore size will increase, because RawTable has 5 usize/pointers compared to the 3 it's replacing (mask: usize, indices: Box<[Pos]>).

This way we outsource to a well-known and well tested implementation.

I'm somewhat inclined to give this a lot of weight, myself, even if some performance is lost.

I guess that we unfortunately need clarification on the "experimental"-ness of the RawTable

@Amanieu, care to comment?

@bluss
Copy link
Member

bluss commented Jun 5, 2020

Before we get too far on hashbrown, are you OK with the changes in this PR alone?

Yes, sure. Nothing seems remotely controversial about it, unless I'm missing something (?), so it's just a very nice cleanup.

I'm somewhat inclined to give this a lot of weight, myself, even if some performance is lost.

Of course, as long as we have some tangible gains, like performance, for indexmap users, too.

@cuviper
Copy link
Member Author

cuviper commented Jun 5, 2020

Nothing seems remotely controversial about it, unless I'm missing something (?), so it's just a very nice cleanup.

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 hashbrown change will probably make that one moot, but we'll deal with that later.)

@Amanieu
Copy link

Amanieu commented Jun 5, 2020

The RawTable API is experimental in the sense that it may experience significant API changes and it could even be removed in the future if all its functionality becomes available directly on HashMap. However any such changes will be subject to a major version bump of the hashbrown crate so you don't need to worry about minor version bumps breaking the build.

@cuviper
Copy link
Member Author

cuviper commented Jun 5, 2020

Sure, it's fine to have API changes with proper semver treatment -- that's a pretty safe "experimental". If we convert indexmap, I hope you might even involve us in the design of proposed changes.

@cuviper
Copy link
Member Author

cuviper commented Jun 9, 2020

OK, rebase is green, let's go!

@cuviper cuviper merged commit 9525a63 into indexmap-rs:master Jun 9, 2020
@cuviper cuviper deleted the refactor-core branch July 23, 2020 21:26
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.

None yet

3 participants