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

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

Merged

Conversation

ericsnowcurrently
Copy link
Member

@ericsnowcurrently ericsnowcurrently commented Apr 11, 2024

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 gh-94673: 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
Copy link
Member

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

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
Copy link
Member

Choose a reason for hiding this comment

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

Actually, it's void *.

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

@@ -163,7 +163,17 @@ static void
static_builtin_state_init(PyInterpreterState *interp, PyTypeObject *self)
{
if (!static_builtin_index_is_set(self)) {
static_builtin_index_set(self, interp->types.num_builtins_initialized);
assert(_Py_IsMainInterpreter(interp));
Copy link
Member

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()?)

Copy link
Member Author

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.

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

@@ -163,7 +163,17 @@ static void
static_builtin_state_init(PyInterpreterState *interp, PyTypeObject *self)
{
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 == \
Copy link
Member

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 ==

Copy link
Member Author

Choose a reason for hiding this comment

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

dropped

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

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) ==

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

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.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Perfect!

@ericsnowcurrently ericsnowcurrently merged commit eca5362 into python:main Apr 12, 2024
36 checks passed
@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
… 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
Copy link
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants