Skip to content

[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

Merged
merged 2 commits into from
Apr 6, 2025

Conversation

miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Aug 10, 2023

(cherry picked from commit 37d8b90)

Co-authored-by: Mark Shannon mark@hotpy.org

Closes #107674

…events (pythonGH-107780)

(cherry picked from commit 37d8b90)

Co-authored-by: Mark Shannon <mark@hotpy.org>
@markshannon
Copy link
Member

@Yhg1s

@Yhg1s
Copy link
Member

Yhg1s commented Aug 11, 2023

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...

@markshannon
Copy link
Member

How important is this performance improvement?

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.

Does it solve all of the reported slowdown, or just some of it?

I don't know as yet. I'll get back to you when I have some numbers.

I don't really want pure performance improvements, especially of this complexity, in the bug fix stage, let alone between rc1 and rc2...

That's understandable.

@ambv
Copy link
Contributor

ambv commented Aug 11, 2023

Closing and re-opening to retrigger CLA checks. Sorry for the noise.

@ambv ambv closed this Aug 11, 2023
@ambv ambv reopened this Aug 11, 2023
@Yhg1s
Copy link
Member

Yhg1s commented Sep 5, 2023

How important is this performance improvement?

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.

Does it solve all of the reported slowdown, or just some of it?

I don't know as yet. I'll get back to you when I have some numbers.

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?

@markshannon
Copy link
Member

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 sys.monitoring which will give a huge speedup and not worry about the performance of sys.settrace based approaches.

So, I'd leave it for now.

@vstinner
Copy link
Member

vstinner commented Oct 4, 2023

@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.

@tarpas
Copy link

tarpas commented Oct 24, 2023

The overhead is killing the utility of https://github.com/tarpas/pytest-testmon. It would be great if it's addressed.

@serhiy-storchaka
Copy link
Member

Was this PR forgotten? Should it be merged or closed?

@hugovk
Copy link
Member

hugovk commented Aug 9, 2024

I don't think that coverage.py was ported to PEP 669 yet.

coverage.py now has support for PEP 669 / sys.monitoring for line coverage, but not yet for branch coverage: nedbat/coveragepy#1746 (comment)

Does this still need backporting to 3.12?

cc @nedbat

@patrys
Copy link

patrys commented Sep 6, 2024

I just found this PR while investigating why pytest was taking several minutes longer after upgrading to Python 3.12. While coverage.py has experimental support for sys.monitoring, it's incomplete and not enabled by default. In our case it won't help until it works with branch coverage, so we're forced to endure the slow path.

Another tool affected is pytest-testmon, which programmatically uses coverage.py as a library. I'm not even sure it supports COVERAGE_CORE=sysmon as setting it did not make a difference.

@nedbat
Copy link
Member

nedbat commented Sep 6, 2024

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.

@nedbat
Copy link
Member

nedbat commented Sep 6, 2024

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.

@nedbat
Copy link
Member

nedbat commented Sep 6, 2024

I ran some benchmarks:

cov proj python3.10 python3.11 python3.12 python3.13 3.13 vs 3.12 3.12 vs 3.11 3.11 vs 3.10
761 mashumaro 59.0s 56.9s 62.6s 69.4s 111% 110% 96%
761 pygments 26.5s 25.1s 56.4s 38.6s 68% 224% 95%
761 mypy 109.3s 105.7s 114.1s 102.2s 90% 108% 97%

(the last three columns would make more sense in reverse order).

@vstinner
Copy link
Member

What's the status of this PR created 1 year 1/2 ago?

@Yhg1s
Copy link
Member

Yhg1s commented Apr 2, 2025

@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?

Copy link
Member

@markshannon markshannon left a 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 DISABLEing 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.

@Yhg1s Yhg1s enabled auto-merge (squash) April 6, 2025 19:39
@Yhg1s Yhg1s merged commit 55b2303 into python:3.12 Apr 6, 2025
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.