-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
[3.12] GH-107674: Avoid allocating boxed ints for sys.settrace
line events (GH-107780)
#107841
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
Conversation
…events (pythonGH-107780) (cherry picked from commit 37d8b90) Co-authored-by: Mark Shannon <mark@hotpy.org>
How important is this performance improvement? Does it solve all of the reported slowdown, or just some of it? I don't really want pure performance improvements, especially of this complexity, in the bug fix stage, let alone between rc1 and rc2... |
It largely depends on whether coverage.py moves to using PEP 669 for 3.12. It is does, then this is not important at all.
I don't know as yet. I'll get back to you when I have some numbers.
That's understandable. |
Closing and re-opening to retrigger CLA checks. Sorry for the noise. |
Where are we at with this? Does it make sense to try and get this in now, or are you comfortable making these changes in 3.12.1 if they turn out to be necessary? |
It definitely helps performance of ``sys.settrace`, but performance is not what it was for 3.11. TBH, I think the best way to forward is to help coverage.py move to using So, I'd leave it for now. |
@markshannon @Yhg1s: What's the status of this change? Python 3.12.0 was released without this optimization. I don't think that coverage.py was ported to PEP 669 yet. |
The overhead is killing the utility of https://github.com/tarpas/pytest-testmon. It would be great if it's addressed. |
Was this PR forgotten? Should it be merged or closed? |
coverage.py now has support for PEP 669 / Does this still need backporting to 3.12? cc @nedbat |
I just found this PR while investigating why Another tool affected is |
Definitely coverage.py still needs sys.settrace to work well for all versions of Python. We are working on sys.monitoring support, but have not found a solution for branch coverage. |
I haven't heard people complaining about the speed on 3.12, but looking at my own coverage runs, I can see a definitely slowdown from 3.11 to 3.12. |
I ran some benchmarks:
(the last three columns would make more sense in reverse order). |
What's the status of this PR created 1 year 1/2 ago? |
@markshannon I received some questions about whether this can get applied to 3.12 (because people are still upgrading to 3.12 even though it's nearing the end of its bugfix state). What do you think? |
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.
Boxing and unboxing ints is expensive, so avoiding the box/unbox pair is definitely going to help sys.settrace
.
This is going to slow down other users of LINE
events a tiny bit, but those should be DISABLE
ing events anyway if they care about performance.
We'll fix this properly for 3.15 with tagged ints, but this fix looks like a worthwhile change in the meantime.
(cherry picked from commit 37d8b90)
Co-authored-by: Mark Shannon mark@hotpy.org
Closes #107674