Fix performance impact of exception handling in _is_annotation_tid() #925
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
…ed exception (including statistics comparison with earlier code). The data shows the newly added exception handling code impact the performance most while the difference in dictionary access is insignificant so only the exception handling code is changed.
|
Thanks for the fix. It looks like it is not ideal to create and include a large string in the exception. I think this PR is fine. Please fix the CI problem and we can merge it. To avoid unnecessary turnaround due to things like "black", you could add the pre-commit-hook locally as specified here. |
Codecov Report
@@ Coverage Diff @@
## master #925 +/- ##
=======================================
Coverage 81.05% 81.05%
=======================================
Files 256 256
Lines 19851 19851
=======================================
Hits 16091 16091
Misses 3760 3760
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
ci not passed yet. |
…g-of-_is_annontation_tid()
hunterhector
approved these changes
Apr 2, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR fixes #923.
Description of changes
Related test code were added to determine details related exception (including statistics comparison with earlier code). The data shows the newly added exception handling code impact the performance most while the difference in dictionary access is insignificant so only the exception handling code is changed.
Possible influences of this PR.
Since no other place than the exception handling part is changed, the impact to other code is minimum, and the performance issue can be resolved (speed restored to previous level).
Test Conducted
Usual unit tests and profiling tests.