draft: Store integers in ob_size field of PyLongObjects #31595

lpereira · 2022-02-26T03:10:15Z

Changes the representation of long objects so that the ob_digit field isn't used for integers that would fit in the ob_size field. This reduces the amount of memory used by long objects, and increases the range of calculations that can be performed directly by the CPU, without requiring our big num implementation.

This is my go at implementing this optimization suggested by @markshannon.

The way this is done is by reserving one bit (the least significant bit) from the ob_size field as a flag that tells if we're using the inlined representation or not.

If the bit is set, the value is inlined; shifting right by one yields an integer that can be used directly. On 64-bit architectures, this means that values are effectively 62-bit, as one is still used for the signal. On 32-bit architectures, this is 30 bit, the same as the largest digit size we support currently.

If the bit is unset, the value is stored as a big num representation: the sign bit is still the sign, and the absolute value is the number of 15- or 30-bit digits in a PyLong object.

More details can be found in longintrepr.h.

This is not finished yet, and I'm posting the code as-is to gather feedback before going any further.

This implements _Py_bit_length() in function of either _Py_bit_length32() or _Py_bit_length64(). This is needed for some of the work in inlining integers that would fit in a PyVarObject's ob_size field. Also cleans up the implementation slightly.

This uses the GCC/Clang builtin to check if either a 32-bit or a 64-bit addition would overflow, and a fast software-only check everywhere else.

This uses the GCC/Clang builtin to check if either a 32-bit or a 64-bit subtraction would underflow, and a fast software-only check everywhere else.

…rflow This uses the GCC/Clang builtin to check if either a 32-bit or a 64-bit multiplication would overflow, and a fast software-only check everywhere else.

This tries to use GCC/Clang builtins when available, and falls back to using the 32-bit popcount twice.

Changes the representation of long objects so that the ob_digit field isn't used for integers that would fit in the ob_size field. This reduces the amount of memory used by long objects, and increases the range of calculations that can be performed directly by the CPU, without requiring our big num implementation. The way this is done is by reserving one bit (the least significant bit) from the ob_size field as a flag that tells if we're using the inlined representation or not. If the bit is set, the value is inlined; shifting right by one yields an integer that can be used directly. On 64-bit architectures, this means that values are effectively 62-bit, as one is still used for the signal. On 32-bit architectures, this is 30 bit, the same as the largest digit size we support currently. If the bit is unset, the value is stored as a big num representation: the sign bit is still the sign, and the absolute value is the number of 15- or 30-bit digits in a PyLong object. More details can be found in longintrepr.h. This is not finished yet, and I'm posting the code as-is to gather feedback before going any further.

Objects/longobject.c

gvanrossum

I tried to compile and found a bunch of trivial errors. Mind fixing those?

gvanrossum · 2022-02-26T16:58:37Z

Include/cpython/longintrepr.h

+       * These integers have a capacity of 62bits on 64-bit architectures:
+         one bit for the "is inlined" flag, and one sign bit.  This is 30 bits on
+         32-bit architectures (for the same reasons).


Shouldn't that be called a capacity of 63 (or 31) bits? When we use the full 64-bit word we don't say that the capacity is 63 bits plus sign, we usually just say "64-bit signed integers".

gvanrossum · 2022-02-26T21:49:33Z

Include/internal/pycore_bitutils.h

+    if (sizeof(long) == sizeof(uint64_t)) {
+        return _Py_popcount64(x);
+    }
+    _Py_UNREACHABLE();


The actual macro has no leading _.

gvanrossum · 2022-02-26T21:55:35Z

Include/internal/pycore_bitutils.h

-        return 0;
-    }
+    // __builtin_clzl() is undefined for x = 0.
+    Py_BUILT_ASSERT(sizeof(long) <= sizeof(uint32_t));


Did you mean >=?

gvanrossum · 2022-02-26T22:30:15Z

Include/internal/pycore_bitutils.h

+
+static inline int _Py_bit_length(unsigned long x)
+{
+    _Py_BUILD_ASSERT(sizeof(x) == sizeof(uint32_t) || sizeof(x) == sizeof(uint64_t));


No leading _.

gvanrossum · 2022-02-26T22:33:43Z

Include/internal/pycore_bitutils.h

+    _Py_UNREACHABLE();
+}
+
+static inline bool _Py_add_overflow32(int32_t a, int32_t b, int32_t *result)


This file needs #include <stdbool.h> somewhere near the top (say at line 19).

(Also our C style guide requires a line break between the return type and the function name.)

gvanrossum · 2022-02-26T22:36:48Z

Objects/longobject.c

-    assert(IS_MEDIUM_VALUE(x));
-    return ((stwodigits)Py_SIZE(x)) * x->ob_digit[0];
+    /* 1 bit for the sign bit, 1 bit for the "is inlined or bignum" flag */
+    return (Py_size_t)size < 1UL<<(sizeof(size) * CHAR_BIT - 2);


There is no Py_size_t -- just use size_t.

gvanrossum · 2022-02-26T22:44:13Z

Objects/longobject.c

+Py_LOCAL_INLINE(bool)
+as_inlined_int(const PyLongObject *x, Py_ssize_t *value)
+{
+    *value = (Py_ssize_t)((Py_size_t)x->ob_size >> 1UL);


Separately, x->ob_size doesn't exist; use Py_SIZE(x).

gvanrossum · 2022-02-26T22:45:58Z

Objects/longobject.c

            Py_DECREF(v);
-            return (PyLongObject *)get_small_int((sdigit)ival);
+            return get_small_int(value);


Need (PyLongObject *) cast.

gvanrossum · 2022-02-26T22:47:02Z

Objects/longobject.c

+     * checks that value is larger than MAX_LONG_DIGITS. */
+    memcpy(result->ob_digit, src->ob_digit, sizeof(sdigit) * value);
+
+    return result;


Need (PyObject *) cast.

A lot of these little things will be ironed out whenever the compiler yells at me. I still haven't compiled this, so I'm sure I'll hear a lot whenever I'm ready to do that.

Well, just understand that I find it easier to review when there aren't little details that could be fixed by trying to compile and fixing what it finds. :-)

gvanrossum · 2022-02-26T22:49:11Z

Objects/longobject.c

+        }
+    } else {
+        assert(!is_inlined);
+        *x_p = (PyLongObject *)_PyLongFromSignedSize(-value);


_PyLong_FromSignedSize (_ after _PyLong) here and a few times below.

gvanrossum

Couple more. (I'm sending these as I come across them, I'm reviewing this in dribs and drabs between other activities. :-)

gvanrossum · 2022-02-26T22:59:52Z

Include/cpython/longintrepr.h

+/* Long Integer Representation
+   ---------------------------
+
+   There are two representations of long objects: the inlined


Technically the optimized representation isn't "inlined" -- I would think that that term might be reserved for a version using tagged pointer values.

I'll change this to "small num" and "big num", or something like that, to make this clearer.

gvanrossum · 2022-02-26T23:00:29Z

Include/cpython/longintrepr.h

+
+   For inlined longs, their value can be obtained with this expression:
+
+        ob_size >> 1


Also document the macro one is suppsed to use. :-)

gvanrossum · 2022-02-26T23:13:23Z

Include/cpython/longintrepr.h

+       * These numbers can also be normalized.  In a normalized number,
+         ob_digit[abs(ob_size)-1] (the most significant digit) is never zero.


Isn't there also a stronger guarantee that when the object leaves longobject.c it is always normalized? (I.e. unnormalized longs only exist as intermediary results.)

gvanrossum · 2022-02-26T23:14:01Z

Include/cpython/longintrepr.h

+   aware that ints abuse  ob_size's sign bit and its least significant
+   bit.


I'd rather say that they abuse the ob_size value. :-)

gvanrossum · 2022-02-26T23:22:14Z

Include/internal/pycore_bitutils.h

+    int upper_bits = _Py_bit_length32((uint32_t)(x >> 32));
+    int lower_bits = _Py_bit_length32((uint32_t)x);
+    return upper_bits + lower_bits;


That doesn't look right? It should use the bit length of the upper bits, plus 32, if the upper bits are nonzero, else the bit length of the lower bits, or something like that.

(I wonder if there should be a mode where you don't use the GCC/clang/MSVC versions for any of these functions, to test that the fallback versions are correct.)

You're right. Good catch. I was really tired when I wrote this code and ended up not testing/proving this is correct.

gvanrossum

Flushing a few comments I had pending. I will wait with more review activities until you've decided how to address the issues Mark brought up. (If you want me to review more, please point me to something specific and I'll take a look, of course.)

gvanrossum · 2022-02-27T00:40:14Z

Include/internal/pycore_bitutils.h

+    _Py_UNREACHABLE();
+}
+
+static inline bool _Py_add_overflow32(int32_t a, int32_t b, int32_t *result)


(Also our C style guide requires a line break between the return type and the function name.)

gvanrossum · 2022-02-27T00:53:10Z

Objects/longobject.c

+    Py_ssize_t value;
+    (void)as_inlined_int(x, &value);
+    return value == 0;


Couldn't this just check whether the size is zero? I.e. return Py_SIZE(x) == 0;

(Same for is_negative() below.)

I don't see that working, because the LSB is used as a flag to distinguish between inlined numbers and big numbers.

What could be done, though, to avoid shifting, is doing a unsigned, lesser-than-or-equal-to comparison against 1ULL. If the size is either 1 (inlined number with LSB set) or 0 (big number with LSB unset), then the number is certainly zero.

gvanrossum · 2022-02-27T00:55:45Z

Objects/longobject.c

+Py_LOCAL_INLINE(Py_ssize_t)
+encode_size(Py_ssize_t value)
+{
+    assert(!fits_in_size_field(value));


Isn't this assert reversed? Assuming the argument is the number of digits, it had better fit in the size field. :-)

No, this is used to encode the number of digits for big nums. It's not supposed to be used when we're encoding the value that will be used in ob_size for inlined ints (they have the LSB set).

Maybe the function has to have a better name to avoid this confusion.

gvanrossum · 2022-02-27T00:56:47Z

Objects/longobject.c

+{
+    Py_ssize_t value;
+
+    if (x && as_inlined_int(x, &value)) {


Why check for x != NULL?

github-actions · 2022-03-31T00:09:42Z

This PR is stale because it has been open for 30 days with no activity. If the CLA is not signed within 14 days, it will be closed. See also https://devguide.python.org/pullrequest/#licensing

cpython-cla-bot · 2022-04-10T15:57:38Z

Commit authors are required to sign the Contributor License Agreement.

lpereira added 8 commits February 25, 2022 19:00

pycore_bitutils: Add functions to check for signed int addition overflow

e7513f1

This uses the GCC/Clang builtin to check if either a 32-bit or a 64-bit addition would overflow, and a fast software-only check everywhere else.

pycore_bitutils: Add functions to check for signed subtraction overflow

2ff6545

This uses the GCC/Clang builtin to check if either a 32-bit or a 64-bit subtraction would underflow, and a fast software-only check everywhere else.

pycore_bitutils: Add functions to check for signed multiplication ove…

91916a4

…rflow This uses the GCC/Clang builtin to check if either a 32-bit or a 64-bit multiplication would overflow, and a fast software-only check everywhere else.

pycore_bitutils: Add 64-bit implementation of _Py_popcount()

2dfcba2

This tries to use GCC/Clang builtins when available, and falls back to using the 32-bit popcount twice.

Mark _PyLong_DigitValue[] as constant

959563f

longobject: Calculate bits per char more efficiently

e3d7fdc

bedevere-bot added the awaiting review label Feb 26, 2022

This comment was marked as outdated.

Sign in to view

the-knights-who-say-ni added the CLA not signed label Feb 26, 2022

sweeneyde reviewed Feb 26, 2022

View reviewed changes

Objects/longobject.c Show resolved Hide resolved

gvanrossum reviewed Feb 26, 2022

View reviewed changes

gvanrossum reviewed Feb 28, 2022

View reviewed changes

github-actions bot added the stale Stale PR or inactive for long period of time. label Mar 31, 2022

ambv closed this Apr 10, 2022

ambv reopened this Apr 10, 2022

ambv removed the CLA not signed label Apr 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

draft: Store integers in ob_size field of PyLongObjects #31595

draft: Store integers in ob_size field of PyLongObjects #31595

lpereira commented Feb 26, 2022

This comment was marked as outdated.

gvanrossum left a comment

gvanrossum Feb 26, 2022

gvanrossum Feb 26, 2022

gvanrossum Feb 26, 2022

gvanrossum Feb 26, 2022

gvanrossum Feb 26, 2022

gvanrossum Feb 27, 2022

gvanrossum Feb 26, 2022

gvanrossum Feb 26, 2022

gvanrossum Feb 26, 2022

gvanrossum Feb 26, 2022

lpereira Feb 28, 2022

gvanrossum Feb 28, 2022

gvanrossum Feb 26, 2022

gvanrossum left a comment

gvanrossum Feb 26, 2022

lpereira Feb 28, 2022

gvanrossum Feb 26, 2022

gvanrossum Feb 26, 2022

gvanrossum Feb 26, 2022

gvanrossum Feb 26, 2022

lpereira Feb 28, 2022

gvanrossum left a comment

gvanrossum Feb 27, 2022

gvanrossum Feb 27, 2022

lpereira Feb 28, 2022

gvanrossum Feb 27, 2022

lpereira Feb 28, 2022

gvanrossum Feb 27, 2022

github-actions bot commented Mar 31, 2022

cpython-cla-bot bot commented Apr 10, 2022


		For inlined longs, their value can be obtained with this expression:

		ob_size >> 1

		* These numbers can also be normalized. In a normalized number,
		ob_digit[abs(ob_size)-1] (the most significant digit) is never zero.

		aware that ints abuse ob_size's sign bit and its least significant
		bit.

draft: Store integers in ob_size field of PyLongObjects #31595

Are you sure you want to change the base?

draft: Store integers in ob_size field of PyLongObjects #31595

Conversation

lpereira commented Feb 26, 2022

This comment was marked as outdated.

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

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

github-actions bot commented Mar 31, 2022

cpython-cla-bot bot commented Apr 10, 2022