◐ Shell
reader mode source ↗
Skip to content

bpo-28254: Add a C-API for controlling the state of the garbage collector#25687

Merged
vstinner merged 17 commits into
python:masterfrom
scoder:bpo-28254_gc_capi
Apr 28, 2021
Merged

bpo-28254: Add a C-API for controlling the state of the garbage collector#25687
vstinner merged 17 commits into
python:masterfrom
scoder:bpo-28254_gc_capi

Conversation

@scoder

@scoder scoder commented Apr 28, 2021 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

@scoder scoder requested a review from vstinner April 28, 2021 13:13
@scoder scoder requested a review from pablogsal as a code owner April 28, 2021 13:13
@pablogsal

pablogsal commented Apr 28, 2021

Copy link
Copy Markdown
Member

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.

pablogsal and others added 2 commits April 28, 2021 14:44
@pablogsal
Apply suggestions from code review
f41d269
@scoder
bpo-28254: Return the previous GC state from the Enable/Disable funct… …
56465a9 
…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.
@pablogsal

pablogsal commented Apr 28, 2021

Copy link
Copy Markdown
Member

Thanks for the PR @scoder! Make sure to merge it before end of the week 😉

@scoder

scoder commented Apr 28, 2021

Copy link
Copy Markdown
Contributor Author

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.

@scoder

scoder commented Apr 28, 2021

Copy link
Copy Markdown
Contributor Author

Are the C functions prototypes defined in the right place? Limited API, header file hierarchy, etc.?

@vstinner

vstinner commented Apr 28, 2021

Copy link
Copy Markdown
Member

Are the C functions prototypes defined in the right place? Limited API, header file hierarchy, etc.?

You added them to the stable ABI and so you should run "regen-limited-abi".

@scoder

scoder commented Apr 28, 2021

Copy link
Copy Markdown
Contributor Author

Are the C functions prototypes defined in the right place? Limited API, header file hierarchy, etc.?

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.
I'll check where the others came from.

23 hidden items Load more…
@vstinner

vstinner commented Apr 28, 2021

Copy link
Copy Markdown
Member

That added more than the three. I only committed the ones I added here in order to keep the PR clean.
I'll check where the others came from.

Right, thanks: https://bugs.python.org/issue43795#msg392212

@scoder @vstinner
Apply suggestions from doc review …
10ad929 
Co-authored-by: Victor Stinner <vstinner@python.org>
vstinner
vstinner approved these changes Apr 28, 2021
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

LGTM but please fix the Sphinx markup.

vstinner
vstinner approved these changes Apr 28, 2021
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

LGTM.

@vstinner vstinner merged commit 3cc481b into python:master Apr 28, 2021
@scoder scoder deleted the bpo-28254_gc_capi branch April 28, 2021 16:12
@vstinner

vstinner commented Apr 28, 2021

Copy link
Copy Markdown
Member

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 ;-)

@pablogsal

pablogsal commented Apr 28, 2021

Copy link
Copy Markdown
Member

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 ;)

@scoder

scoder commented Apr 28, 2021

Copy link
Copy Markdown
Contributor Author

Thanks Victor and Pablo!

@vstinner

vstinner commented Apr 28, 2021

Copy link
Copy Markdown
Member

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 ;-)

@scoder

scoder commented Apr 28, 2021

Copy link
Copy Markdown
Contributor Author

35 lines added, 116 lines saved. Seems a good deal.

@markshannon

markshannon commented Apr 29, 2021

Copy link
Copy Markdown
Member

FYI, this has broken CI
Example: https://github.com/python/cpython/pull/25719/checks?check_run_id=2466874649
Proposed fix:
#25716

@encukou

encukou commented Apr 29, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Yes, this adds things to Doc/data/stable_abi.dat but not PC/python3dll.c, which is quite wrong.
Meanwhile, I merged the part of PEP-652 that will prevent these from going out of sync (by generating them both from a single manifest), but the CI on this PR ran before that and the merge happened after.

From now on, CI will be fail on the pull request when issues like this happen, but this one's timing was unfortunate.

@pablogsal

pablogsal commented Apr 29, 2021

Copy link
Copy Markdown
Member

Ok, then let's merge #25720 to unblock this

This was referenced Apr 29, 2021
Merged
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pablogsal pablogsal pablogsal approved these changes

@vstinner vstinner vstinner approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@scoder @pablogsal @vstinner @markshannon @encukou @the-knights-who-say-ni @bedevere-bot