-
Notifications
You must be signed in to change notification settings - Fork 145
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
Improve performance of custom-iterator __getitem__
#1096
Merged
Merged
Conversation
This file contains 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
Flipping the order of `Slice` and `Int` in `SliceOrInt` so that `Int` comes first means that the `FromPyObject` derivation will then try `Int` first, which is the correct variant in like 99.9% of uses of the struct. This has the impact of improving int `__getitem__` times in the custom iterators by about 3x (from 205ns to 61ns on my machine), which has knock-on effects for the implicit iterators Python defines for these classes.
65a4b71
to
7a49961
Compare
Pull Request Test Coverage Report for Build 7978469504Details
💛 - Coveralls |
mtreinish
approved these changes
Feb 20, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for finding this and fixing it
mergify bot
pushed a commit
that referenced
this pull request
Feb 20, 2024
Flipping the order of `Slice` and `Int` in `SliceOrInt` so that `Int` comes first means that the `FromPyObject` derivation will then try `Int` first, which is the correct variant in like 99.9% of uses of the struct. This has the impact of improving int `__getitem__` times in the custom iterators by about 3x (from 205ns to 61ns on my machine), which has knock-on effects for the implicit iterators Python defines for these classes. (cherry picked from commit d4f28e3)
@Mergifyio backport stable/0.14 |
✅ Backports have been created
|
mergify bot
pushed a commit
that referenced
this pull request
Feb 20, 2024
Flipping the order of `Slice` and `Int` in `SliceOrInt` so that `Int` comes first means that the `FromPyObject` derivation will then try `Int` first, which is the correct variant in like 99.9% of uses of the struct. This has the impact of improving int `__getitem__` times in the custom iterators by about 3x (from 205ns to 61ns on my machine), which has knock-on effects for the implicit iterators Python defines for these classes. (cherry picked from commit d4f28e3)
Thanks for writing this! I had a feeling we’d need to profile the function but I guess the fix was easier than we thought |
mtreinish
pushed a commit
that referenced
this pull request
Feb 21, 2024
Flipping the order of `Slice` and `Int` in `SliceOrInt` so that `Int` comes first means that the `FromPyObject` derivation will then try `Int` first, which is the correct variant in like 99.9% of uses of the struct. This has the impact of improving int `__getitem__` times in the custom iterators by about 3x (from 205ns to 61ns on my machine), which has knock-on effects for the implicit iterators Python defines for these classes. (cherry picked from commit d4f28e3) Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
mtreinish
added a commit
to mtreinish/retworkx
that referenced
this pull request
Feb 22, 2024
This commit prepares for a 0.14.1 release which just includes Qiskit#1109 to fix ppc64le builds and Qiskit#1096 to fix the overhead of `__getitem__` on custom sequence return types.
Merged
mtreinish
added a commit
that referenced
this pull request
Feb 22, 2024
* Prepare 0.14.1 release This commit prepares for a 0.14.1 release which just includes #1109 to fix ppc64le builds and #1096 to fix the overhead of `__getitem__` on custom sequence return types. * Update releasenotes/notes/prepare-0.14.1-e5065553a44eb035.yaml Co-authored-by: Ivan Carvalho <8753214+IvanIsCoding@users.noreply.github.com> * Update version in docs config too --------- Co-authored-by: Ivan Carvalho <8753214+IvanIsCoding@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
stable-backport-potential
This PR or issue is potentially worth backporting for inclusion in a stable branch
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.
Flipping the order of
Slice
andInt
inSliceOrInt
so thatInt
comes first means that theFromPyObject
derivation will then tryInt
first, which is the correct variant in like 99.9% of uses of the struct. This has the impact of improving int__getitem__
times in the custom iterators by about 3x (from 205ns to 61ns on my machine), which has knock-on effects for the implicit iterators Python defines for these classes.This implements the performance improvement I mentioned in #1090 (comment). I can also look into defining manual Python-space iterators (rather than using the implicit one based on
__getitem__
), if that's an interesting path - I haven't tried it yet, so I don't know if there's more performance to be had.