GeezerVison

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

GeezerVison is showing github.com — gh-94673: Clarify About Runtime State Related to Static Builtin Types by ericsnowcurrently · Pull Request #117761 · python/cpython

Conversation

ericsnowcurrently commented Apr 11, 2024 •

edited by bedevere-app Bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

Guido pointed out to me that some details about the per-interpreter state for the builtin types aren't especially clear. I'm addressing that by:

adding a comment explaining that state
adding _PyRuntimeState.types.next_builtin_index
adding some asserts to point out the relationship between each index and the interp/global runtime state

Regarding next_builtin_index, it replaces our use of the main interpreter's interp->types.num_builtins_initialized as the next index. Even though it's technically redundant, the distinct global runtime field helps clarify explicitly that each index is from global state.

Issue: [subinterpreters] Static types incorrectly share some objects between interpreters #94673

Sorry, something went wrong.

Uh oh!

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

All reactions

ericsnowcurrently added 3 commits

April 11, 2024 10:01


          Add a note about the static builtin types to struct types_state.

c0b035e


          Add asserts for static builtin type initialization constraints.

a73483b


          Add _PyRuntimeState.types.next_builtin_index.

b8d622e

ericsnowcurrently added the skip news label

Apr 11, 2024

ericsnowcurrently requested a review from gvanrossum

April 11, 2024 16:18

ericsnowcurrently changed the title ~~gh-???: Clarify About Runtime State Related to Static Builtin Types~~ gh-94673: Clarify About Runtime State Related to Static Builtin Types

Apr 11, 2024

ericsnowcurrently marked this pull request as ready for review

April 11, 2024 20:40

ericsnowcurrently requested a review from markshannon as a code owner

April 11, 2024 20:40

bedevere-app Bot added the awaiting core review label

Apr 11, 2024

ericsnowcurrently removed the request for review from markshannon

April 11, 2024 20:40

bedevere-app Bot mentioned this pull request

Apr 11, 2024

[subinterpreters] Static types incorrectly share some objects between interpreters #94673

Closed

gvanrossum reviewed

Apr 12, 2024

View reviewed changes

gvanrossum 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.

Hm. With suitable explanation we may not need the runtime counter -- we can just explain that the index is assigned by the main thread based upon its counter.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Include/internal/pycore_typeobject.h Outdated

		Those objects are stored in the PyInterpreterState.types.builtins
		array, at the index corresponding to each specific static builtin
		type. That index (a size_t value) is stored in the tp_subclasses
		field (defined as PyObject*). We re-purposed the now-unused

gvanrossum Apr 12, 2024

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.

Actually, it's void *.

Sorry, something went wrong.

Uh oh!

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

ericsnowcurrently reacted with thumbs up emoji

All reactions

👍 1 reaction

ericsnowcurrently Apr 12, 2024

Copy link

Copy Markdown

Member 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.

fixed

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Objects/typeobject.c Outdated

		{
		if (!static_builtin_index_is_set(self)) {
		static_builtin_index_set(self, interp->types.num_builtins_initialized);
		assert(_Py_IsMainInterpreter(interp));

gvanrossum Apr 12, 2024

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 static_builtin_state_clear() there's a check for this rather than an assert. Maybe it should be an if here too, for symmetry? (Maybe the assert should be !static_builtin_index_is_set()?)

Sorry, something went wrong.

Uh oh!

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

All reactions

ericsnowcurrently Apr 12, 2024

Copy link

Copy Markdown

Member 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 point. I'll flip it around.

Sorry, something went wrong.

Uh oh!

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

All reactions

ericsnowcurrently Apr 12, 2024

Copy link

Copy Markdown

Member 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.

fixed

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Objects/typeobject.c Outdated

		if (!static_builtin_index_is_set(self)) {
		static_builtin_index_set(self, interp->types.num_builtins_initialized);
		assert(_Py_IsMainInterpreter(interp));
		assert(interp->types.num_builtins_initialized == \

gvanrossum Apr 12, 2024

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.

It's C. :-)

Suggested change

	assert(interp->types.num_builtins_initialized == \
	assert(interp->types.num_builtins_initialized ==

Sorry, something went wrong.

Uh oh!

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

ericsnowcurrently reacted with thumbs up emoji

All reactions

👍 1 reaction

ericsnowcurrently Apr 12, 2024

Copy link

Copy Markdown

Member 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.

dropped

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Objects/typeobject.c Outdated

		}
		else {
		assert(!_Py_IsMainInterpreter(interp));
		assert(static_builtin_index_get(self) == \

gvanrossum Apr 12, 2024

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.

Suggested change

	assert(static_builtin_index_get(self) == \
	assert(static_builtin_index_get(self) ==

Sorry, something went wrong.

Uh oh!

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

ericsnowcurrently reacted with thumbs up emoji

All reactions

👍 1 reaction

ericsnowcurrently Apr 12, 2024

Copy link

Copy Markdown

Member 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.

fixed

Sorry, something went wrong.

Uh oh!

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

All reactions

ericsnowcurrently added 5 commits

April 12, 2024 10:15


          Revert "Add _PyRuntimeState.types.next_builtin_index."

00feed6

This reverts commit b8d622e.


          Describe the correct type for tp_subclasses.

6e1853b


          Clarify that tp_subclasses is PyObject* for other type objects.

7b029d9


          Flip around the condition in static_builtin_state_init().

7cf7c04


          Drop an unnecessary line escape.

f668cf1

ericsnowcurrently commented Apr 12, 2024

Copy link

Copy Markdown

Member Author

Hm. With suitable explanation we may not need the runtime counter -- we can just explain that the index is assigned by the main thread based upon its counter.

Good point. I've dropped the extra counter.

All reactions

Sorry, something went wrong.

Uh oh!

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

gvanrossum approved these changes

Apr 12, 2024

View reviewed changes

gvanrossum 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.

Perfect!

Sorry, something went wrong.

Uh oh!

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

ericsnowcurrently reacted with heart emoji

All reactions

❤️ 1 reaction

bedevere-app Bot added awaiting merge and removed awaiting core review labels

Apr 12, 2024

ericsnowcurrently merged commit eca5362 into python:main

Apr 12, 2024

bedevere-app Bot removed the awaiting merge label

Apr 12, 2024

ericsnowcurrently deleted the static-builtin-types-index branch

April 12, 2024 22:39

diegorusso pushed a commit to diegorusso/cpython that referenced this pull request

Apr 17, 2024


          pythongh-94673: Clarify About Runtime State Related to Static Builtin…

8d71390

… Types (pythongh-117761)

Guido pointed out to me that some details about the per-interpreter state for the builtin types aren't especially clear.  I'm addressing that by:

* adding a comment explaining that state
* adding some asserts to point out the relationship between each index and the interp/global runtime state

erlend-aasland commented Apr 17, 2024

Copy link

Copy Markdown

Contributor

After this PR, static_builtin_index_is_set is only used in debug builds; I'm now getting a warning for release builds:

Objects/typeobject.c:120:1: warning: unused function 'static_builtin_index_is_set' [-Wunused-function]
static_builtin_index_is_set(PyTypeObject *self)
^
1 warning generated.

All reactions

Sorry, something went wrong.

Uh oh!

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

Reviewers

gvanrossum

gvanrossum approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

skip news

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.

3 participants

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.

Conversation

ericsnowcurrently commented Apr 11, 2024 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently commented Apr 12, 2024

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

erlend-aasland commented Apr 17, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ericsnowcurrently commented Apr 11, 2024 •

edited by bedevere-app Bot

Loading

Uh oh!

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