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.

Sign up for GitHub

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

Jump to bottom

gh-105020: Share tp_bases and tp_mro Between Interpreters For All Static Builtin Types #105115

Merged
merged 8 commits into from
May 31, 2023
Merged

gh-105020: Share tp_bases and tp_mro Between Interpreters For All Static Builtin Types #105115

merged 8 commits into from
May 31, 2023

Conversation

ericsnowcurrently
Copy link
Member

@ericsnowcurrently ericsnowcurrently commented May 30, 2023 •
edited by bedevere-bot
Loading

In gh-103912 we added tp_bases and tp_mro to each PyInterpreterState.types.builtins entry. However, doing so ignored the fact that both PyTypeObject fields are public API, and not documented as internal (as opposed to tp_subclasses). We address that here by reverting back to shared objects, making them immortal in the process.

@bedevere-bot bedevere-bot mentioned this pull request May 30, 2023
Closed
@ericsnowcurrently ericsnowcurrently marked this pull request as ready for review May 30, 2023 21:00
gpshead
gpshead reviewed May 30, 2023
View reviewed changes
Copy link
Member

@gpshead gpshead left a comment

Choose a reason for hiding this comment

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

Do we have a meaningful place to put a regression test to assert that tp_bases and tp_mro are never NULL in the situations user code was encountering that in 3.12beta1?

gpshead
gpshead reviewed May 30, 2023
View reviewed changes
Include/internal/pycore_typeobject.h Outdated
@@ -49,8 +49,6 @@ typedef struct {
// XXX tp_dict, tp_bases, and tp_mro can probably be statically
Copy link
Member

Choose a reason for hiding this comment

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

should this comment (added by the original PR) be updated now that bases and mro don't exist here?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@ericsnowcurrently
Copy link
Member Author

Do we have a meaningful place to put a regression test to assert that tp_bases and tp_mro are never NULL in the situations user code was encountering that in 3.12beta1?

I'll add a test in test_capi.

@ericsnowcurrently
Copy link
Member Author

I've added a test. Thanks for the suggestion.

@ericsnowcurrently ericsnowcurrently requested a review from Yhg1s May 30, 2023 22:04
gpshead
gpshead approved these changes May 30, 2023
View reviewed changes
Copy link
Member

@gpshead gpshead left a comment

Choose a reason for hiding this comment

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

This looks good to me.

I do wonder if anything is going to be tripped up by the tp_dict as well, but that seems like a separate category of change given I believe it could've been NULL in the past - if it comes up it can be addressed separately.

@gpshead
Copy link
Member

gpshead commented May 30, 2023

you might want a NEWS entry fwiw... as people do read the changelog between beta1 and beta2.

@ericsnowcurrently ericsnowcurrently enabled auto-merge (squash) May 30, 2023 23:46
@ericsnowcurrently ericsnowcurrently merged commit 7be667d into python:main May 31, 2023
@miss-islington
Copy link
Contributor

Thanks @ericsnowcurrently for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry @ericsnowcurrently, I had trouble checking out the 3.12 backport branch.
Please retry by removing and re-adding the "needs backport to 3.12" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker 7be667dfafa2465df6342d72dca9c1f82dd830d0 3.12

@ericsnowcurrently ericsnowcurrently added needs backport to 3.12 bug and security fixes and removed needs backport to 3.12 bug and security fixes labels May 31, 2023
@miss-islington
Copy link
Contributor

Thanks @ericsnowcurrently for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 31, 2023
@ericsnowcurrently @miss-islington
pythongh-105020: Share tp_bases and tp_mro Between Interpreters For A…
c28c929 
…ll Static Builtin Types (pythongh-105115)

In pythongh-103912 we added tp_bases and tp_mro to each PyInterpreterState.types.builtins entry.  However, doing so ignored the fact that both PyTypeObject fields are public API, and not documented as internal (as opposed to tp_subclasses).  We address that here by reverting back to shared objects, making them immortal in the process.
(cherry picked from commit 7be667d)

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
@bedevere-bot
Copy link

GH-105124 is a backport of this pull request to the 3.12 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.12 bug and security fixes label May 31, 2023
@ericsnowcurrently ericsnowcurrently deleted the fix-tp-bases branch May 31, 2023 00:23
ericsnowcurrently pushed a commit that referenced this pull request Jun 1, 2023
@miss-islington
[3.12] gh-105020: Share tp_bases and tp_mro Between Interpreters For …
c38ceb0 
…All Static Builtin Types (gh-105115) (gh-105124)

In gh-103912 we added tp_bases and tp_mro to each PyInterpreterState.types.builtins entry.  However, doing so ignored the fact that both PyTypeObject fields are public API, and not documented as internal (as opposed to tp_subclasses).  We address that here by reverting back to shared objects, making them immortal in the process.
(cherry picked from commit 7be667d)

Co-authored-by: Eric Snow ericsnowcurrently@gmail.com
@erlend-aasland erlend-aasland mentioned this pull request Jun 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gpshead gpshead gpshead approved these changes

@lysnikolaou lysnikolaou Awaiting requested review from lysnikolaou

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

@Yhg1s Yhg1s Awaiting requested review from Yhg1s

Assignees

@ericsnowcurrently ericsnowcurrently

Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ericsnowcurrently @gpshead @miss-islington @bedevere-bot