GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-153568: Pause the GC while converting the AST to Python objects by pablogsal · Pull Request #153583 · python/cpython Conversation
|
| build | time per run | speedup |
|---|---|---|
| main | 133.6 ms | |
| this PR | 120.5 ms | 1.11x faster |
Executed instructions (stable under machine load, perf stat) drop by 5.6%.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| // so a garbage collection while building them cannot free anything of | ||
| // this tree and only pays to traverse its half-built nodes. Pause the | ||
| // collector while the conversion runs. | ||
| int gc_was_enabled = PyGC_Disable(); |
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
My, probably naive, question here is what about free-threading?
There seems to be a single global (per interpret) state:
cpython/Python/gc_free_threading.c
Lines 2527 to 2539 in 9d231cb
| int | |
| PyGC_Enable(void) | |
| { | |
| GCState *gcstate = get_gc_state(); | |
| return _Py_atomic_exchange_int(&gcstate->enabled, 1); | |
| } | |
| int | |
| PyGC_Disable(void) | |
| { | |
| GCState *gcstate = get_gc_state(); | |
| return _Py_atomic_exchange_int(&gcstate->enabled, 0); | |
| } |
cpython/Python/gc_free_threading.c
Lines 1656 to 1661 in 9d231cb
| static GCState * | |
| get_gc_state(void) | |
| { | |
| PyInterpreterState *interp = _PyInterpreterState_GET(); | |
| return &interp->gc; | |
| } |
For example: a thread disabled GC (in the window between here) but the parser would enable it again?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Yes this will always be racy because is currently not possible to know if this was us or someone else doing it. This is something we need to consider when reviewing this optimization
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is a more general problem?
Quick ripgrep reveals:
cpython/Modules/_posixsubprocess.c
Lines 1063 to 1067 in 1b9fe5c
| /* We need to call gc.disable() when we'll be calling preexec_fn */ | |
| if (preexec_fn != Py_None) { | |
| need_to_reenable_gc = PyGC_Disable(); | |
| } | |
cpython/Modules/_posixsubprocess.c
Lines 1326 to 1329 in 1b9fe5c
| if (need_to_reenable_gc) { | |
| PyGC_Enable(); | |
| } |
In reverse, maybe even more places would gain from disabling changing the default GC behaviour (disabling it?).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
7a56747 to
09a5678
Compare
Freshly converted AST nodes cannot be part of a reference cycle yet, so collections during the conversion only pay to traverse the half-built tree without ever freeing any of it.
7b2600f to
e00fbc2
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.