GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-28254: Add a C-API for controlling the state of the garbage collector by scoder · Pull Request #25687 · python/cpython

Conversation

@scoder

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

https://bugs.python.org/issue28254

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@scoder
bpo-28254: Add a C-API for controlling the state of the garbage colle…
6229a31 
…ctor.
@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
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Apr 28, 2021
@bedevere-bot bedevere-bot added the awaiting core review label Apr 28, 2021
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pablogsal
pablogsal reviewed Apr 28, 2021
View reviewed changes
Comment thread Misc/NEWS.d/next/C API/2021-04-28-12-33-44.bpo-28254.a2561e.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@scoder
bpo-28254: Improve markup and add a What's New entry.
bfbadb9
pablogsal
pablogsal reviewed Apr 28, 2021
View reviewed changes
Comment thread Doc/whatsnew/3.10.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
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 approved these changes Apr 28, 2021
View reviewed changes
Comment thread Modules/_testcapimodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_testcapimodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_testcapimodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Apr 28, 2021
pablogsal added 3 commits April 28, 2021 14:47
@pablogsal
Update Modules/_testcapimodule.c
7b6e320
@pablogsal
Update Modules/_testcapimodule.c
b7e4ee3
@pablogsal
Update Modules/_testcapimodule.c
4e11560
@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 😉
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@scoder
bpo-28254: Formatting.
e5f0077
vstinner
vstinner reviewed Apr 28, 2021
View reviewed changes
Comment thread Doc/whatsnew/3.10.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Apr 28, 2021
View reviewed changes
Comment thread Modules/_testcapimodule.c
int old_state;

old_state = PyGC_Enable();
msg = "Enable(1)";

@vstinner vstinner Apr 28, 2021

Copy link
Copy Markdown
Member

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().

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@scoder scoder Apr 28, 2021

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Apr 28, 2021

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@scoder scoder Apr 28, 2021

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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.?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
scoder added 2 commits April 28, 2021 16:26
@scoder
bpo-28254: Move new functions to a better place (not cutting into the…
c64aa42 
… gc.collect() implementation).
@scoder
Remove an incorrect part of a comment: PyGC_Collect() actually does n…
cbee918 
…othing if GC is disabled.
@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".
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
scoder added 2 commits April 28, 2021 16:43
@scoder
bpo-28254: Add documentation for the PyGC_*() functions.
0f7e608
@scoder
bpo-28254: Add new PyGC_*() functions to the stable ABI list.
80c3625
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Apr 28, 2021
View reviewed changes
Comment thread Doc/c-api/gcsupport.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/c-api/gcsupport.rst
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.

@vstinner vstinner Apr 28, 2021

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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.

@vstinner vstinner Apr 28, 2021

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@scoder scoder Apr 28, 2021

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/c-api/gcsupport.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/c-api/gcsupport.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/c-api/gcsupport.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@scoder
bpo-28254: Apply suggestions from doc review.
25fb2aa
vstinner
vstinner reviewed Apr 28, 2021
View reviewed changes
Comment thread Doc/c-api/gcsupport.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/c-api/gcsupport.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/c-api/gcsupport.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@scoder @vstinner
Apply suggestions from doc review
10ad929 
Co-authored-by: Victor Stinner <[email protected]>
vstinner
vstinner approved these changes Apr 28, 2021
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/c-api/gcsupport.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
scoder added 2 commits April 28, 2021 17:48
@scoder
Add a word on how PyGC_Collect() and gc.collect() differ.
1ffe937
@scoder
Fix version marker.
d517665
vstinner
vstinner reviewed Apr 28, 2021
View reviewed changes
Comment thread Doc/c-api/gcsupport.rst Outdated
Returns the previous state, 0 for disabled and 1 for enabled.

.. versionchanged:: 3.10
.. versionadded:: 3.10

@vstinner vstinner Apr 28, 2021

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@scoder
Fix version marker (II).
84fb2af
vstinner
vstinner approved these changes Apr 28, 2021
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner
vstinner merged commit 3cc481b into python:master Apr 28, 2021
@bedevere-bot bedevere-bot removed the awaiting merge label 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 ;-)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 ;)
vstinner reacted with heart emoji vstinner reacted with eyes emoji
All reactions
  • ❤️ 1 reaction
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@scoder

scoder commented Apr 28, 2021

Copy link
Copy Markdown
Contributor Author

Thanks Victor and Pablo!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 ;-)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@scoder

scoder commented Apr 28, 2021

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@encukou

encukou commented Apr 29, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pablogsal

pablogsal commented Apr 29, 2021

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Apr 29, 2021
bpo-43933: Show frame.f_lineno as None, rather than -1, if there is no line number. #25717
Merged
bpo-43976: add vendor config #25718
Closed
bpo-43933: Set frame.f_lineno during call to __exit__ #25719
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@scoder @pablogsal @vstinner @markshannon @encukou @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.