Skip to content

gh-113688: Split up gcmodule.c - #113715

Merged
nascheme merged 5 commits into
python:mainfrom
colesbury:gh-113688
Jan 5, 2024
Merged

gh-113688: Split up gcmodule.c#113715
nascheme merged 5 commits into
python:mainfrom
colesbury:gh-113688

Conversation

@colesbury

@colesbury colesbury commented Jan 4, 2024 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

This splits part of Modules/gcmodule.c of into Python/gc.c, which now contains the core garbage collection implementation. The Python module remain in the Modules/gcmodule.c file.

@bedevere-app bedevere-app Bot mentioned this pull request Jan 4, 2024
Closed
@colesbury colesbury added skip news 3.13 only security fixes labels Jan 4, 2024
@colesbury
pythongh-113688: Split up gcmodule.c
fa068b5 
This splits part of Modules/gcmodule.c of into Python/gc.c, which
now contains the core garbage collection implementation. The Python
module remain in the Modules/gcmodule.c file.
@colesbury
colesbury requested a review from nascheme January 4, 2024 21:20
@colesbury
colesbury marked this pull request as ready for review January 4, 2024 21:20
@colesbury
colesbury requested review from a team and pablogsal as code owners January 4, 2024 21:20
@colesbury

colesbury commented Jan 4, 2024

Copy link
Copy Markdown
Contributor Author

Note for reviewers: The PR is split in two commits in an attempt to make it easier to review. The first commit simply moves Modules/gcmodule.c to Python/gc.c without changes. The second commit pulls out the Python package back into Modules/gcmodule.c and fixes things up.

I recommend reviewing the second commit: fa068b5 "Split up gcmodule.c".

Here's the summary of changes beyond simply moving functions from one file to another:

  • Move logic for freezing, unfreezing, getting all GC objects, etc. to Python/gc.c. See changes to pycore_gc.h. This avoids requiring many of the GC list operations in Modules/gcmodule.c.
  • Move the DEBUG_STATS, etc. constants to pycore_gc.h and prefix them as e.g., _PyGC_DEBUG_STATS. The Python constants are not renamed (i.e., gc.DEBUG_STATS). They are used in both Modules/gcmodule.c and Python/gc.c.

@nascheme

nascheme commented Jan 4, 2024

Copy link
Copy Markdown
Member

Looks okay to me. I'd suggest it's time to revise that comment at the top of the file. It's not so useful anymore and someone can use git log to see contributors and the history. Maybe add a link to https://devguide.python.org/internals/garbage-collector/ instead of links to those mailing list posts. E.g. you could put this at the top of the gc.c file instead:

//  This implements the reference cycle garbage collector.
//  The Python module inteface to the collector is in gcmodule.c.
//  See https://devguide.python.org/internals/garbage-collector/

erlend-aasland
erlend-aasland approved these changes Jan 5, 2024
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, this is a nice refactor; the resulting extension module is a lot easier to read now.

Nitpick: since we're touching these lines, we now have a golden opportunity to align the new Python/gc.c file with PEP-7, so I suggest we do so.

Comment thread Python/gc.c Outdated
Comment thread Python/gc.c Outdated
Comment thread Python/gc.c Outdated
Comment thread Python/gc.c
Comment thread Python/gc.c Outdated
Comment thread Python/gc.c Outdated
Comment thread Python/gc.c Outdated
Comment thread Python/gc.c Outdated
Comment thread Python/gc.c
Comment thread Python/gc.c Outdated
@pablogsal

pablogsal commented Jan 5, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Looks okay to me. I'd suggest it's time to revise that comment at the top of the file. It's not so useful anymore and someone can use git log to see contributors and the history. Maybe add a link to https://devguide.python.org/internals/garbage-collector/ instead of links to those mailing list posts. E.g. you could put this at the top of the gc.c file instead:

//  This implements the reference cycle garbage collector.
//  The Python module inteface to the collector is in gcmodule.c.
//  See https://devguide.python.org/internals/garbage-collector/

Related to this although not a blocker for this PR: let's ensure we document the nogil parts also in https://devguide.python.org/internals/garbage-collector/

colesbury and others added 2 commits January 5, 2024 11:23
@colesbury @erlend-aasland
Apply suggestions from code review
1a7d893 
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@colesbury @erlend-aasland
Apply suggestions from code review
24535df 
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
pablogsal
pablogsal approved these changes Jan 5, 2024
View reviewed changes

@pablogsal pablogsal 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.

I made a first round of review and everything looks good to me.

I would advise to keep this PR at minimum changes (so let's avoid the temptation to do refactors 😉 ) to keep the split as "pure" as possible.

@colesbury

colesbury commented Jan 5, 2024

Copy link
Copy Markdown
Contributor Author

@pablogsal - that makes sense. Should I revert the style changes and put them out as a subsequent PR?

@pablogsal

pablogsal commented Jan 5, 2024

Copy link
Copy Markdown
Member

Nah, don't worry, I agree it was a good opportunity but I was being careful that we end putting a lot more of other stylistic changes or similar

@colesbury

colesbury commented Jan 5, 2024

Copy link
Copy Markdown
Contributor Author

Great - would one of the reviewers please merge the PR?

@nascheme
nascheme merged commit 99854ce into python:main Jan 5, 2024
@bedevere-bot

This comment was marked as resolved.

Sign in to view
@pablogsal

pablogsal commented Jan 5, 2024

Copy link
Copy Markdown
Member

False positive, the builder is out of disk space

@colesbury
colesbury deleted the gh-113688 branch January 5, 2024 21:20
kulikjak pushed a commit to kulikjak/cpython that referenced this pull request Jan 22, 2024
@colesbury @kulikjak
pythongh-113688: Split up gcmodule.c (pythongh-113715)
e2d319f 
This splits part of Modules/gcmodule.c of into Python/gc.c, which
now contains the core garbage collection implementation. The Python
module remain in the Modules/gcmodule.c file.
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@colesbury @aisk
pythongh-113688: Split up gcmodule.c (pythongh-113715)
02927b8 
This splits part of Modules/gcmodule.c of into Python/gc.c, which
now contains the core garbage collection implementation. The Python
module remain in the Modules/gcmodule.c file.
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@colesbury @Glyphack
pythongh-113688: Split up gcmodule.c (pythongh-113715)
4eef188 
This splits part of Modules/gcmodule.c of into Python/gc.c, which
now contains the core garbage collection implementation. The Python
module remain in the Modules/gcmodule.c file.
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
@erlend-aasland erlend-aasland erlend-aasland approved these changes
@nascheme nascheme Awaiting requested review from nascheme

Assignees

No one assigned

Labels

3.13 only security fixes skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@colesbury @nascheme @pablogsal @bedevere-bot @erlend-aasland