Repository navigation
gh-159437: Skip unnecessary GC finalizer scans and align functions - #159587
mfalco-oai wants to merge 2 commits into
Conversation
Sorry, something went wrong.
|
All commit authors signed the Contributor License Agreement. |
Sorry, something went wrong.
There was a problem hiding this comment.
The callback refactor seems bigger than skipping this scan needs. There is a new helper with a long argument list and the caller now has to choose between different garbage lists.
A related change I made in an earlier PR already tracks whether Python may have run. I tried extending that flow by recording pending finalizers during the weakref scan. That was a much smaller change in gc.c and kept the existing callback handling.
Let's keep the existing callback and resurrection flow unless there is a separate application benefit from the larger refactor.
Let's see what @nascheme thinks about the callback refactor too.
Sorry, something went wrong.
| return len(text) | ||
|
|
||
| writer = Stderr() | ||
| gc.collect() |
There was a problem hiding this comment.
This fails on its own with -X lazy_imports=all. I tried it with and without the PR and hit the collected object count assertion in each case. Binding patch_stderr = unittest.mock.patch.object before the initial collection fixed the failure. We should do that before this cleanup and use the bound function below.
Sorry, something went wrong.
I agree that the handle_callbacks() is a code style to be avoided. E.g. the argument list for the function is almost the same size as the body. Something wrong there. It's also only called once. So I'd say it should rather just be open-coded at the call site. The idea of this optimization good. I'd suggest looking at Pablo's earlier PR and see if a more minimal code change can achieve the same thing. The 64-bit alignment change really should be a separate PR. Imagine we want to revert one thing or the other. Should not be combined. |
Sorry, something went wrong.
|
@pablogsal is your PR this one: #160464 ? Your link goes to a blob so it's a bit hard to find the commit that made it. |
Sorry, something went wrong.
With this optimization the GIL collector records pending finalizers while preparing weakref callbacks. If neither finalizers nor callbacks are pending, it skips the finalizer scan. Callbacks and DEBUG_COLLECTABLE keep the full path because they can run Python.
The build also requests 64-byte function alignment for GIL and free-threaded non-debug builds on Linux (excluding Android) and macOS when the compiler supports it, except when optimizing for size. Small collector changes have moved otherwise unchanged traversal functions to different offsets within cache lines and this had negatively impacted performance. Aligning function entry points should make performance less dependent on those offsets.
Performance
gc_collect_self_lists times full collection of one million self-referential lists. Before is upstream; after is this PR. Both builds use -falign-functions=64.
Benchmarks: python/pyperformance#496