classification
Title: Remove private _PyLong_Zero and _PyLong_One variables
Type: Stage: resolved
Components: Interpreter Core Versions: Python 3.10
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: corona10, mark.dickinson, miss-islington, pablogsal, rhettinger, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2020-10-26 21:59 by vstinner, last changed 2021-05-26 23:13 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 22993 merged vstinner, 2020-10-26 22:14
PR 22995 merged vstinner, 2020-10-26 23:33
PR 22998 merged vstinner, 2020-10-27 01:41
PR 23003 merged vstinner, 2020-10-27 16:16
PR 23008 merged vstinner, 2020-10-27 20:31
PR 26391 merged vstinner, 2021-05-26 22:31
PR 26393 merged miss-islington, 2021-05-26 22:51
Messages (15)
msg379691 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-10-26 21:59
In bpo-38858, I made the small integer singletons per interpreter: commit 630c8df5cf126594f8c1c4579c1888ca80a29d59. _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.
msg379700 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-10-26 23:00
New changeset 8e3b9f92835654943bb59d9658bb52e1b0f40a22 by Victor Stinner in branch 'master':
bpo-42161: Add _PyLong_GetZero() and _PyLong_GetOne() (GH-22993)
https://github.com/python/cpython/commit/8e3b9f92835654943bb59d9658bb52e1b0f40a22
msg379709 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-10-27 01:24
New changeset c9bc290dd6e3994a4ead2a224178bcba86f0c0e4 by Victor Stinner in branch 'master':
bpo-42161: Use _PyLong_GetZero() and _PyLong_GetOne() (GH-22995)
https://github.com/python/cpython/commit/c9bc290dd6e3994a4ead2a224178bcba86f0c0e4
msg379768 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-10-27 16:13
New changeset 37834136d0afe51d274bfc79d8705514cbe73727 by Victor Stinner in branch 'master':
bpo-42161: Modules/ uses _PyLong_GetZero() and _PyLong_GetOne() (GH-22998)
https://github.com/python/cpython/commit/37834136d0afe51d274bfc79d8705514cbe73727
msg379798 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2020-10-27 20:15
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)
msg379799 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-10-27 20:32
> 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 ;-)
msg379800 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-10-27 20:34
New changeset c310185c081110741fae914c06c7aaf673ad3d0d by Victor Stinner in branch 'master':
bpo-42161: Remove private _PyLong_Zero and _PyLong_One (GH-23003)
https://github.com/python/cpython/commit/c310185c081110741fae914c06c7aaf673ad3d0d
msg379804 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-10-27 21:24
New changeset 35b95aaf21534e4a8e3370dfd6f7482265316c9e by Victor Stinner in branch 'master':
bpo-42161: Micro-optimize _collections._count_elements() (GH-23008)
https://github.com/python/cpython/commit/35b95aaf21534e4a8e3370dfd6f7482265316c9e
msg379805 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-10-27 21:25
Thanks Raymond, I fixed the code.

I close the issue, I removed _PyLong_Zero and _PyLong_One variables.
msg388988 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2021-03-18 04:42
> 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 L326

Ideally, 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;
msg389002 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-03-18 09:00
Since it seems like you have already a ready patch to optimize the code, so go ahead and merge it.
msg389049 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2021-03-18 23:49
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.
msg394496 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-05-26 22:51
New changeset 3e7ee02327db13e4337374597cdc4458ecb9e3ad by Victor Stinner in branch 'main':
bpo-42161: mathmodule.c: move _PyLong_GetOne() loop invariant (GH-26391)
https://github.com/python/cpython/commit/3e7ee02327db13e4337374597cdc4458ecb9e3ad
msg394498 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-05-26 22:54
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.
msg394499 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-05-26 23:13
New changeset 4115996342278de7c2a1b59ac348322e7a4e9072 by Miss Islington (bot) in branch '3.10':
bpo-42161: mathmodule.c: move _PyLong_GetOne() loop invariant (GH-26391) (GH-26393)
https://github.com/python/cpython/commit/4115996342278de7c2a1b59ac348322e7a4e9072
History
Date User Action Args
2021-05-26 23:13:24vstinnersetmessages: + msg394499
2021-05-26 22:54:42vstinnersetstatus: open -> closed

messages: + msg394498
stage: patch review -> resolved
2021-05-26 22:51:16miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request24986
2021-05-26 22:51:14vstinnersetmessages: + msg394496
2021-05-26 22:31:15vstinnersetstage: resolved -> patch review
pull_requests: + pull_request24984
2021-03-18 23:49:45rhettingersetmessages: + msg389049
2021-03-18 11:40:07mark.dickinsonsetnosy: + mark.dickinson
2021-03-18 09:00:36vstinnersetmessages: + msg389002
2021-03-18 04:42:41rhettingersetstatus: closed -> open
nosy: + serhiy.storchaka, pablogsal
messages: + msg388988

2020-10-27 21:25:31vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg379805

stage: patch review -> resolved
2020-10-27 21:24:41vstinnersetmessages: + msg379804
2020-10-27 20:34:46vstinnersetmessages: + msg379800
2020-10-27 20:32:54vstinnersetmessages: + msg379799
2020-10-27 20:31:28vstinnersetpull_requests: + pull_request21924
2020-10-27 20:15:19rhettingersetnosy: + rhettinger
messages: + msg379798
2020-10-27 16:16:49vstinnersetpull_requests: + pull_request21918
2020-10-27 16:13:12vstinnersetmessages: + msg379768
2020-10-27 01:41:28vstinnersetpull_requests: + pull_request21913
2020-10-27 01:24:58vstinnersetmessages: + msg379709
2020-10-26 23:39:34corona10setnosy: + corona10
2020-10-26 23:33:25vstinnersetpull_requests: + pull_request21909
2020-10-26 23:00:10vstinnersetmessages: + msg379700
2020-10-26 22:14:23vstinnersetkeywords: + patch
stage: patch review
pull_requests: + pull_request21907
2020-10-26 21:59:37vstinnercreate