Repository navigation
Remove private _PyLong_Zero and _PyLong_One variables #86327
Description
Activity
In bpo-38858, I made the small integer singletons per interpreter: commit 630c8df. _PyLong_Zero and _PyLong_One variables are still shared by all interpreters, whereas subinterpreters must not share Python objects: see bpo-40533.
I propose to add new _PyLong_GetZero() and _PyLong_GetOne() functions to replace _PyLong_Zero and _PyLong_One variables. These functions will retrieve the singletons from tstate->interp->small_ints.
Why did you put _PyLong_GetOne() inside the loop for the fast path and outside the loop for the slow path?
==========================================================================
diff --git a/Modules/_collectionsmodule.c b/Modules/_collectionsmodule.c index 8990071f51..0e6c64d1a6 100644 --- a/Modules/_collectionsmodule.c +++ b/Modules/_collectionsmodule.c @@ -2278,6 +2278,8 @@ _collections__count_elements_impl(PyObject *module, PyObject *mapping, PyObject *dict_get; PyObject *mapping_setitem; PyObject *dict_setitem; + PyObject *zero = _PyLong_GetZero(); // borrowed reference + PyObject *one = _PyLong_GetOne(); // borrowed reference it = PyObject_GetIter(iterable); if (it == NULL) @@ -2324,10 +2326,10 @@ _collections__count_elements_impl(PyObject *module, PyObject *mapping, if (oldval == NULL) { if (PyErr_Occurred()) goto done; - if (_PyDict_SetItem_KnownHash(mapping, key, _PyLong_GetOne(), hash) < 0) + if (_PyDict_SetItem_KnownHash(mapping, key, one, hash) < 0) goto done; } else { - newval = PyNumber_Add(oldval, _PyLong_GetOne()); + newval = PyNumber_Add(oldval, one); if (newval == NULL) goto done; if (_PyDict_SetItem_KnownHash(mapping, key, newval, hash) < 0) @@ -2341,8 +2343,6 @@ _collections__count_elements_impl(PyObject *module, PyObject *mapping, if (bound_get == NULL) goto done;
- PyObject *zero = _PyLong_GetZero(); // borrowed reference - PyObject *one = _PyLong_GetOne(); // borrowed reference while (1) { key = PyIter_Next(it); if (key == NULL)
Why did you put _PyLong_GetOne() inside the loop for the fast path and outside the loop for the slow path?
Oh, I didn't notice that the first part was also a loop. I wrote PR 23008 to move the call out of the loop.
I tried to avoid calling the function if it's possible that the variable it not used. But here, it's always used, so it's relevant to move the loop invariant out of the loop ;-)
Thanks Raymond, I fixed the code.
I close the issue, I removed _PyLong_Zero and _PyLong_One variables.
Thanks Raymond, I fixed the code.
Can you fix all the other cases where this is used in inner-loop; otherwise, it is undoing everyone else's optimizations and fast paths.
Also, it seems the that primary motivation for this is support subinterpreters. That PEP hasn't been approved, so we should not be making code worse until we know there is going to some offsetting benefit.
For example, the inner-loop code in math_lcm() used to have "res == _PyLong_Zero" which was fast a register-to-register comparison (1 cycle at worst and typically 0 cycles with branch prediction). Also there used to be zero memory accesses. The new code has five sequentially dependent operations and four memory accesses (not what we want in an inner-loop):
movq __PyRuntime@GOTPCREL(%rip), %rax movq 616(%rax), %rax movq 16(%rax), %rax cmpq %r12, 3560(%rax) jne L326Ideally, the whole PR should be reverted until the subinterpreter PEP is approved. If not, then at least the changes should be made more carefully, hoisting the new call out of the hot loop:
+ zero = _PyLong_GetZero(); for (i = 1; i < nargs; i++) { x = PyNumber_Index(args[i]); if (x == NULL) { Py_DECREF(res); return NULL; } - if (res == _PyLong_GetZero()) { + if (res == zero) { /* Fast path: just check arguments. It is okay to use identity comparison here. */ Py_DECREF(x); continue; } Py_SETREF(res, long_lcm(res, x)); Py_DECREF(x); if (res == NULL) { return NULL; } } return res;
Since it seems like you have already a ready patch to optimize the code, so go ahead and merge it.
I don't have a PR in-hand. I just ran across this when trying to explain 3.9 vs 3.10 timings and M-1 vs Intel timings. Ideally, all of these PRs should be reverted. Short of that, each change needs to be reviewed to see if it created extra work inside a loop. There are 35 calls to PyLong_GetOne and 3 for PyLong_GetZero. Each of those should be checked.
Raymond: "Can you fix all the other cases where this is used in inner-loop; otherwise, it is undoing everyone else's optimizations and fast paths."
Done, thanks for the report.
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields: