bpo-28254: Add a C-API for controlling the state of the garbage collector #25687
Conversation
|
Looks good, I will review it in more detail today, but please, add an entry to the what's new document for 3.10 in the meantime. |
…ions to make it easy for users to ('atomically') know whether they actually changed something and what state to go back to later. Also, returning an "int" may make it easier to add error handling later, if that becomes needed at some point.
|
Thanks for the PR @scoder! Make sure to merge it before end of the week 😉 |
|
I changed the functions to return the previous state. That's a common idiom in C, and I think a useful one in this case. |
| int old_state; | ||
|
|
||
| old_state = PyGC_Enable(); | ||
| msg = "Enable(1)"; |
There was a problem hiding this comment.
Bugs are unlikely. IMO using assert() is good enough for _testcapi tests. You can leave the code as it is ;-)
See for example test_refcount_funcs() which uses assert().
There was a problem hiding this comment.
I was thinking that assertions can be compiled out, which is decidedly not intended here.
There was a problem hiding this comment.
In general yes, but _testcapimodule.c starts with:
/* Always enable assertions */
#undef NDEBUG
There was a problem hiding this comment.
Good to know! I'd keep the test as it is though. It doesn't really hurt, I think.
|
Are the C functions prototypes defined in the right place? Limited API, header file hierarchy, etc.? |
… gc.collect() implementation).
…othing if GC is disabled.
You added them to the stable ABI and so you should run "regen-limited-abi". |
That added more than the three. I only committed the ones I added here in order to keep the PR clean. |
| If the garbage collector is disabled or already running, | ||
| returns ``0`` immediately. | ||
| Errors during garbage collection are ignored and printed. | ||
| This function does not raise exceptions. |
There was a problem hiding this comment.
@pablogsal: Do we want to document that it's safe to call this function with an exception raised, and that the exception is saved and then restored?
Doc/c-api/gcsupport.rst
Outdated
| .. c:function:: Py_ssize_t PyGC_Collect(void) | ||
|
|
||
| Performs a garbage collection, if the garbage collector is enabled. | ||
| Returns the number of collected + uncollectable objects. |
There was a problem hiding this comment.
Hum, uncollectable objects are stored in gc.garbage no? Shoud we document that?
The C code uses "unreachable objects that couldn't be collected" in a comment instead of "uncollectable objects". Maybe it's a more explicit definition.
There was a problem hiding this comment.
I don't think this C-API function is the right place to document these details. That's what the gc module is for.
Right, thanks: https://bugs.python.org/issue43795#msg392212 |
Co-authored-by: Victor Stinner <vstinner@python.org>
Doc/c-api/gcsupport.rst
Outdated
| @@ -199,20 +199,20 @@ garbage collection runs. | |||
| Enable the garbage collector: similar to :func:`gc.enable`. | |||
| Returns the previous state, 0 for disabled and 1 for enabled. | |||
|
|
|||
| .. versionchanged:: 3.10 | |||
| .. versionadded:: 3.10 | |||
There was a problem hiding this comment.
Please fix also the indentation.
|
Merged, thanks @scoder! I'm not really excited by adding a C API for each Python function, but this one seems common enough to justify adding an API. Also, @pablogsal liked the idea ;-) |
The GC gods we all serve are pleased with the new API ;) |
|
Thanks Victor and Pablo! |
|
I created PR #25693 which shows how useful are these new C API functions ;-) Ok, maybe, it makes the C code a little bit simpler ;-) |
|
35 lines added, 116 lines saved. Seems a good deal. |
|
FYI, this has broken CI |
|
Yes, this adds things to From now on, CI will be fail on the pull request when issues like this happen, but this one's timing was unfortunate. |
|
Ok, then let's merge #25720 to unblock this |
https://bugs.python.org/issue28254