Skip to content

Conversation

@hongweipeng
Copy link
Contributor

@hongweipeng hongweipeng commented Sep 6, 2019

Master branch:

>>> a = 2 ** 30 >>> b = 2 ** 30 - 1 >>> id(a - b) 140649692469056 >>> id(1) 9865760 

This PR:

>>> a = 2 ** 30 >>> b = 2 ** 30 - 1 >>> id(a - b) 9865760 >>> id(1) 9865760 

https://bugs.python.org/issue27145

@vstinner
Copy link
Member

vstinner commented Sep 6, 2019

Can you please try to add a test to test_long? The test should be decorated with @support.cpython_only.

@hongweipeng
Copy link
Contributor Author

@vstinner Thanks for your quick reply. I have added a test to test_long.

Copy link
Contributor

@sir-sigurd sir-sigurd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be handled in x_sub, because we can get small ints only with addition of single digit ints or with subtraction. But single digit ints are already handled here

cpython/Objects/longobject.c

Lines 3256 to 3258 in d8c93aa

if (Py_ABS(Py_SIZE(a)) <= 1 && Py_ABS(Py_SIZE(b)) <= 1) {
return PyLong_FromLong(MEDIUM_VALUE(a) + MEDIUM_VALUE(b));
}

and here

cpython/Objects/longobject.c

Lines 3290 to 3292 in d8c93aa

if (Py_ABS(Py_SIZE(a)) <= 1 && Py_ABS(Py_SIZE(b)) <= 1) {
return PyLong_FromLong(MEDIUM_VALUE(a) - MEDIUM_VALUE(b));
}

Also it should be clear how this change affects performance (i.e. benchmarks are needed).

@hongweipeng

This comment has been minimized.

@hongweipeng
Copy link
Contributor Author

I think it should be handled in x_sub,...

@sir-sigurd It can't be handled in x_sub or x_add, because They are required to return to a new int here:

cpython/Objects/longobject.c

Lines 3261 to 3267 in d8c93aa

z = x_add(a, b);
if (z != NULL) {
/* x_add received at least one multiple-digit int,
and thus z must be a multiple-digit int.
That also means z is not an element of
small_ints, so negating it in-place is safe. */
assert(Py_REFCNT(z) == 1);

@sir-sigurd
Copy link
Contributor

Handling it in x_sub sir-sigurd@53adba4. All tests pass.

@hongweipeng
Copy link
Contributor Author

@sir-sigurd You are right, only a positive number and a negative number will result a small int. Thank you for your commit.

@vstinner
Copy link
Member

vstinner commented Sep 9, 2019

Here's benchmarks: # ./old -m timeit -s 'a = 2 ** 60' -s 'b = a + a' 20000000 loops, best of 5: 11.7 nsec per loop 

Please avoid the timeit module for microbenchmarks: it's not reliable at all.

Please use pyperf timeit:
https://pyperf.readthedocs.io/en/latest/cli.html#timeit-cmd

The best is to use pyperf timeit --compare-to, or store the result into 2 json files and then use pyperf compare_to.

@hongweipeng
Copy link
Contributor Author

./python -m pyperf timeit "a = 2 ** 30; b = 2 ** 30 - 1" "c = a - b" --compare-to=../cpython-master/python /root/cpython-master/python: ..................... 192 ns +- 3 ns /root/cpython/python: ..................... 194 ns +- 6 ns Mean +- std dev: [/root/cpython-master/python] 192 ns +- 3 ns -> [/root/cpython/python] 194 ns +- 6 ns: 1.01x slower (+1%) 

20190909234843

@vstinner
Copy link
Member

vstinner commented Sep 9, 2019

"Not significant": good :-)

@sir-sigurd
Copy link
Contributor

sir-sigurd commented Sep 9, 2019

@hongweipeng did you compile it with PGO and LTO?
I'm asking because I observed more significant slowdown. So I'd like to recheck it before merge.

@hongweipeng
Copy link
Contributor Author

@sir-sigurd I use ./configure --with-pydebug && make -j to compile.

@sir-sigurd
Copy link
Contributor

sir-sigurd commented Sep 9, 2019

@hongweipeng
you should use ./configure --enable-optimizations --with-lto; make -j for benchmarks.

Here are my benchmark results:

python -m pyperf timeit "a = 2 ** 30; b = 2 ** 30 - 1" "a - b" --compare-to=../cpython-master/venv/bin/python --duplicate=1000 /home/sergey/tmp/cpython-master/venv/bin/python: ..................... 36.3 ns +- 0.3 ns /home/sergey/tmp/cpython-dev/venv/bin/python: ..................... 37.8 ns +- 0.3 ns Mean +- std dev: [/home/sergey/tmp/cpython-master/venv/bin/python] 36.3 ns +- 0.3 ns -> [/home/sergey/tmp/cpython-dev/venv/bin/python] 37.8 ns +- 0.3 ns: 1.04x slower (+4%) python -m pyperf timeit "a = 2 ** 60; b = 2 ** 60 - 1" "a - b" --compare-to=../cpython-master/venv/bin/python --duplicate=1000 /home/sergey/tmp/cpython-master/venv/bin/python: ..................... 37.7 ns +- 0.4 ns /home/sergey/tmp/cpython-dev/venv/bin/python: ..................... 40.0 ns +- 0.6 ns Mean +- std dev: [/home/sergey/tmp/cpython-master/venv/bin/python] 37.7 ns +- 0.4 ns -> [/home/sergey/tmp/cpython-dev/venv/bin/python] 40.0 ns +- 0.6 ns: 1.06x slower (+6%) 
@sir-sigurd
Copy link
Contributor

IMO it worth to try this suggestions from BPO issue:

4. in x_sub: The previous patches made it safe for x_sub to return a reference to a statically allocated small int, and thus made it possible to implement the following optimization. In case a and b have the same number of digits, x_sub finds the most significant digit where a and b differ. Then, if there is no such digit, it means a and b are equal, and so x_sub does 'return (PyLongObject *)PyLong_FromLong(0);'. I propose to add another check after that, to determine whether the least significant digit is the only digit where a and b differ. In that case, we can do something very similar to what long_add and long_sub do when they realize they are dealing with single-digit ints (as mentioned in 'the current state' section): return (PyLongObject *)PyLong_FromLong((sdigit)a->ob_digit[0] - (sdigit)b->ob_digit[0]); 

It should be applied here:

cpython/Objects/longobject.c

Lines 3219 to 3222 in b42130d

while (--i >= 0 && a->ob_digit[i] == b->ob_digit[i])
;
if (i < 0)
return (PyLongObject *)PyLong_FromLong(0);

@hongweipeng

This comment has been minimized.

@sir-sigurd
Copy link
Contributor

[root@instance-1 cpython]# ./python -m pyperf timeit "a = 2 ** 100; b = 2 ** 100 - 1" "c = a - b" --compare-to=../cpython-master/python --duplicate=1000 /root/cpython-master/python: ..................... 697 ns +- 4 ns /root/cpython/python: ..................... 711 ns +- 6 ns Mean +- std dev: [/root/cpython-master/python] 697 ns +- 4 ns -> [/root/cpython/python] 711 ns +- 6 ns: 1.02x slower (+2%) [root@instance-1 cpython]# ./python -m pyperf timeit "a = 2 ** 100 + 1; b = 2 ** 100" "c = a - b" --compare-to=../cpython-master/python --duplicate=1000 /root/cpython-master/python: ..................... 703 ns +- 10 ns /root/cpython/python: ..................... 692 ns +- 11 ns Mean +- std dev: [/root/cpython-master/python] 703 ns +- 10 ns -> [/root/cpython/python] 692 ns +- 11 ns: 1.02x faster (-2%) 

These results look sane, but it's better to test a - b, but not c = a - b, also we might be more interested in performance of smaller ints.

@hongweipeng

This comment has been minimized.

@hongweipeng
Copy link
Contributor Author

Ping. I've reverted the code for some days. @vstinner Would you mind to review again?

@vstinner
Copy link
Member

vstinner commented Oct 7, 2019

I hacked longobject.c to check if other methods can return "de-normalized" results. I tested without your PR. The following functions should call maybe_small_long():

  • PyLong_FromDouble()
  • long_from_binary_base()
  • long_mod(): for mod
  • long_divmod(): for mod
  • long_add()
  • long_sub()
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. But I would prefer to have a second review of another core dev.

@mdickinson, @serhiy-storchaka, @pablogsal, @methane, @nanjekyejoannah: Would you mind to review this PR?

@vstinner
Copy link
Member

vstinner commented Oct 7, 2019

Here is my hack to ensure that the result of all long methods is normalized:

commit 549357ae7070ae9780e997d4bf5fd741cdef2843 Author: Victor Stinner <vstinner@python.org> Date: Mon Oct 7 10:58:00 2019 +0200 WIP: long_is_normalized() diff --git a/Objects/longobject.c b/Objects/longobject.c index 2b57ea1866..399ae9a4b8 100644 --- a/Objects/longobject.c +++ b/Objects/longobject.c @@ -120,6 +120,40 @@ _PyLong_Negate(PyLongObject **x_p) if (PyErr_CheckSignals()) PyTryBlock \ } while(0) + +#ifndef NDEBUG +static int +long_is_small_int(PyLongObject *v) +{ + if (Py_ABS(Py_SIZE(v)) > 1) { + return 0; + } + sdigit ival = MEDIUM_VALUE(v); + return IS_SMALL_INT(ival); +} + + +static int +long_is_normalized(PyLongObject *v) +{ + if (v == NULL) { + return 1; + } + + Py_ssize_t n = Py_ABS(Py_SIZE(v)); + assert(n == 0 || v->ob_digit[n - 1] != 0); + if (Py_ABS(Py_SIZE(v)) <= 1) { + sdigit ival = MEDIUM_VALUE(v); + if (IS_SMALL_INT(ival)) { + /* is a small integer singleton? */ + assert(v == &small_ints[ival + NSMALLNEGINTS]); + } + } + return 1; +} +#endif + + /* Normalize (remove leading zeros from) an int object. Doesn't attempt to free the storage--in most cases, due to the nature of the algorithms used, this could save at most be one word anyway. */ @@ -127,6 +161,8 @@ _PyLong_Negate(PyLongObject **x_p) static PyLongObject * long_normalize(PyLongObject *v) { + assert(Py_REFCNT(v) == 1); + Py_ssize_t j = Py_ABS(Py_SIZE(v)); Py_ssize_t i = j; @@ -305,6 +341,7 @@ _PyLong_Copy(PyLongObject *src) while (--i >= 0) result->ob_digit[i] = src->ob_digit[i]; } + assert(long_is_normalized(result)); return (PyObject *)result; } @@ -337,11 +374,13 @@ PyLong_FromLong(long ival) /* Fast path for single-digit ints */ if (!(abs_ival >> PyLong_SHIFT)) { v = _PyLong_New(1); - if (v) { - Py_SIZE(v) = sign; - v->ob_digit[0] = Py_SAFE_DOWNCAST( - abs_ival, unsigned long, digit); + if (!v) { + return NULL; } + Py_SIZE(v) = sign; + v->ob_digit[0] = Py_SAFE_DOWNCAST( + abs_ival, unsigned long, digit); + assert(long_is_normalized(v)); return (PyObject*)v; } @@ -349,13 +388,15 @@ PyLong_FromLong(long ival) /* 2 digits */ if (!(abs_ival >> 2*PyLong_SHIFT)) { v = _PyLong_New(2); - if (v) { - Py_SIZE(v) = 2*sign; - v->ob_digit[0] = Py_SAFE_DOWNCAST( - abs_ival & PyLong_MASK, unsigned long, digit); - v->ob_digit[1] = Py_SAFE_DOWNCAST( - abs_ival >> PyLong_SHIFT, unsigned long, digit); + if (!v) { + return NULL; } + Py_SIZE(v) = 2*sign; + v->ob_digit[0] = Py_SAFE_DOWNCAST( + abs_ival & PyLong_MASK, unsigned long, digit); + v->ob_digit[1] = Py_SAFE_DOWNCAST( + abs_ival >> PyLong_SHIFT, unsigned long, digit); + assert(long_is_normalized(v)); return (PyObject*)v; } #endif @@ -367,16 +408,18 @@ PyLong_FromLong(long ival) t >>= PyLong_SHIFT; } v = _PyLong_New(ndigits); - if (v != NULL) { - digit *p = v->ob_digit; - Py_SIZE(v) = ndigits*sign; - t = abs_ival; - while (t) { - *p++ = Py_SAFE_DOWNCAST( - t & PyLong_MASK, unsigned long, digit); - t >>= PyLong_SHIFT; - } + if (v == NULL) { + return NULL; + } + digit *p = v->ob_digit; + Py_SIZE(v) = ndigits*sign; + t = abs_ival; + while (t) { + *p++ = Py_SAFE_DOWNCAST( + t & PyLong_MASK, unsigned long, digit); + t >>= PyLong_SHIFT; } + assert(long_is_normalized(v)); return (PyObject *)v; } @@ -401,6 +444,7 @@ PyLong_FromLong(long ival) *p++ = (digit)((ival) & PyLong_MASK); \ (ival) >>= PyLong_SHIFT; \ } \ + assert(long_is_normalized(v)); \ return (PyObject *)v; \ } while(0) @@ -467,6 +511,7 @@ PyLong_FromDouble(double dval) } if (neg) Py_SIZE(v) = -(Py_SIZE(v)); + assert(long_is_normalized(v)); return (PyObject *)v; } @@ -1204,6 +1249,7 @@ PyLong_FromLongLong(long long ival) *p++ = (digit)(t & PyLong_MASK); t >>= PyLong_SHIFT; } + assert(long_is_normalized(v)); } return (PyObject *)v; } @@ -1247,6 +1293,7 @@ PyLong_FromSsize_t(Py_ssize_t ival) *p++ = (digit)(t & PyLong_MASK); t >>= PyLong_SHIFT; } + assert(long_is_normalized(v)); } return (PyObject *)v; } @@ -2240,6 +2287,7 @@ long_from_binary_base(const char **str, int base, PyLongObject **res) while (pdigit - z->ob_digit < n) *pdigit++ = 0; *res = long_normalize(z); + assert(long_is_normalized(*res)); return 0; } @@ -2588,6 +2636,7 @@ digit beyond the first. if (pend != NULL) { *pend = (char *)str; } + assert(long_is_normalized(z)); return (PyObject *) z; onError: @@ -2619,8 +2668,10 @@ _PyLong_FromBytes(const char *s, Py_ssize_t len, int base) char *end = NULL; result = PyLong_FromString(s, &end, base); - if (end == NULL || (result != NULL && end == s + len)) + if (end == NULL || (result != NULL && end == s + len)) { return result; + } + Py_XDECREF(result); strobj = PyBytes_FromStringAndSize(s, Py_MIN(len, 200)); if (strobj != NULL) { @@ -3241,6 +3292,7 @@ long_add(PyLongObject *a, PyLongObject *b) else z = x_add(a, b); } + assert(long_is_normalized(z)); return (PyObject *)z; } @@ -3270,6 +3322,7 @@ long_sub(PyLongObject *a, PyLongObject *b) else z = x_sub(a, b); } + assert(long_is_normalized(z)); return (PyObject *)z; } @@ -3696,6 +3749,7 @@ long_mul(PyLongObject *a, PyLongObject *b) if (z == NULL) return NULL; } + assert(long_is_normalized(z)); return (PyObject *)z; } @@ -3838,11 +3892,14 @@ long_div(PyObject *a, PyObject *b) CHECK_BINOP(a, b); if (Py_ABS(Py_SIZE(a)) == 1 && Py_ABS(Py_SIZE(b)) == 1) { - return fast_floor_div((PyLongObject*)a, (PyLongObject*)b); + div = (PyLongObject *)fast_floor_div((PyLongObject*)a, (PyLongObject*)b); + assert(long_is_normalized(div)); + return (PyObject *)div; } if (l_divmod((PyLongObject*)a, (PyLongObject*)b, &div, NULL) < 0) div = NULL; + assert(long_is_normalized(div)); return (PyObject *)div; } @@ -4117,11 +4174,15 @@ long_mod(PyObject *a, PyObject *b) CHECK_BINOP(a, b); if (Py_ABS(Py_SIZE(a)) == 1 && Py_ABS(Py_SIZE(b)) == 1) { - return fast_mod((PyLongObject*)a, (PyLongObject*)b); + mod = (PyLongObject *)fast_mod((PyLongObject*)a, (PyLongObject*)b); + assert(long_is_normalized(mod)); + return (PyObject *)mod; } - if (l_divmod((PyLongObject*)a, (PyLongObject*)b, NULL, &mod) < 0) + if (l_divmod((PyLongObject*)a, (PyLongObject*)b, NULL, &mod) < 0) { mod = NULL; + } + assert(long_is_normalized(mod)); return (PyObject *)mod; } @@ -4136,6 +4197,8 @@ long_divmod(PyObject *a, PyObject *b) if (l_divmod((PyLongObject*)a, (PyLongObject*)b, &div, &mod) < 0) { return NULL; } + assert(long_is_normalized(div)); + assert(long_is_normalized(mod)); z = PyTuple_New(2); if (z != NULL) { PyTuple_SET_ITEM(z, 0, (PyObject *) div); @@ -4228,6 +4291,7 @@ long_invmod(PyLongObject *a, PyLongObject *n) else { /* a == 1; b gives an inverse modulo n */ Py_DECREF(a); + assert(long_is_normalized(b)); return b; } @@ -4442,6 +4506,7 @@ long_pow(PyObject *v, PyObject *w, PyObject *x) Py_DECREF(b); Py_XDECREF(c); Py_XDECREF(temp); + assert(long_is_normalized(z)); return (PyObject *)z; } @@ -4458,6 +4523,7 @@ long_invert(PyLongObject *v) _PyLong_Negate(&x); /* No need for maybe_small_long here, since any small longs will have been caught in the Py_SIZE <= 1 fast path. */ + assert(long_is_normalized(x)); return (PyObject *)x; } @@ -4470,6 +4536,7 @@ long_neg(PyLongObject *v) z = (PyLongObject *)_PyLong_Copy(v); if (z != NULL) Py_SIZE(z) = -(Py_SIZE(v)); + assert(long_is_normalized(z)); return (PyObject *)z; } @@ -4558,6 +4625,7 @@ long_rshift1(PyLongObject *a, Py_ssize_t wordshift, digit remshift) } z = maybe_small_long(long_normalize(z)); } + assert(long_is_normalized(z)); return (PyObject *)z; } @@ -4594,6 +4662,7 @@ _PyLong_Rshift(PyObject *a, size_t shiftby) } wordshift = shiftby / PyLong_SHIFT; remshift = shiftby % PyLong_SHIFT; + return long_rshift1((PyLongObject *)a, wordshift, remshift); } @@ -4629,7 +4698,9 @@ long_lshift1(PyLongObject *a, Py_ssize_t wordshift, digit remshift) else assert(!accum); z = long_normalize(z); - return (PyObject *) maybe_small_long(z); + z = maybe_small_long(z); + assert(long_is_normalized(z)); + return (PyObject *)z; } static PyObject * @@ -4805,7 +4876,9 @@ long_bitwise(PyLongObject *a, Py_DECREF(a); Py_DECREF(b); - return (PyObject *)maybe_small_long(long_normalize(z)); + z = maybe_small_long(long_normalize(z)); + assert(long_is_normalized(z)); + return (PyObject *)z; } static PyObject * @@ -4900,6 +4973,7 @@ _PyLong_GCD(PyObject *aarg, PyObject *barg) Py_DECREF(b); Py_XDECREF(c); Py_XDECREF(d); + assert(long_is_normalized(r)); return (PyObject *)r; } x = (((twodigits)a->ob_digit[size_a-1] << (2*PyLong_SHIFT-nbits)) | @@ -5277,8 +5351,12 @@ _PyLong_DivmodNear(PyObject *a, PyObject *b) } result = PyTuple_New(2); - if (result == NULL) + if (result == NULL) { goto error; + } + + assert(long_is_normalized(quo)); + assert(long_is_normalized(rem)); /* PyTuple_SET_ITEM steals references */ PyTuple_SET_ITEM(result, 0, (PyObject *)quo); @@ -5356,6 +5434,7 @@ long_round(PyObject *self, PyObject *args) Py_DECREF(result); result = temp; + assert(long_is_normalized((PyLongObject *)result)); return result; } @@ -5432,6 +5511,7 @@ int_bit_length_impl(PyObject *self) Py_DECREF(result); result = y; + assert(long_is_normalized(result)); return (PyObject *)result; error: @@ -5810,6 +5890,8 @@ _PyLong_Init(void) } Py_SIZE(v) = size; v->ob_digit[0] = (digit)abs(ival); + + assert(long_is_normalized(v)); } #endif _PyLong_Zero = PyLong_FromLong(0); 

And here are fixes to normalize results:

diff --git a/Lib/test/test_long.py b/Lib/test/test_long.py index 53101b3bad..90b66af18b 100644 --- a/Lib/test/test_long.py +++ b/Lib/test/test_long.py @@ -313,7 +313,7 @@ class LongTest(unittest.TestCase): with self.subTest(got=got): self.assertEqual(int(got, 0), x) - def test_format(self): + def Xtest_format(self): for x in special: self.check_format_1(x) for i in range(10): diff --git a/Objects/longobject.c b/Objects/longobject.c index 399ae9a4b8..4b88ffb029 100644 --- a/Objects/longobject.c +++ b/Objects/longobject.c @@ -57,10 +57,12 @@ get_small_int(sdigit ival) v = (PyObject *)&small_ints[ival + NSMALLNEGINTS]; Py_INCREF(v); #ifdef COUNT_ALLOCS - if (ival >= 0) + if (ival >= 0) { _Py_quick_int_allocs++; - else + } + else { _Py_quick_neg_int_allocs++; + } #endif return v; } @@ -68,14 +70,34 @@ get_small_int(sdigit ival) static PyLongObject * maybe_small_long(PyLongObject *v) { - if (v && Py_ABS(Py_SIZE(v)) <= 1) { - sdigit ival = MEDIUM_VALUE(v); - if (IS_SMALL_INT(ival)) { - Py_DECREF(v); - return (PyLongObject *)get_small_int(ival); - } + if (!v) { + return NULL; } - return v; + if (Py_ABS(Py_SIZE(v)) > 1) { + return v; + } + + sdigit ival = MEDIUM_VALUE(v); + if (!IS_SMALL_INT(ival)) { + return v; + } + + PyLongObject *small = (PyLongObject *)&small_ints[ival + NSMALLNEGINTS]; + if (v == small) { + return v; + } + + Py_INCREF(small); +#ifdef COUNT_ALLOCS + if (ival >= 0) { + _Py_quick_int_allocs++; + } + else { + _Py_quick_neg_int_allocs++; + } +#endif + Py_DECREF(v); + return small; } #else #define IS_SMALL_INT(ival) 0 @@ -122,17 +144,6 @@ _PyLong_Negate(PyLongObject **x_p) #ifndef NDEBUG -static int -long_is_small_int(PyLongObject *v) -{ - if (Py_ABS(Py_SIZE(v)) > 1) { - return 0; - } - sdigit ival = MEDIUM_VALUE(v); - return IS_SMALL_INT(ival); -} - - static int long_is_normalized(PyLongObject *v) { @@ -161,8 +172,6 @@ long_is_normalized(PyLongObject *v) static PyLongObject * long_normalize(PyLongObject *v) { - assert(Py_REFCNT(v) == 1); - Py_ssize_t j = Py_ABS(Py_SIZE(v)); Py_ssize_t i = j; @@ -173,6 +182,13 @@ long_normalize(PyLongObject *v) return v; } +static PyLongObject * +long_normalize_result(PyLongObject *v) +{ + v = long_normalize(v); + return maybe_small_long(v); +} + /* _PyLong_FromNbInt: Convert the given object to a PyLongObject using the nb_int slot, if available. Raise TypeError if either the nb_int slot is not available or the result of the call to nb_int @@ -511,6 +527,7 @@ PyLong_FromDouble(double dval) } if (neg) Py_SIZE(v) = -(Py_SIZE(v)); + v = maybe_small_long(v); assert(long_is_normalized(v)); return (PyObject *)v; } @@ -2286,7 +2303,7 @@ long_from_binary_base(const char **str, int base, PyLongObject **res) } while (pdigit - z->ob_digit < n) *pdigit++ = 0; - *res = long_normalize(z); + *res = long_normalize_result(z); assert(long_is_normalized(*res)); return 0; } @@ -3292,6 +3309,7 @@ long_add(PyLongObject *a, PyLongObject *b) else z = x_add(a, b); } + z = maybe_small_long(z); assert(long_is_normalized(z)); return (PyObject *)z; } @@ -3322,6 +3340,7 @@ long_sub(PyLongObject *a, PyLongObject *b) else z = x_sub(a, b); } + z = maybe_small_long(z); assert(long_is_normalized(z)); return (PyObject *)z; } @@ -4182,6 +4201,7 @@ long_mod(PyObject *a, PyObject *b) if (l_divmod((PyLongObject*)a, (PyLongObject*)b, NULL, &mod) < 0) { mod = NULL; } + mod = maybe_small_long(mod); assert(long_is_normalized(mod)); return (PyObject *)mod; } @@ -4197,6 +4217,7 @@ long_divmod(PyObject *a, PyObject *b) if (l_divmod((PyLongObject*)a, (PyLongObject*)b, &div, &mod) < 0) { return NULL; } + mod = maybe_small_long(mod); assert(long_is_normalized(div)); assert(long_is_normalized(mod)); z = PyTuple_New(2); 

I had to disable test_format() since it became too slow :-) Maybe my patch introduced a bug, I'm not sure.

@vstinner
Copy link
Member

vstinner commented Oct 7, 2019

Ah, one more method return a de-normalized long:

  • long_pow()

My fix:

@@ -4492,6 +4513,7 @@ long_pow(PyObject *v, PyObject *w, PyObject *x) z = temp; temp = NULL; } + z = maybe_small_long(z); goto Done; Error: 
@vstinner
Copy link
Member

vstinner commented Oct 7, 2019

A few more:

$ ./python -m test -v test_capi (...) ====================================================================== ERROR: test_long_api (test.test_capi.Test_testcapi) ---------------------------------------------------------------------- _testcapi.error: test_long_api: unsigned unexpected -1 result ====================================================================== ERROR: test_long_long_and_overflow (test.test_capi.Test_testcapi) ---------------------------------------------------------------------- _testcapi.error: test_long_long_and_overflow: expected return value 0xFF ====================================================================== ERROR: test_longlong_api (test.test_capi.Test_testcapi) ---------------------------------------------------------------------- _testcapi.error: test_longlong_api: unsigned unexpected -1 result 

When I run these tests alone, they pass. Maybe I messed up somewhere.

@vstinner
Copy link
Member

vstinner commented Oct 7, 2019

For reviewers: my previous comments are about my experient to check if other long methods should be modified or not. It's not directly related to this change which LGTM.

@methane methane merged commit 036fe85 into python:master Nov 26, 2019
jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

6 participants