Skip to content

Conversation

pgdr
Copy link
Contributor

@pgdr pgdr commented Oct 20, 2025

Fixes #140358

This code includes pycore_time.h in gc.c and times the run of _PyGC_Collect. If _PyGC_DEBUG_STATS, then we print the total time used:

E.g.

gc: done, 0 unreachable, 0 uncollectable, 0.0741s elapsed

See also discussion at elapsed time debugging in gc.c at discuss.

@bedevere-app
Copy link

bedevere-app bot commented Oct 20, 2025

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@sergey-miryanov
Copy link
Contributor

Please add test to test_gc.py. You have to check that a line with your changes presented in the output.

@pgdr
Copy link
Contributor Author

pgdr commented Oct 20, 2025

I'm not sure if it is appropriate to use the performance counter or the monotonic (raw).

I currently chose monotonic raw, whereas the Python 3.13 version has performance counter.

@sergey-miryanov
Copy link
Contributor

I'm not sure if it is appropriate to use the performance counter or the monotonic (raw).

I currently chose monotonic raw, whereas the Python 3.13 version has performance counter.

IMHO it is better to use counter like in 3.13 vesion.

@sergey-miryanov sergey-miryanov changed the title gh-140358: Bring back elapsed time and unreachable count to gc debug gh-140358: Bring back elapsed time and unreachable count to gc debug output Oct 20, 2025
@pgdr
Copy link
Contributor Author

pgdr commented Oct 20, 2025

@sergey-miryanov Thanks a lot for all the comments. I believe all issues you pointed to are addressed now. A test has been added, and I included the checking of the other lines in the test as well.

I also moved to using performance counter.

@sergey-miryanov
Copy link
Contributor

sergey-miryanov commented Oct 20, 2025

You need to use the requires_gil_enabled decorator to avoid tests run on the free-threading build.

@sergey-miryanov
Copy link
Contributor

@pgdr Thanks!

@sergey-miryanov
Copy link
Contributor

@markshannon, @nascheme, @pablogsal Please take a look.

@pgdr
Copy link
Contributor Author

pgdr commented Oct 20, 2025

@efimov-mikhail I believe I have addressed all your comments, but I still have a doubt regarding including pycore_time. Is it correct as it is now without the include?

@efimov-mikhail
Copy link
Member

efimov-mikhail commented Oct 20, 2025

@efimov-mikhail I believe I have addressed all your comments, but I still have a doubt regarding including pycore_time. Is it correct as it is now without the include?

I think so. CI is green for this PR. And we don't include this file in Python/gc.c on 3.13.
So, I don't see any reason to include this file.

And PyTime_t is defined here:

typedef int64_t PyTime_t;

It's part of the public Python C API, as well as PyTime_PerfCounterRaw and PyTime_AsSecondsDouble.

Copy link
Member

@efimov-mikhail efimov-mikhail left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the PR!

But we have to wait for some Core Developers.

@nascheme
Copy link
Member

This looks okay to me.

@pablogsal pablogsal merged commit b2f9fb9 into python:main Oct 20, 2025
49 checks passed
@pablogsal
Copy link
Member

This should be probably backported. @nascheme WDYT?

@sergey-miryanov
Copy link
Contributor

@pablogsal @nascheme Thanks!

@pgdr
Copy link
Contributor Author

pgdr commented Oct 21, 2025

This should be probably backported. @nascheme WDYT?

I would be very happy if this found its way to 3.14 as it would help me debugging my main issue #139951.

@efimov-mikhail
Copy link
Member

efimov-mikhail commented Oct 21, 2025

IMO, we can consider this a bug, since removing elapsed line wasn't intended.
I'll add 3.14 label, because we can simply close the backport PR if we decided to.

@pablogsal @nascheme
I hope that's appropriate.

@efimov-mikhail efimov-mikhail added 3.14 bugs and security fixes needs backport to 3.13 bugs and security fixes labels Oct 21, 2025
@miss-islington-app
Copy link

Thanks @pgdr for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@miss-islington-app
Copy link

Sorry, @pgdr and @pablogsal, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker b2f9fb9db29296410f76009ac65cf81afa8a92a9 3.13

@efimov-mikhail efimov-mikhail added needs backport to 3.14 bugs and security fixes and removed needs backport to 3.13 bugs and security fixes labels Oct 21, 2025
@miss-islington-app
Copy link

Thanks @pgdr for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

@efimov-mikhail efimov-mikhail removed the 3.14 bugs and security fixes label Oct 21, 2025
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 21, 2025
…debug output (pythonGH-140359)

(cherry picked from commit b2f9fb9)

Co-authored-by: Pål Grønås Drange <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Oct 21, 2025

GH-140405 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Oct 21, 2025
@efimov-mikhail
Copy link
Member

Sorry, some misclicks.

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.

Bring back elapsed time debugging in gc.c

5 participants