← 返回首页
bpo-1635741: Port gc module to multiphase initialization by tiran · Pull Request #23377 · python/cpython · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Include my email address so I can be contacted
Cancel Submit feedback

Saved searches

Use saved searches to filter your results more quickly

Cancel Create saved search
Appearance settings
Resetting focus

bpo-1635741: Port gc module to multiphase initialization#23377

Merged
miss-islington merged 3 commits into
python:masterfrom
tiran:bpo-1635741-gc
Nov 19, 2020
Merged

bpo-1635741: Port gc module to multiphase initialization#23377
miss-islington merged 3 commits into
python:masterfrom
tiran:bpo-1635741-gc

Conversation

tiran commented Nov 18, 2020 •
edited by miss-islington
Loading

Copy link
Copy Markdown
Member

Signed-off-by: Christian Heimes christian@python.org

https://bugs.python.org/issue1635741

Automerge-Triggered-By: GH:tiran

tiran requested a review from pablogsal as a code owner November 18, 2020 19:34
pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 18, 2020

bedevere-bot commented Nov 18, 2020

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 668780e30978e11065113c438850074f3fefab5b 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 18, 2020

pablogsal commented Nov 18, 2020

Copy link
Copy Markdown
Member

LGTM, but I added the buildbot label because the gc module has some special handling on deallocation

tiran commented Nov 18, 2020

Copy link
Copy Markdown
Member Author

refleak bots are complaining about a new reference like in test_ast.

tiran requested review from corona10 and vstinner November 18, 2020 23:38

tiran commented Nov 19, 2020

Copy link
Copy Markdown
Member Author

refleak bots are complaining about a new reference like in test_ast.

Found it! gcstate->garbage is initialized much earlier at the beginning of an interpreter lifecycle.

tiran added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 19, 2020

bedevere-bot commented Nov 19, 2020

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @tiran for commit d1d2cbcb6b14b2f2d8464cb56095b44a4e118ac6 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 19, 2020
vstinner reviewed Nov 19, 2020
View reviewed changes
Comment thread Modules/gcmodule.c Outdated

vstinner Nov 19, 2020

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

This looks wrong to me. If you create multiple instances of the GC modul, callbacks member will be overriden by a new list at each call. It must be initialized exactly once in _PyGC_Init().

By the way, if (gcstate->garbage == NULL) { test in _PyGC_Init() can be removed, it's useless. This function is called exactly once per interpreter.

tiran Nov 19, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Makes sense, I'm moving gcstate->callbacks = PyList_New(0) to _PyGC_Init().

tiran added 2 commits November 19, 2020 08:53
bpo-1635741: Port gc module to multiphase initialization …
271e017
Signed-off-by: Christian Heimes <christian@python.org>
Fix reference leak …
331d676
Signed-off-by: Christian Heimes <christian@python.org>
vstinner reviewed Nov 19, 2020
View reviewed changes
Comment thread Modules/gcmodule.c Outdated Show resolved Hide resolved
Comment thread Modules/gcmodule.c Outdated Show resolved Hide resolved
tiran added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 19, 2020

bedevere-bot commented Nov 19, 2020

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @tiran for commit 7ad98d9 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 19, 2020
vstinner approved these changes Nov 19, 2020
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

LGTM.

miss-islington commented Nov 19, 2020

Copy link
Copy Markdown
Contributor

@tiran: Status check is done, and it's a pending ❌ .

miss-islington commented Nov 19, 2020

Copy link
Copy Markdown
Contributor

@tiran: Status check is done, and it's a success ✅ .

miss-islington merged commit 646d7fd into python:master Nov 19, 2020
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
bpo-1635741: Port gc module to multiphase initialization (pythonGH-23377 …
50e5184
) Signed-off-by: Christian Heimes <christian@python.org> Automerge-Triggered-By: GH:tiran
kylotan mannequin mentioned this pull request Sep 19, 2022
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

vstinner vstinner approved these changes
pablogsal Awaiting requested review from pablogsal
corona10 Awaiting requested review from corona10

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Footer

© 2026 GitHub, Inc.