◐ Shell
reader mode source ↗
Skip to content

gh-94673: Clarify About Runtime State Related to Static Builtin Types#117761

Merged
ericsnowcurrently merged 8 commits into
python:mainfrom
ericsnowcurrently:static-builtin-types-index
Apr 12, 2024
Merged

gh-94673: Clarify About Runtime State Related to Static Builtin Types#117761
ericsnowcurrently merged 8 commits into
python:mainfrom
ericsnowcurrently:static-builtin-types-index

Conversation

@ericsnowcurrently

@ericsnowcurrently ericsnowcurrently commented Apr 11, 2024 •
edited by bedevere-app Bot
Loading

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.

@ericsnowcurrently ericsnowcurrently changed the title gh-???: Clarify About Runtime State Related to Static Builtin Types Apr 11, 2024
@ericsnowcurrently ericsnowcurrently marked this pull request as ready for review April 11, 2024 20:40
@ericsnowcurrently ericsnowcurrently removed the request for review from markshannon April 11, 2024 20:40
@bedevere-app bedevere-app Bot mentioned this pull request Apr 11, 2024
Closed
gvanrossum
gvanrossum reviewed Apr 12, 2024
View reviewed changes

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

Hide comment

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.

@ericsnowcurrently

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.

gvanrossum
gvanrossum approved these changes Apr 12, 2024
View reviewed changes

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

Hide comment

Perfect!

@ericsnowcurrently ericsnowcurrently merged commit eca5362 into python:main Apr 12, 2024
@ericsnowcurrently 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
@ericsnowcurrently @diegorusso
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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ericsnowcurrently @erlend-aasland @gvanrossum