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

Open
wants to merge 12 commits into
base: 3.13
Choose a base branch
from

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

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.

3 participants