GH-101291: Rearrange the size bits in PyLongObject #102464

markshannon · 2023-03-06T09:57:00Z

This PR rearranges the bits in what was ob_size, to slightly speedup the most common operations and to prepare for storing the tagged 2-complement value directly in a future PR.

The new layout is as follows:

Bits 0 and 1: 1 - sign. I.e. 0 for positive numbers, 1 for zero and 2 for negative numbers.
Bit 2 reserved (probably for the immortal bit)
Bits 3+ the unsigned size.

The bulk of the change is removing all the uses of Py_SIZE and Py_SETSIZE, and replacing them with a new set of inline functions.
It disturbs me how much we use unchecked casts, but that's a separate issue...

This will, inevitably, break Cython generated code again.

Performance measurement shows no significant change: https://github.com/faster-cpython/benchmarking/tree/main/results/bm-20230302-3.12.0a5%2B-ce6bfb2

Issue: Restore (or beat) Python 2 performance for arithmetic operations on ints that fit into a single word #101291

…tCount().

…Long_SignedDigitCount which might not be optimal, but is safe.

…header file.

…ion of immortal ints and tagged medium ints.

gvanrossum

As my flight might depart soon here's a first batch of review comments. Still to do longobject.c, and some modules.

gvanrossum · 2023-03-16T00:28:43Z

Tools/build/umarshal.py

+        if not (0 <= n <= self.end - self.pos):
+            print(n, self.end, self.pos)


Remove debug print()?

Suggested change

if not (0 <= n <= self.end - self.pos):

print(n, self.end, self.pos)

gvanrossum · 2023-03-16T00:34:48Z

Misc/NEWS.d/next/Core and Builtins/2023-03-06-10-02-22.gh-issue-101291.0FT2QS.rst

@@ -0,0 +1,7 @@
+Rearrage bits in first field (after header) of PyLongObject. * Bits 0 and 1:
+1- sign. I.e. 0 for positive numbers, 1 for zero and 2 for negative numbers.


Use consistent spacing around binary -.

Suggested change

1- sign. I.e. 0 for positive numbers, 1 for zero and 2 for negative numbers.

1 - sign. I.e. 0 for positive numbers, 1 for zero and 2 for negative numbers.

gvanrossum · 2023-03-17T19:04:02Z

Include/internal/pycore_long.h

+    return (a->long_value.lv_tag | b->long_value.lv_tag) < (2 << NON_SIZE_BITS);
+}
+
+/* The value returned by this function will have at least one bit to spare,


"one bit to spare" feels ambiguous, since the return type is signed -- is the spare bit the sign bit, or should there be at least one additional spare bit? (I know in practice we have 4 spare bits including the sign, but still, I'm not sure whether a 63-bit digit would be acceptable or not, from this description (or others).)

gvanrossum · 2023-03-17T19:12:13Z

Include/internal/pycore_long.h

+static inline bool
+_PyLong_IsPositive(const PyLongObject *op)
+{
+    return (op->long_value.lv_tag & SIGN_MASK) == 0;


Why not have #define SIGN_POSITIVE 0?

I want these functions to be the only way to determine the sign.
Defining SIGN_POSITIVE will just encourage people to do the test elsewhere.

Sure, fine. Next question: maybe we also need a _PyLong_IsNonZero? I see !_PyLong_IsZero a lot, and the ! is easily missed. (Or maybe that's just my old eyes.) Possibly also IsNonNegative and IsNonPositive.

gvanrossum · 2023-03-17T19:13:54Z

Include/internal/pycore_long.h

+    return op->long_value.lv_tag >> NON_SIZE_BITS;
+}
+
+/* Equivalent to _PyLong_DigitCount(op) * _PyLong_NonZeroSign(op) */


I take it this is for algorithms where the old "signed size" representation worked well?

It is for code that uses the "signed size" representation.
I make no judgement as to how well it works 🙂

gvanrossum · 2023-03-17T19:19:10Z

Include/internal/pycore_long.h

+    return (a->long_value.lv_tag & SIGN_MASK) == (b->long_value.lv_tag & SIGN_MASK);
+}
+
+#define TAG_FROM_SIGN_AND_SIZE(sign, size) ((1 - (sign)) | ((size) << NON_SIZE_BITS))


So maybe add a comment that this macro should only be used with literal or size_t arguments?

gvanrossum · 2023-03-17T19:22:49Z

Include/internal/pycore_long.h

+#define TAG_FROM_SIGN_AND_SIZE(sign, size) ((1 - (sign)) | ((size) << NON_SIZE_BITS))
+
+static inline void
+_PyLong_SetSignAndSize(PyLongObject *op, int sign, Py_ssize_t size)


Shouldn't this use DigitCount instead of Size, for consistency with earlier APIs? Same for the next one.

gvanrossum · 2023-03-17T19:25:37Z

Include/internal/pycore_long.h

+static inline void
+_PyLong_FlipSign(PyLongObject *op) {
+    unsigned int flipped_sign = 2 - (op->long_value.lv_tag & SIGN_MASK);
+    op->long_value.lv_tag &= ~7;


I think you want to use some defined name instead of hardcoding 7? Perhaps ~((1 << NON_SIZE_BITS) - 1).

gvanrossum · 2023-03-17T19:36:24Z

Python/bltinmodule.c

                return PyLong_FromLong(i_result);
            }
            if (PyLong_CheckExact(item) || PyBool_Check(item)) {


The GitHub warning is at the wrong line, it applies to the PyLong_FromLong(i_result) two lines up. It does seem to warrant some attention. Similar below.

gvanrossum · 2023-03-17T19:40:54Z

Python/ast_opt.c

@@ -152,7 +153,9 @@ check_complexity(PyObject *obj, Py_ssize_t limit)
 static PyObject *
 safe_multiply(PyObject *v, PyObject *w)
 {
-    if (PyLong_Check(v) && PyLong_Check(w) && Py_SIZE(v) && Py_SIZE(w)) {
+    if (PyLong_Check(v) && PyLong_Check(w) &&
+        !_PyLong_IsZero((PyLongObject *)v) && !_PyLong_IsZero((PyLongObject *)w)


Maybe we need another convenience macro IsNonZero.

lpereira · 2023-03-21T19:28:41Z

Include/internal/pycore_long.h

+static inline int
+_PyLong_CompactSign(const PyLongObject *op)
+{
+    assert(PyLong_Check(op));
+    assert(_PyLong_IsCompact(op));
+    return 1 - (op->long_value.lv_tag & SIGN_MASK);
+}
+


Shouldn't this be the new implementation of _PyLong_Sign(), if _PyLong_NonCompactSign() is removed? This gets rid of a branch in the proposed version of _PyLong_Sign().

They sure look identical to me. Maybe Mark has plans and maybe the compiler would optimize this anyway?

if (P(x)) return F(x); else return F(x);

could just become return F(x);.

We want the freedom to implement the "compact" and non-compact forms differently.
They have the same implementation at the moment, but that will change.

_PyLong_Sign() is part of the ABI, so we need to retain it. But almost all code using _PyLong_Sign() actually wants to know if an int is negative and should be using _PyLong_IsNegative().

gvanrossum

Here's the rest. I went over every diff chunk in longobject.c. Let's get this merged...

gvanrossum · 2023-03-21T20:06:32Z

Misc/NEWS.d/next/Core and Builtins/2023-03-06-10-02-22.gh-issue-101291.0FT2QS.rst

+Rearrage bits in first field (after header) of PyLongObject. * Bits 0 and 1:
+1 - sign. I.e. 0 for positive numbers, 1 for zero and 2 for negative numbers.
+* Bit 2 reserved (probably for the immortal bit) * Bits 3+ the unsigned
+size.


Format as bullets?

Suggested change

Rearrage bits in first field (after header) of PyLongObject. * Bits 0 and 1:

1 - sign. I.e. 0 for positive numbers, 1 for zero and 2 for negative numbers.

* Bit 2 reserved (probably for the immortal bit) * Bits 3+ the unsigned

size.

Rearrage bits in first field (after header) of PyLongObject:

* Bits 0 and 1: 1 - sign. I.e. 0 for positive numbers, 1 for zero and 2 for negative numbers.

* Bit 2 reserved (probably for the immortal bit).

* Bits 3+ the unsigned size.

Fixed. I suspect it got reformatted by something.

gvanrossum · 2023-03-21T20:16:22Z

Modules/_tkinter.c

+    assert(PyLong_Check(value));
+    neg = _PyLong_IsNegative((PyLongObject *)value);


Please put the blank line back.

Suggested change

assert(PyLong_Check(value));

neg = _PyLong_IsNegative((PyLongObject *)value);

assert(PyLong_Check(value));

neg = _PyLong_IsNegative((PyLongObject *)value);

gvanrossum · 2023-03-21T20:46:37Z

Include/internal/pycore_long.h

+
+/* Like _PyLong_DigitCount but asserts that op is non-negative */
+static inline Py_ssize_t
+_PyLong_UnsignedDigitCount(const PyLongObject *op)


I'm not excited about this name; I keep having to look up how it differs from _PyLong_DigitCount, and it's not really related to _PyLong_SignedDigitCount. :-( Maybe _PyLong_NonNegativeDigitCount? Or perhaps better _PyLong_DigitCountOfNonNegative?

I needed this for the extra check during implementation. _PyLong_UnsignedDigitCount is now the same as _PyLong_DigitCount, and should remain so.

I'll remove it.

gvanrossum · 2023-03-21T21:03:50Z

Objects/longobject.c

+            * care (see comment above).
+            */


Accidental reformat?

Suggested change

* care (see comment above).

*/

* care (see comment above).

*/

gvanrossum · 2023-03-21T21:16:34Z

Objects/longobject.c

@@ -4839,7 +4788,7 @@ long_pow(PyObject *v, PyObject *w, PyObject *x)
            pending = 0; \
        } while(0)

-        for (i = Py_SIZE(b) - 1; i >= 0; --i) {
+        for (i = _PyLong_SignedDigitCount(b) - 1; i >= 0; --i) {


Mybe we can prove that b is nonnegative here?

Maybe.
I'm not going to change any algorithms in this PR.

Hopefully, the more explicit semantics of the new API will allow someone to make some improvements in a future PR.

gvanrossum · 2023-03-21T21:51:12Z

Objects/longobject.c

@@ -3740,7 +3690,7 @@ k_mul(PyLongObject *a, PyLongObject *b)
    /* Split a & b into hi & lo pieces. */
    shift = bsize >> 1;
    if (kmul_split(a, shift, &ah, &al) < 0) goto fail;
-    assert(Py_SIZE(ah) > 0);            /* the split isn't degenerate */
+    assert(_PyLong_UnsignedDigitCount(ah) > 0);            /* the split isn't degenerate */


This should just check the sign, right?

Suggested change

assert(_PyLong_UnsignedDigitCount(ah) > 0); /* the split isn't degenerate */

assert(_PyLong_IsPositive(ah)); /* the split isn't degenerate */

Same below several occurrences.

Objects/longobject.c

gvanrossum · 2023-03-21T22:10:57Z

Include/internal/pycore_long.h

+static inline bool
+_PyLong_IsPositive(const PyLongObject *op)
+{
+    return (op->long_value.lv_tag & SIGN_MASK) == 0;


Sure, fine. Next question: maybe we also need a _PyLong_IsNonZero? I see !_PyLong_IsZero a lot, and the ! is easily missed. (Or maybe that's just my old eyes.) Possibly also IsNonNegative and IsNonPositive.

gvanrossum · 2023-03-21T22:19:15Z

Objects/longobject.c

    Py_ssize_t i;

    z = _PyLong_New(size_a + size_b);
    if (z == NULL)
        return NULL;

-    memset(z->long_value.ob_digit, 0, Py_SIZE(z) * sizeof(digit));
+    memset(z->long_value.ob_digit, 0, _PyLong_UnsignedDigitCount(z) * sizeof(digit));


Since z was just created with a nonnegative size:

Suggested change

memset(z->long_value.ob_digit, 0, _PyLong_UnsignedDigitCount(z) * sizeof(digit));

memset(z->long_value.ob_digit, 0, _PyLong_DigitCount(z) * sizeof(digit));

gvanrossum

Go for it!

sobolevn · 2023-03-23T09:25:26Z

I've opened #102940 to fix two new warnings from this PR :)

…2464) * Eliminate all remaining uses of Py_SIZE and Py_SET_SIZE on PyLongObject, adding asserts. * Change layout of size/sign bits in longobject to support future addition of immortal ints and tagged medium ints. * Add functions to hide some internals of long object, and for setting sign and digit count. * Replace uses of IS_MEDIUM_VALUE macro with _PyLong_IsCompact().

scoder · 2023-03-28T06:01:51Z

ISTM that the simple accessor functions like IsZero, IsPositive, IsNegative should be publicly available.

What about this part? Looks like it was dropped on the floor along the way.

) See python/cpython#102464

eduardo-elizondo · 2023-04-07T13:00:00Z

Include/internal/pycore_long.h

+ * 0-1: Sign bits value = (1-sign), ie. negative=2, positive=0, zero=1.
+ * 2: Reserved for immortality bit


I don't think we need an immortality flag here, but we do need a static flag (immortality should be marked by the refcount and this marks if the object is static or not. Using this, we can do the static check at dealloc time to prevent the deallocation of the objects

eduardo-elizondo · 2023-04-07T14:02:17Z

Include/internal/pycore_long.h

+static inline int
+_PyLong_IsNonNegativeCompact(const PyLongObject* op) {
+    assert(PyLong_Check(op));
+    return op->long_value.lv_tag <= (1 << NON_SIZE_BITS);


This doesn't work if we set the second (immortal/static) bit, i.e: the immortal small int 1 since it will have an lv_tag of 1100 and return an incorrect value here.

I'll create a new PR to restructure this a bit to make it work with the new bit flag.

cc @ericsnowcurrently

…2464) * Eliminate all remaining uses of Py_SIZE and Py_SET_SIZE on PyLongObject, adding asserts. * Change layout of size/sign bits in longobject to support future addition of immortal ints and tagged medium ints. * Add functions to hide some internals of long object, and for setting sign and digit count. * Replace uses of IS_MEDIUM_VALUE macro with _PyLong_IsCompact().

verhovsky · 2023-04-29T13:10:25Z

Include/cpython/longintrepr.h

@@ -80,7 +80,7 @@ typedef long stwodigits; /* signed variant of twodigits */
 */


You didn't update this comment that documents _longobject, it's still talking about ob_size and PyVarObject

/* Long integer representation. The absolute value of a number is equal to SUM(for i=0 through abs(ob_size)-1) ob_digit[i] * 2**(SHIFT*i)

markshannon added 20 commits February 28, 2023 11:59

Add functions to hide some internals of long object.

0ec07e4

Add internal functions to longobject.c for setting sign and digit count.

292b9d0

Replace Py_SIZE(x) < 0 with _PyLong_IsNegative(x) in longobject.c

5c54894

Replace Py_ABS(Py_SIZE(a)) with _PyLong_DigitCount(a) in longobject.c

029aaa4

Remove many uses of Py_SIZE in longobject.c

b56e6da

Remove _PyLong_AssignValue, as it is no longer used.

91269fc

Remove some more uses of Py_SIZE in longobject.c.

c48e825

Remove a few more uses of Py_SIZE in longobject.c.

449c0e2

Remove some more uses of Py_SIZE, replacing with _PyLong_UnsignedDigi…

c5ba601

…tCount().

Replace a few Py_SIZE() with _PyLong_SameSign().

4b3a3e8

Remove a few more Py_SIZE() from longobject.c

9ef9d2c

Replace uses of IS_MEDIUM_VALUE macro with _PyLong_IsSingleDigit.

9c408c1

Remove most of the remaining uses of Py_SIZE in longobject.c

548d656

Replace last remaining uses of Py_SIZE applied to longobject with _Py…

3e3fefd

…Long_SignedDigitCount which might not be optimal, but is safe.

Don't use _PyObject_InitVar and move a couple of inline functions to …

391fb51

…header file.

Correct name of inline function.

df8c7d3

Eliminate all remaining uses of Py_SIZE and Py_SET_SIZE on PyLongObject.

bc14fa6

Change layout of size/sign bits in longobject to support future addit…

54c6f1b

…ion of immortal ints and tagged medium ints.

Test pairs of longs together on fast path of add/mul/sub.

ce6bfb2

Tidy up comment and delete commented out code.

4c1956b

markshannon requested review from rhettinger, tiran and isidentical as code owners March 6, 2023 09:57

bedevere-bot mentioned this pull request Mar 6, 2023

Restore (or beat) Python 2 performance for arithmetic operations on ints that fit into a single word #101291

Open

bedevere-bot added the awaiting core review label Mar 6, 2023

corona10 requested a review from mdickinson March 6, 2023 10:01

markshannon added 4 commits March 6, 2023 10:02

Add news.

301158b

Remove debugging asserts.

1aa1891

Fix storage classes.

bf2a9af

Remove development debug functions.

169f521

markshannon added 5 commits March 16, 2023 18:39

Merge branch 'main' into long-rearrange-size-bits

b06bb6f

Merge branch 'main' into long-rearrange-size-bits

a19b0a7

Fix _PyLong_Sign

87f49b2

Replace _PyLong_Sign(x) < 0 with _PyLong_IsNegative(x).

f764aa8

fix sign check

9843ac0

gvanrossum reviewed Mar 17, 2023

View reviewed changes

markshannon mentioned this pull request Mar 18, 2023

gh-102509: Start initializing ob_digit of _PyLongValue #102510

Merged

lpereira reviewed Mar 21, 2023

View reviewed changes

gvanrossum reviewed Mar 21, 2023

View reviewed changes

markshannon added 2 commits March 22, 2023 11:23

Address some review comments.

d6cb917

Change asserts on digit counts to asserts on sign where applicable.

469d26f

gvanrossum approved these changes Mar 22, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Mar 22, 2023

markshannon merged commit 7559f5f into python:main Mar 22, 2023

bedevere-bot removed the awaiting merge label Mar 22, 2023

markshannon deleted the long-rearrange-size-bits branch March 22, 2023 14:50

scoder mentioned this pull request Apr 3, 2023

Implement support for the new PyLong struct layout in Py3.12a7 cython/cython#5353

Merged

gvanrossum mentioned this pull request Apr 3, 2023

gh-84436: Implement Immortal Objects #19474

Merged

scoder added a commit to cython/cython that referenced this pull request Apr 5, 2023

Implement support for the new PyLong struct layout in Py3.12a7. (GH-5353

781b087

) See python/cpython#102464

eduardo-elizondo reviewed Apr 7, 2023

View reviewed changes

skirpichev mentioned this pull request Apr 24, 2023

Breaking changes in Python 3.12.0a7 release aleaxit/gmpy#405

Closed

verhovsky reviewed Apr 29, 2023

View reviewed changes

carljm mentioned this pull request May 5, 2023

build fails with --enable-pystats --with-pydebug (use of Py_SIZE on PyLongObject) #104184

Closed

eduardo-elizondo mentioned this pull request Jun 19, 2023

gh-84436: Add static flag in PyLongObject's lv_tag #103403

Closed

		if not (0 <= n <= self.end - self.pos):
		print(n, self.end, self.pos)

		@@ -0,0 +1,7 @@
		Rearrage bits in first field (after header) of PyLongObject. * Bits 0 and 1:
		1- sign. I.e. 0 for positive numbers, 1 for zero and 2 for negative numbers.

	1- sign. I.e. 0 for positive numbers, 1 for zero and 2 for negative numbers.
	1 - sign. I.e. 0 for positive numbers, 1 for zero and 2 for negative numbers.

-Rearrage bits in first field (after header) of PyLongObject. * Bits 0 and 1:
-- sign. I.e. 0 for positive numbers, 1 for zero and 2 for negative numbers.
-* Bit 2 reserved (probably for the immortal bit) * Bits 3+ the unsigned
-size.
+Rearrage bits in first field (after header) of PyLongObject:
+* Bits 0 and 1: 1 - sign. I.e. 0 for positive numbers, 1 for zero and 2 for negative numbers.
+* Bit 2 reserved (probably for the immortal bit).
+* Bits 3+ the unsigned size.

		assert(PyLong_Check(value));
		neg = _PyLong_IsNegative((PyLongObject *)value);

	assert(_PyLong_UnsignedDigitCount(ah) > 0); /* the split isn't degenerate */
	assert(_PyLong_IsPositive(ah)); /* the split isn't degenerate */

	memset(z->long_value.ob_digit, 0, _PyLong_UnsignedDigitCount(z) * sizeof(digit));
	memset(z->long_value.ob_digit, 0, _PyLong_DigitCount(z) * sizeof(digit));

		* 0-1: Sign bits value = (1-sign), ie. negative=2, positive=0, zero=1.
		* 2: Reserved for immortality bit

		@@ -80,7 +80,7 @@ typedef long stwodigits; /* signed variant of twodigits */
		*/

GH-101291: Rearrange the size bits in PyLongObject #102464

GH-101291: Rearrange the size bits in PyLongObject #102464

Conversation

markshannon commented Mar 6, 2023 • edited by gvanrossum Loading

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum left a comment

Choose a reason for hiding this comment

sobolevn commented Mar 23, 2023

scoder commented Mar 28, 2023

eduardo-elizondo Apr 7, 2023 • edited Loading

Choose a reason for hiding this comment

eduardo-elizondo Apr 7, 2023 • edited Loading

Choose a reason for hiding this comment

verhovsky Apr 29, 2023 • edited Loading

Choose a reason for hiding this comment

markshannon commented Mar 6, 2023 •

edited by gvanrossum

Loading

eduardo-elizondo Apr 7, 2023 •

edited

Loading

eduardo-elizondo Apr 7, 2023 •

edited

Loading

verhovsky Apr 29, 2023 •

edited

Loading