Skip to content
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-1635741: Port gc module to multiphase initialization #23377

Merged
merged 3 commits into from
Nov 19, 2020

Conversation

tiran
Copy link
Member

@tiran tiran commented Nov 18, 2020

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

https://bugs.python.org/issue1635741

Automerge-Triggered-By: GH:tiran

@bedevere-bot
Copy link

🤖 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 bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 18, 2020
@pablogsal
Copy link
Member

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

@tiran
Copy link
Member Author

tiran commented Nov 18, 2020

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

@tiran
Copy link
Member Author

tiran commented Nov 19, 2020

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 tiran added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 19, 2020
@bedevere-bot
Copy link

🤖 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 bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 19, 2020
}

assert(gcstate->callbacks == NULL);
gcstate->callbacks = PyList_New(0);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Signed-off-by: Christian Heimes <christian@python.org>
Signed-off-by: Christian Heimes <christian@python.org>
Modules/gcmodule.c Outdated Show resolved Hide resolved
Modules/gcmodule.c Outdated Show resolved Hide resolved
@tiran tiran added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 19, 2020
@bedevere-bot
Copy link

🤖 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 bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 19, 2020
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

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

@miss-islington 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
)

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

Automerge-Triggered-By: GH:tiran
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants