Use a single type alias for indexmap and indexset to enforce hasher#16130
Open
mtreinish wants to merge 8 commits intoQiskit:mainfrom
Open
Use a single type alias for indexmap and indexset to enforce hasher#16130mtreinish wants to merge 8 commits intoQiskit:mainfrom
mtreinish wants to merge 8 commits intoQiskit:mainfrom
Conversation
This commit switches our use of ahash as the hashing algorithm for IndexMap to use foldhash instead. This mirrors the change that was done in hashbrown for the default hashing algorithm in its hashmap. Foldhash promises faster hashing than ahash with similar quality tradeoffs. Since IndexSet/IndexMap defaults to using the stdlib SipHash algorithm which is not good for performance we need to set a custom hash algorithm to maintain good performance. Since we use an IndexSet for the interners saving a small amount of time per lookup here is especially important. The only place that ahash is retained is for DAGNode in Python. The usage was specific to how ahash worked and switching it to an analogous foldhash interface caused test failures. As the performance is not critical for this code path as it's only used by the Python API for accessing the DAG and it's already not a performant path saving nanoseconds on hashing doesn't matter. There is probably an alternative API in foldhash we could use, but it didn't seem critical to figure out.
Building off of Qiskit#15920 which moved our hasher for IndexMap usage to be foldhash this moves to having a common type alias for IndexMap and IndexSet that has the hasher set. All the internal usage of IndexMap and IndexSet are updated to use this new alias instead which removes the boiler plate around dealing with initializing the hasher for the most part, the only exception is when using `with_capacity_and_hasher()` there is no method to create an empty object with a capacity preallocated and the hasher is initialized from the generic type. To enforce that we're using the correct hasher for all indexmap usage the clippy rules are updated to disallow the indexmap::IndexMap and indexmap::IndexSet types directly. This means that after this commit all indexmap usage with be forced by clippy to leverage our standard pattern using foldhash. The advantage is if we ever need to change the default hasher again in the future we only need to update one central place (and the places using with_capacity_and_hasher).
mtreinish
added
on hold
Collaborator
|
One or more of the following people are relevant to this code:
|
Coverage Report for CI Build 25329395775Coverage increased (+0.02%) to 87.576%Details
Uncovered Changes
Coverage Regressions4 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Building off of #15920 which moved our hasher for IndexMap usage to be
foldhash this moves to having a common type alias for IndexMap and
IndexSet that has the hasher set. All the internal usage of IndexMap and
IndexSet are updated to use this new alias instead which removes the
boiler plate around dealing with initializing the hasher for the most
part, the only exception is when using
with_capacity_and_hasher()there is no method to create an empty object with a capacity
preallocated and the hasher is initialized from the generic type. To
enforce that we're using the correct hasher for all indexmap usage the
clippy rules are updated to disallow the indexmap::IndexMap and
indexmap::IndexSet types directly. This means that after this commit all
indexmap usage with be forced by clippy to leverage our standard pattern
using foldhash. The advantage is if we ever need to change the default
hasher again in the future we only need to update one central place
(and the places using with_capacity_and_hasher).
This PR is based on top of #15920 and will need to be rebased after that is merged. In the meantime you can view the contents of just this PR by looking at the PR branch's HEAD commit:
12f7ed5
AI/LLM disclosure