Skip to content

gh-107674: Remove some unnecessary code in instrumentation code #117393

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

Merged

Conversation

gaogaotiantian
Copy link
Member

@gaogaotiantian gaogaotiantian commented Mar 30, 2024

This might not be super helpful but it's free and easy to justify. Also it makes the code cleaner, not messier.

  • line >= 0 was already asserted above so the check is unnecessary
  • Py_None is immortal now so Py_DECREF is a no-op.

@bedevere-app
Copy link

bedevere-app bot commented Apr 1, 2024

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

FWIW, I'm unclear about the merit of this PR relative to addressing the performance regression identified in gh-107674. However, the change itself is correct and not a problem, so I don't see any reason not to merge it.

@gaogaotiantian
Copy link
Member Author

The changes (including the one in sysmodule.c that we decided to postpone) were all made when I was investigating the performance regression. They may not contribute significantly to the final result, but that is the direct trigger we made the change. I think linking that issue might give the people looking at the change an idea about why this change was made at the time.

Also, even though it might not be significant or even observable, this strictly improves the performance by removing an unnecessary comparison, so the gh issue is not that irrelevant :)

@gaogaotiantian
Copy link
Member Author

Considering the simplicity of this PR, maybe we can merge it? Not sure if @markshannon is busy recently.

@markshannon markshannon merged commit 5718324 into python:main Apr 9, 2024
35 of 36 checks passed
@gaogaotiantian gaogaotiantian deleted the remove-instrumentation-redundancy branch May 3, 2024 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants