gh-115103: Update gc.collect to process delayed objects #116238
Conversation
corona10
commented
Mar 2, 2024
•
corona10
commented
- Issue: Add delayed reclamation mechanism for free-threaded build (QSBR) #115103
|
!buildbot nogil |
|
🤖 New build scheduled with the buildbot fleet by @corona10 for commit c28c79f 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
|
!buildbot nogil |
|
🤖 New build scheduled with the buildbot fleet by @corona10 for commit 81f333a 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
There was a problem hiding this comment.
Let's do this as part of the normal gc.collect() instead of adding a separate call. We want gc.collect() to be the go-to way to ensuring that things that should be freed are freed -- we already empty freelists and merge refcounts and will probably have a few more similar tasks in the near future (like collecting shared dictionary keys).
|
Yeah more better! |
|
@pablogsal Do you have any comments? |
| @@ -1006,6 +1023,7 @@ gc_collect_internal(PyInterpreterState *interp, struct collection_state *state) | |||
| _PyEval_StopTheWorld(interp); | |||
| // merge refcounts for all queued objects | |||
| merge_all_queued_objects(interp, state); | |||
| process_delayed_frees(interp); | |||
There was a problem hiding this comment.
Nit/question: _PyEval_StopTheWorld also locks _PyRuntime. Maybe is worth moving the lock up?
There was a problem hiding this comment.
_PyEval_StopTheWorld locks _PyRuntime temporarily to loop over threads, but does not hold the lock
We want process_delayed_frees after the stop-the-world call because it guarantees that we can free all the "delayed free" pointers. If we moved it up before the _PyEval_StopTheWorld there might be some memory blocks we don't free.
There was a problem hiding this comment.
Makes sense, thanks for clarifying
There was a problem hiding this comment.
If we moved it up before the
_PyEval_StopTheWorldthere might be some memory blocks we don't free.
With "moving the lock up" i meant to have a unique locking for both functions, not to move this call before the other. But your comment still applies
|
Nah I forgot the change the PR title :( anyway, I merged. |
corona10
changed the title
|
I left the comment about commit title for the future tracker: #115103 (comment) |
…cessDelayed (pythongh-116238)" This reverts commit 2e91578.
|
I tried to fix the commit title for tracking with the double revert technique but it doesn't work for git blame: d1fd060 Let's forget about it :( |
…gh-116251) * Revert "pythongh-115103: Update refleak checker to trigger _PyMem_ProcessDelayed (pythongh-116238)" This reverts commit 2e91578. * pythongh-115103: Update gc.collect to process delayed objects
…gh-116251) * Revert "pythongh-115103: Update refleak checker to trigger _PyMem_ProcessDelayed (pythongh-116238)" This reverts commit 2e91578. * pythongh-115103: Update gc.collect to process delayed objects
…gh-116251) * Revert "pythongh-115103: Update refleak checker to trigger _PyMem_ProcessDelayed (pythongh-116238)" This reverts commit 2e91578. * pythongh-115103: Update gc.collect to process delayed objects
