-
-
Notifications
You must be signed in to change notification settings - Fork 31.3k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that assertions can be compiled out, which is decidedly not intended here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general yes, but _testcapimodule.c starts with:
/* Always enable assertions */
#undef NDEBUG
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but please fix the Sphinx markup.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix also the indentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
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