GeezerVison is showing github.com — bpo-46541: Replace core use of _Py_IDENTIFIER() with statically initialized global objects. by ericsnowcurrently · Pull Request #30928 · python/cpython
Conversation
In this PR we're no longer using _Py_IDENTIFIER() (or _Py_static_string()) in any core CPython code. It is still used in a number of non-builtin stdlib modules.
The replacement is: PyUnicodeObject (not pointer) fields under _PyRuntimeState, statically initialized as part of _PyRuntime. A new _Py_GET_GLOBAL_IDENTIFIER() macro facilitates lookup of the fields (along with _Py_GET_GLOBAL_STRING() for non-identifier strings).
https://bugs.python.org/issue46541#msg411799 explains the rationale for this change.
The core of the change is in:
- (new) Include/internal/pycore_global_strings.h - the declarations for the global strings, along with the macros
- Include/internal/pycore_runtime_init.h - added the static initializers for the global strings
- Include/internal/pycore_global_objects.h - where the struct in pycore_global_strings.h is hooked into
_PyRuntimeState
- Tools/scripts/generate_global_objects.py - added generation of the global string declarations and static initializers
I've also added a --check flag to generate_global_objects.py (along with make check-global-objects) to check for unused global strings. That check is added to the PR CI config.
The remainder of the PR updates the core code to use _Py_GET_GLOBAL_IDENTIFIER() instead of _Py_IDENTIFIER() and the related _Py*Id functions (likewise for _Py_GET_GLOBAL_STRING() instead of _Py_static_string()). This includes adding a few functions where there wasn't already an alternative to _Py*Id(), replacing the _Py_Identifier * parameter with PyObject *.
I'm planning on addressing the following separately:
- stop using
_Py_IDENTIFIER() in the stdlib modules
- (maybe) get rid of
_Py_IDENTIFIER(), etc. entirely -- this may not be doable as at least one package on PyPI using this (private) API
- (maybe) intern the strings during runtime init
https://bugs.python.org/issue46541
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
sweeneyde and erlend-aasland reacted with hooray emoji
ericsnowcurrently
changed the title
bpo-XXXXX: Replace _Py_IDENTIFIER() with statically initialized global objects.
bpo-46541: Replace _Py_IDENTIFIER() with statically initialized global objects.
Jan 26, 2022
This was referenced Jan 26, 2022
ericsnowcurrently
requested review from
1st1,
abalkin,
brettcannon,
encukou,
ethanfurman,
iritkatriel,
isidentical,
lysnikolaou,
markshannon,
methane,
ncoghlan,
pablogsal,
pganssle,
rhettinger,
tiran and
warsaw
as code owners
February 1, 2022 01:06
|
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
|
PyObject *
|
|
|
_PyDict_GetItemWithError(PyObject *dp, PyObject *kv)
|
|
|
{
|
|
|
assert(PyUnicode_CheckExact(kv));
|
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.
Just a half-baked idea: would it be possible to initialize all hashes of global strings at interpreter startup, then eliminate some branching with a special
int
_PyDict_GetItemStrWithHashInitialized(PyDictObject *mp, PyUnicodeObject *key)
{
assert(PyDict_CheckExact(mp));
assert(PyUnicode_CheckExact(key));
Py_hash_t hash = ((PyASCIIObject *)key)->hash;
assert(hash != -1);
// Inline _PyDict_GetItem_KnownHash --> _Py_dict_lookup
PyDictKeysObject *dk = mp->ma_keys;
Py_ssize_t ix = dictkeys_stringlookup(dk, key, hash);
if (ix == DKIX_EMPTY) {
return NULL;
}
else if (kind == DICT_KEYS_SPLIT) {
return mp->ma_values->values[ix];
}
else {
return DK_ENTRIES(dk)[ix].me_value;
}
}
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
ericsnowcurrently and sweeneyde reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
methane
methane left review comments
markshannon
Awaiting requested review from markshannon
markshannon is a code owner
Uh oh!
There was an error while loading. Please reload this page.
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.
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.
|