Skip to content
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

[3.12] GH-107674: Avoid allocating boxed ints for sys.settrace line events (GH-107780) #107841

Open
wants to merge 1 commit into
base: 3.12
Choose a base branch
from

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

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.