gh-148871: extend and improve LOAD_COMMON_CONSTANT#148971
gh-148871: extend and improve LOAD_COMMON_CONSTANT#148971markshannon merged 12 commits intopython:mainfrom
LOAD_COMMON_CONSTANT#148971Conversation
NekoAsakura
requested review from
Eclips4,
FFY00,
Fidget-Spinner,
ZeroIntensity,
ericsnowcurrently,
iritkatriel,
markshannon,
savannahostrowski and
tomasr8
as code owners
markshannon
left a comment
markshannon
left a comment
There was a problem hiding this comment.
Mostly looks good.
You need to take care distinguishing between process-wide data and per-interpreter data. See the comments below.
| meth_getsets, /* tp_getset */ | ||
| 0, /* tp_base */ | ||
| 0, /* tp_dict */ | ||
| 0, /* tp_descr_get */ |
There was a problem hiding this comment.
Can you remove this list of zeros and use a named initializer: .tp_is_gc = cfunction_is_gc
| return PyLong_FromLong(oparg); | ||
| } | ||
| if (opcode == LOAD_COMMON_CONSTANT) { | ||
| switch (oparg) { |
There was a problem hiding this comment.
| switch (oparg) { | |
| return load_common_const(oparg); |
All these objects are immortal, so no need to worry about reference counts
| op(_LOAD_COMMON_CONSTANT, (-- value)) { | ||
| assert(oparg < NUM_COMMON_CONSTANTS); | ||
| PyObject *val = _PyInterpreterState_GET()->common_consts[oparg]; | ||
| PyObject *val; |
There was a problem hiding this comment.
Why aren't you using the constants table?
| static inline _PyStackRef | ||
| load_common_constant(unsigned int oparg) | ||
| { | ||
| switch (oparg) { |
There was a problem hiding this comment.
Why is this needed? These values should be in the table
| .vectorcall = _PyCFunction_vectorcall_O, | ||
| }; | ||
|
|
||
| PyObject *_PyCommonConsts[NUM_COMMON_CONSTANTS]; |
There was a problem hiding this comment.
For this to be process-wide (not per-interpreter) PyExc_AssertionError and PyExc_NotImplementedError will need to be statically allocated.
Doing that might be complex, so maybe keep the constants per-interpreter for this PR, and make everything statically allocation in a later PR?
| (PyObject *)&_PyLong_SMALL_INTS[_PY_NSMALLNEGINTS - 1]; | ||
| for (int i = 0; i < NUM_COMMON_CONSTANTS; i++) { | ||
| assert(_PyCommonConsts[i] != NULL); | ||
| assert(_Py_IsImmortal(_PyCommonConsts[i])); |
There was a problem hiding this comment.
| assert(_Py_IsImmortal(_PyCommonConsts[i])); | |
| assert(_Py_IsStaticImmortal(_PyCommonConsts[i])); |
If _PyCommonConsts is to be statically allocated.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase And if you don't make the requested changes, you will be poked with soft cushions! |
|
Thanks for the review. Pushed the lot. :') |
|
This looks good, except the changes to Sorry to go and forth on this, but I think we shouldn't make I'm worried that the changes to |
|
Sound call. Reverted. Thanks again for the review. |
Documentation build overview
61 files changed ·
|
markshannon
left a comment
markshannon
left a comment
There was a problem hiding this comment.
This fixes the compiler and selection of constants.
There are performance improvements to be made, but that can be done in another PR.
3 participants
Behaviour changes(Reverted)all.__self__/any.__self__<module 'builtins'>Nonegc.is_tracked(all)/gc.is_tracked(any)TrueFalseLOAD_COMMON_CONSTANT#148871