gh-106931: Intern Statically Allocated Strings Globally - #107272
gh-106931: Intern Statically Allocated Strings Globally#107272ericsnowcurrently merged 9 commits into
Conversation
Sorry, something went wrong.
| { | ||
| return PyObject_Length(get_interned_dict(_PyInterpreterState_GET())); | ||
| PyObject *dict = get_interned_dict(_PyInterpreterState_GET()); | ||
| return _Py_hashtable_len(INTERNED_STRINGS) + PyObject_Length(dict); |
There was a problem hiding this comment.
| return _Py_hashtable_len(INTERNED_STRINGS) + PyObject_Length(dict); | |
| return _Py_hashtable_len(INTERNED_STRINGS) + PyDict_GET_SIZE(dict); |
Sorry, something went wrong.
There was a problem hiding this comment.
done
Sorry, something went wrong.
|
FWIW, I wouldn't be too sad if we didn't do this in 3.12 (considering the current state is that strings just aren't interned in isolated subinterpreters), but if you feel confident enough to backport this before next monday, I'm okay with that. |
Sorry, something went wrong.
Honestly, I was a little on the fence myself. My rationale is IMHO reasonable but clearly not the strongest: leaving a performance penalty, however small, because of subinterpreters is frustrating. If the solution were any more complex I'd probably lean the other way.
Regardless of what I said above, I'll stew on it a bit more. Thanks for the clarity. |
Sorry, something went wrong.
|
Thanks @ericsnowcurrently for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12. |
Sorry, something went wrong.
…gh-107272) We tried this before with a dict and for all interned strings. That ran into problems due to interpreter isolation. However, exclusively using a per-interpreter cache caused some inconsistency that can eliminate the benefit of interning. Here we circle back to using a global cache, but only for statically allocated strings. We also use a more-basic _Py_hashtable_t for that global cache instead of a dict. Ideally we would only have the global cache, but the optional isolation of each interpreter's allocator means that a non-static string object must not outlive its interpreter. Thus we would have to store a copy of each such interned string in the global cache, tied to the main interpreter. (cherry picked from commit b72947a) Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
|
GH-107358 is a backport of this pull request to the 3.12 branch. |
Sorry, something went wrong.
⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️Hi! The buildbot wasm32-emscripten node (dynamic linking) 3.x has failed when building commit b72947a. What do you need to do:
You can take a look at the buildbot page here: https://buildbot.python.org/all/#builders/1056/builds/2656 Failed tests:
Summary of the results of the build (if available): == Tests result: FAILURE == 325 tests OK. 10 slowest tests:
1 test failed: 121 tests skipped: Total duration: 26 min 18 sec Click to see traceback logsTraceback (most recent call last):
File "/opt/buildbot/bcannon-wasm/3.x.bcannon-wasm.emscripten-node-dl/build/Lib/test/libregrtest/runtest.py", line 373, in _runtest_inner
refleak = _runtest_inner2(ns, test_name)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/opt/buildbot/bcannon-wasm/3.x.bcannon-wasm.emscripten-node-dl/build/Lib/test/libregrtest/runtest.py", line 313, in _runtest_inner2
the_module = importlib.import_module(abstest)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/opt/buildbot/bcannon-wasm/3.x.bcannon-wasm.emscripten-node-dl/build/Lib/importlib/__init__.py", line 88, in import_module
return _bootstrap._gcd_import(name[level:], package, level)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "<frozen importlib._bootstrap>", line 1293, in _gcd_import
File "<frozen importlib._bootstrap>", line 1266, in _find_and_load
File "<frozen importlib._bootstrap>", line 1237, in _find_and_load_unlocked
File "<frozen importlib._bootstrap>", line 841, in _load_unlocked
File "<frozen importlib._bootstrap_external>", line 1002, in exec_module
File "<frozen importlib._bootstrap>", line 400, in _call_with_frames_removed
File "/opt/buildbot/bcannon-wasm/3.x.bcannon-wasm.emscripten-node-dl/build/Lib/test/test_sys.py", line 17, in <module>
from test.support import interpreters
File "/opt/buildbot/bcannon-wasm/3.x.bcannon-wasm.emscripten-node-dl/build/Lib/test/support/interpreters.py", line 4, in <module>
import _xxsubinterpreters as _interpreters
ModuleNotFoundError: No module named '_xxsubinterpreters'
|
Sorry, something went wrong.
⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️Hi! The buildbot wasm32-emscripten node (pthreads) 3.x has failed when building commit b72947a. What do you need to do:
You can take a look at the buildbot page here: https://buildbot.python.org/all/#builders/1050/builds/2643 Failed tests:
Summary of the results of the build (if available): == Tests result: FAILURE == 329 tests OK. 10 slowest tests:
1 test failed: 117 tests skipped: Total duration: 26 min 17 sec Click to see traceback logsTraceback (most recent call last):
File "/opt/buildbot/bcannon-wasm/3.x.bcannon-wasm.emscripten-node-pthreads/build/Lib/test/libregrtest/runtest.py", line 373, in _runtest_inner
refleak = _runtest_inner2(ns, test_name)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/opt/buildbot/bcannon-wasm/3.x.bcannon-wasm.emscripten-node-pthreads/build/Lib/test/libregrtest/runtest.py", line 313, in _runtest_inner2
the_module = importlib.import_module(abstest)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/opt/buildbot/bcannon-wasm/3.x.bcannon-wasm.emscripten-node-pthreads/build/Lib/importlib/__init__.py", line 88, in import_module
return _bootstrap._gcd_import(name[level:], package, level)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "<frozen importlib._bootstrap>", line 1293, in _gcd_import
File "<frozen importlib._bootstrap>", line 1266, in _find_and_load
File "<frozen importlib._bootstrap>", line 1237, in _find_and_load_unlocked
File "<frozen importlib._bootstrap>", line 841, in _load_unlocked
File "<frozen importlib._bootstrap_external>", line 1002, in exec_module
File "<frozen importlib._bootstrap>", line 400, in _call_with_frames_removed
File "/opt/buildbot/bcannon-wasm/3.x.bcannon-wasm.emscripten-node-pthreads/build/Lib/test/test_sys.py", line 17, in <module>
from test.support import interpreters
File "/opt/buildbot/bcannon-wasm/3.x.bcannon-wasm.emscripten-node-pthreads/build/Lib/test/support/interpreters.py", line 4, in <module>
import _xxsubinterpreters as _interpreters
ModuleNotFoundError: No module named '_xxsubinterpreters'
|
Sorry, something went wrong.
⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️Hi! The buildbot wasm32-wasi 3.x has failed when building commit b72947a. What do you need to do:
You can take a look at the buildbot page here: https://buildbot.python.org/all/#builders/1046/builds/2616 Failed tests:
Summary of the results of the build (if available): == Tests result: FAILURE == 317 tests OK. 10 slowest tests:
1 test failed: 129 tests skipped: Total duration: 5 min 22 sec Click to see traceback logsTraceback (most recent call last):
File "/Lib/test/libregrtest/runtest.py", line 373, in _runtest_inner
refleak = _runtest_inner2(ns, test_name)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Lib/test/libregrtest/runtest.py", line 313, in _runtest_inner2
the_module = importlib.import_module(abstest)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Lib/importlib/__init__.py", line 88, in import_module
return _bootstrap._gcd_import(name[level:], package, level)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "<frozen importlib._bootstrap>", line 1293, in _gcd_import
File "<frozen importlib._bootstrap>", line 1266, in _find_and_load
File "<frozen importlib._bootstrap>", line 1237, in _find_and_load_unlocked
File "<frozen importlib._bootstrap>", line 841, in _load_unlocked
File "<frozen importlib._bootstrap_external>", line 1002, in exec_module
File "<frozen importlib._bootstrap>", line 400, in _call_with_frames_removed
File "/Lib/test/test_sys.py", line 17, in <module>
from test.support import interpreters
File "/Lib/test/support/interpreters.py", line 4, in <module>
import _xxsubinterpreters as _interpreters
ModuleNotFoundError: No module named '_xxsubinterpreters'
|
Sorry, something went wrong.
…ythongh-107272) We tried this before with a dict and for all interned strings. That ran into problems due to interpreter isolation. However, exclusively using a per-interpreter cache caused some inconsistency that can eliminate the benefit of interning. Here we circle back to using a global cache, but only for statically allocated strings. We also use a more-basic _Py_hashtable_t for that global cache instead of a dict. Ideally we would only have the global cache, but the optional isolation of each interpreter's allocator means that a non-static string object must not outlive its interpreter. Thus we would have to store a copy of each such interned string in the global cache, tied to the main interpreter. (cherry-picked from commit b72947a)
…ythongh-107272) We tried this before with a dict and for all interned strings. That ran into problems due to interpreter isolation. However, exclusively using a per-interpreter cache caused some inconsistency that can eliminate the benefit of interning. Here we circle back to using a global cache, but only for statically allocated strings. We also use a more-basic _Py_hashtable_t for that global cache instead of a dict. Ideally we would only have the global cache, but the optional isolation of each interpreter's allocator means that a non-static string object must not outlive its interpreter. Thus we would have to store a copy of each such interned string in the global cache, tied to the main interpreter. (cherry-picked from commit b72947a)
|
GH-110713 is a backport of this pull request to the 3.12 branch. |
Sorry, something went wrong.
…7272) (gh-110713) We tried this before with a dict and for all interned strings. That ran into problems due to interpreter isolation. However, exclusively using a per-interpreter cache caused some inconsistency that can eliminate the benefit of interning. Here we circle back to using a global cache, but only for statically allocated strings. We also use a more-basic _Py_hashtable_t for that global cache instead of a dict. Ideally we would only have the global cache, but the optional isolation of each interpreter's allocator means that a non-static string object must not outlive its interpreter. Thus we would have to store a copy of each such interned string in the global cache, tied to the main interpreter. (cherry-picked from commit b72947a)
We tried this before with a dict and for all interned strings. That ran into problems due to interpreter isolation. However, exclusively using a per-interpreter cache caused some inconsistency that can eliminate the benefit of interning. Here we circle back to using a global cache, but only for statically allocated strings. We also use a more-basic _Py_hashtable_t for that global cache instead of a dict.
Ideally we would only have the global cache, but the optional isolation of each interpreter's allocator means that a non-static string object must not outlive its interpreter. Thus we would have to store a copy of each such interned string in the global cache, tied to the main interpreter.