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

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


Remove debug print()?
Suggested change
ifnot (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

+staticinlinebool
+_PyLong_IsPositive(constPyLongObject*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

+returnop->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);
+}
+
+#defineTAG_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

+#defineTAG_FROM_SIGN_AND_SIZE(sign, size) ((1 - (sign)) | ((size) << NON_SIZE_BITS))
+
+staticinlinevoid
+_PyLong_SetSignAndSize(PyLongObject*op, intsign, Py_ssize_tsize)


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

+staticinlinevoid
+_PyLong_FlipSign(PyLongObject*op){
+unsigned intflipped_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

 returnPyLong_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

 {
-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

+staticinlineint
+_PyLong_CompactSign(constPyLongObject*op)
+{
+assert(PyLong_Check(op));
+assert(_PyLong_IsCompact(op));
+return1- (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 */
+staticinlinePy_ssize_t
+_PyLong_UnsignedDigitCount(constPyLongObject*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 (seecommentabove).
*/
*care (seecommentabove).
*/

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

Objects/longobject.c

 } 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

 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

+staticinlinebool
+_PyLong_IsPositive(constPyLongObject*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

 returnNULL;

-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

+staticinlineint
+_PyLong_IsNonNegativeCompact(constPyLongObject*op){
+assert(PyLong_Check(op));
+returnop->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


 CAUTION: Generic code manipulating subtypes of PyVarObject has to
 aware that ints abuse ob_size's sign bit.
 */


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 isidentical, rhettinger and tiran 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 2 commits March 16, 2023 19:26

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:mainMar 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 gmpy2/gmpy2#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

chris-eibl mentioned this pull request Dec 7, 2024
gh-127119: Faster check for small ints in long_dealloc #127620
Merged

eendebakpt mentioned this pull request Dec 7, 2024
gh-127119: Remove check on accidental deallocation of immortal objects for free-threaded build #127120
Closed

skirpichev mentioned this pull request Apr 25, 2025
sum() several times slower on Python 3 64-bit #68264
Open

		ifnot (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.

-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);

-*care (seecommentabove).
-*/
+*care (seecommentabove).
+*/

	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

Uh oh!

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

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

Uh oh!

Conversation

markshannon commented Mar 6, 2023• edited by gvanrossum Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

sobolevn commented Mar 23, 2023

Uh oh!

scoder commented Mar 28, 2023

markshannon commented Mar 6, 2023•
edited by gvanrossum
Loading

eduardo-elizondoApr 7, 2023•
edited
Loading

eduardo-elizondoApr 7, 2023•
edited
Loading

verhovskyApr 29, 2023•
edited
Loading