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.13] GH-124567: Revert the Incremental GC in 3.13 #124770

Merged
merged 16 commits into from
Sep 30, 2024

Conversation

Yhg1s
Copy link
Member

@Yhg1s Yhg1s commented Sep 29, 2024

Revert the Incremental GC in 3.13.

This reverts PRs #116206 (commit 1530932), #117120 (e28477f), #117213 (8bef34f), #117422 (ddf814d), #118313 (2ba1aed), #123268 (b1372e2) and #123395 (aca6511). Does not revert the raising of the first generation threshold from 700 to 2000, nor changes to C APIs (even private ones).


📚 Documentation preview 📚: https://cpython-previews--124770.org.readthedocs.build/

@Yhg1s
Copy link
Member Author

Yhg1s commented Sep 29, 2024

This is a fairly simple revert, we'll have to address the what's new docs and news entries but I want to get this out there for review and testing in the meantime.

@AlexWaygood
Copy link
Member

AlexWaygood commented Sep 29, 2024

Happy to report that this cuts around 50% off the time it takes to do an HTML build of the CPython docs locally with Sphinx (build options were --with-lto=fat --enable-optimizations). On the tip of the 3.13 branch, the second script I posted in #124567 (comment) reports a Sphinx docs-build time of 66.28s for me locally; with this PR, it reports 34.38s.

@nascheme
Copy link
Member

nascheme commented Sep 29, 2024

A little benchmarking from my desktop PC. "Sphinx" is build/html for typing.rst. The debug column is for the --with-debug build. The opt column is for optimized build without LTO or PGO. The "mypy" runs are for the mypy testsuite after disabling the gc.set_threshold() call it does (it sets a much higher threshold than default one). The mypy run creates quite a big heap but not much cyclic garbage. The Sphinx run is a smaller heap but quite a lot of cyclic garbage with smallish objects.

Based on this limited testing, it's looking pretty good.

Description debug [sec] opt [sec] peak rss [MB]
Sphinx, 3.12 5.17 1.71 88.1
Sphinx, 3.13 inc 10.25 1.92 96.9
Sphinx, 3.13 revert 6.03 1.87 96.9
Mypy, 3.12   25.54 274.4
Mypy, 3.13 inc   28.28 276.2
Mypy, 3.13 revert   25.51 272.4

Modules/gcmodule.c Outdated Show resolved Hide resolved
@AlexWaygood
Copy link
Member

AlexWaygood commented Sep 29, 2024

We can also see that the Doctest CI job on this PR took only 4 minutes, as opposed to e.g. 16-17 minutes on other recent 3.13 PRs such as

Copy link
Member

@nascheme nascheme left a comment

Choose a reason for hiding this comment

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

LGTM. I compared gc.c from before GH-108362 (d5ebf8b) and it looks like your reversion got the right bits.

@Yhg1s
Copy link
Member Author

Yhg1s commented Sep 30, 2024

I believe the ABI changes are acceptable (the size of a private struct within another private struct changes, so later fields in the outer struct move around -- but as far as I can tell those structs are not exposed to users). The hypothesis failure seems unrelated (but possibly worth investigating).

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.

One (unimportant) whitespace regression, otherwise this looks correct to me.

Doc/library/gc.rst Outdated Show resolved Hide resolved
@pablogsal
Copy link
Member

pablogsal commented Sep 30, 2024

I believe the ABI changes are acceptable (the size of a private struct within another private struct changes, so later fields in the outer struct move around -- but as far as I can tell those structs are not exposed to users). The hypothesis failure seems unrelated (but possibly worth investigating).

I checked the ABI changes and they are safe as this just complains about the size change of _gc_runtime_state. This won't affect wheels or anything using the ABI and for profilers the offsets have changed but this is not a problem if they are using the new offsets fields here:

uint64_t collecting;

This may affect still some profilers that are vendoring headers like Austin (https://github.com/P403n1x87/austin/blob/4aead107f10e55a834b1f64a46ebc97fdb70f238/src/python/gc.h) but that is impossible to avoid (also here we were lucky because they haven't released wheels for 3.13 yet!).

@AA-Turner
Copy link
Member

AA-Turner@d7acbbd resolves What's New by just removing all references to the incremental GC.

I'm not sure there's much value discussing that the incremental GC was in pre-releases and was then reverted in What's New as it will serve to confuse the majority of readers, the detail should be in the NEWS entry.

A

Doc/whatsnew/3.13.rst Outdated Show resolved Hide resolved
@Yhg1s Yhg1s requested a review from encukou as a code owner September 30, 2024 20:50
@Yhg1s Yhg1s enabled auto-merge (squash) September 30, 2024 21:07
@Yhg1s Yhg1s merged commit e0eb44a into python:3.13 Sep 30, 2024
35 of 36 checks passed
@ned-deily
Copy link
Member

FWIW, a negative performance impact of the revert to some microbenchmarks: #124567 (comment)

hugovk pushed a commit to hugovk/cpython that referenced this pull request Nov 1, 2024
)

Revert the incremental GC in 3.13, since it's not clear that without further turning, the benefits outweigh the costs.

Co-authored-by: Adam Turner <[email protected]>
hugovk added a commit to hugovk/cpython that referenced this pull request Nov 4, 2024
Revert the incremental GC in 3.14, since it's not clear that without further turning, the benefits outweigh the costs.

Co-authored-by: Adam Turner <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
hugovk added a commit to hugovk/cpython that referenced this pull request Nov 11, 2024
Revert the incremental GC in 3.14, since it's not clear that without further turning, the benefits outweigh the costs.

Co-authored-by: Adam Turner <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
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.

8 participants