GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-101291: Rearrange the size bits in PyLongObject by markshannon · Pull Request #102464 · python/cpython

Conversation

@markshannon

@markshannon markshannon commented Mar 6, 2023 •
edited by gvanrossum
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
markshannon added 20 commits February 28, 2023 11:59
@markshannon
Add functions to hide some internals of long object.
0ec07e4
@markshannon
Add internal functions to longobject.c for setting sign and digit count.
292b9d0
@markshannon
Replace Py_SIZE(x) < 0 with _PyLong_IsNegative(x) in longobject.c
5c54894
@markshannon
Replace Py_ABS(Py_SIZE(a)) with _PyLong_DigitCount(a) in longobject.c
029aaa4
@markshannon
Remove many uses of Py_SIZE in longobject.c
b56e6da
@markshannon
Remove _PyLong_AssignValue, as it is no longer used.
91269fc
@markshannon
Remove some more uses of Py_SIZE in longobject.c.
c48e825
@markshannon
Remove a few more uses of Py_SIZE in longobject.c.
449c0e2
@markshannon
Remove some more uses of Py_SIZE, replacing with _PyLong_UnsignedDigi…
c5ba601 
…tCount().
@markshannon
Replace a few Py_SIZE() with _PyLong_SameSign().
4b3a3e8
@markshannon
Remove a few more Py_SIZE() from longobject.c
9ef9d2c
@markshannon
Replace uses of IS_MEDIUM_VALUE macro with _PyLong_IsSingleDigit.
9c408c1
@markshannon
Remove most of the remaining uses of Py_SIZE in longobject.c
548d656
@markshannon
Replace last remaining uses of Py_SIZE applied to longobject with _Py…
3e3fefd 
…Long_SignedDigitCount which might not be optimal, but is safe.
@markshannon
Don't use _PyObject_InitVar and move a couple of inline functions to …
391fb51 
…header file.
@markshannon
Correct name of inline function.
df8c7d3
@markshannon
Eliminate all remaining uses of Py_SIZE and Py_SET_SIZE on PyLongObject.
bc14fa6
@markshannon
Change layout of size/sign bits in longobject to support future addit…
54c6f1b 
…ion of immortal ints and tagged medium ints.
@markshannon
Test pairs of longs together on fast path of add/mul/sub.
ce6bfb2
@markshannon
Tidy up comment and delete commented out code.
4c1956b
@markshannon
markshannon requested review from isidentical, rhettinger and tiran as code owners March 6, 2023 09:57
@bedevere-bot 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
Closed
@bedevere-bot bedevere-bot added the awaiting core review label Mar 6, 2023
@corona10
corona10 requested a review from mdickinson March 6, 2023 10:01
markshannon added 4 commits March 6, 2023 10:02
@markshannon
Add news.
301158b
@markshannon
Remove debugging asserts.
1aa1891
@markshannon
Fix storage classes.
bf2a9af
@markshannon
Remove development debug functions.
169f521
markshannon added 2 commits March 16, 2023 19:26
@markshannon
Replace _PyLong_Sign(x) < 0 with _PyLong_IsNegative(x).
f764aa8
@markshannon
fix sign check
9843ac0
gvanrossum
gvanrossum reviewed Mar 17, 2023
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Tools/build/umarshal.py Outdated
Comment on lines +97 to +98
if not (0 <= n <= self.end - self.pos):
print(n, self.end, self.pos)

@gvanrossum gvanrossum Mar 16, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Remove debug print()?

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Misc/NEWS.d/next/Core and Builtins/2023-03-06-10-02-22.gh-issue-101291.0FT2QS.rst Outdated
@@ -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.

@gvanrossum gvanrossum Mar 16, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Include/internal/pycore_long.h Outdated
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,

@gvanrossum gvanrossum Mar 17, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

"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).)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Include/internal/pycore_long.h
static inline bool
_PyLong_IsPositive(const PyLongObject *op)
{
return (op->long_value.lv_tag & SIGN_MASK) == 0;

@gvanrossum gvanrossum Mar 17, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why not have #define SIGN_POSITIVE 0?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@markshannon markshannon Mar 20, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gvanrossum gvanrossum Mar 21, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Include/internal/pycore_long.h Outdated
return op->long_value.lv_tag >> NON_SIZE_BITS;
}

/* Equivalent to _PyLong_DigitCount(op) * _PyLong_NonZeroSign(op) */

@gvanrossum gvanrossum Mar 17, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@markshannon markshannon Mar 20, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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))

@gvanrossum gvanrossum Mar 17, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Include/internal/pycore_long.h Outdated
#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)

@gvanrossum gvanrossum Mar 17, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
markshannon reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Include/internal/pycore_long.h Outdated
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;

@gvanrossum gvanrossum Mar 17, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/bltinmodule.c
Comment on lines 2504 to 2506
return PyLong_FromLong(i_result);
}
if (PyLong_CheckExact(item) || PyBool_Check(item)) {

@gvanrossum gvanrossum Mar 17, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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)

@gvanrossum gvanrossum Mar 17, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Maybe we need another convenience macro IsNonZero.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@markshannon markshannon mentioned this pull request Mar 18, 2023
gh-102509: Start initializing ob_digit of _PyLongValue #102510
Merged
lpereira
lpereira reviewed Mar 21, 2023
View reviewed changes
Comment thread Include/internal/pycore_long.h
Comment on lines +205 to +212
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);
}

@lpereira lpereira Mar 21, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gvanrossum gvanrossum Mar 21, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@markshannon markshannon Mar 22, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
gvanrossum
gvanrossum reviewed Mar 21, 2023
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Misc/NEWS.d/next/Core and Builtins/2023-03-06-10-02-22.gh-issue-101291.0FT2QS.rst Outdated
Comment on lines +1 to +4
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.

@gvanrossum gvanrossum Mar 21, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@markshannon markshannon Mar 22, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fixed. I suspect it got reformatted by something.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_tkinter.c
Comment on lines +890 to +891
assert(PyLong_Check(value));
neg = _PyLong_IsNegative((PyLongObject *)value);

@gvanrossum gvanrossum Mar 21, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Include/internal/pycore_long.h Outdated

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

@gvanrossum gvanrossum Mar 21, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@markshannon markshannon Mar 22, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/longobject.c Outdated
Comment on lines +510 to +511
* care (see comment above).
*/

@gvanrossum gvanrossum Mar 21, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Accidental reformat?

Suggested change
* care (see comment above).
*/
* care (see comment above).
*/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@markshannon markshannon Mar 22, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/longobject.c
} while(0)

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

@gvanrossum gvanrossum Mar 21, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Mybe we can prove that b is nonnegative here?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@markshannon markshannon Mar 22, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/longobject.c Outdated
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 */

@gvanrossum gvanrossum Mar 21, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@markshannon markshannon Mar 22, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/longobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/longobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Include/internal/pycore_long.h
static inline bool
_PyLong_IsPositive(const PyLongObject *op)
{
return (op->long_value.lv_tag & SIGN_MASK) == 0;

@gvanrossum gvanrossum Mar 21, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/longobject.c Outdated
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));

@gvanrossum gvanrossum Mar 21, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
markshannon added 2 commits March 22, 2023 11:23
@markshannon
Address some review comments.
d6cb917
@markshannon
Change asserts on digit counts to asserts on sign where applicable.
469d26f
gvanrossum
gvanrossum approved these changes Mar 22, 2023
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Go for it!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Mar 22, 2023
@markshannon
markshannon merged commit 7559f5f into python:main Mar 22, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Mar 22, 2023
@markshannon
markshannon deleted the long-rearrange-size-bits branch March 22, 2023 14:50
@sobolevn

sobolevn commented Mar 23, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this pull request Mar 27, 2023
@markshannon @Fidget-Spinner
pythonGH-101291: Rearrange the size bits in PyLongObject (pythonGH-10…
32f8276 
…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

scoder commented Mar 28, 2023

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@scoder scoder mentioned this pull request Apr 3, 2023
Implement support for the new PyLong struct layout in Py3.12a7 cython/cython#5353
Merged
@gvanrossum 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
@scoder
Implement support for the new PyLong struct layout in Py3.12a7. (GH-5353
781b087 
)

See python/cpython#102464
eduardo-elizondo
eduardo-elizondo reviewed Apr 7, 2023
View reviewed changes
Comment thread Include/internal/pycore_long.h
Comment on lines +112 to +113
* 0-1: Sign bits value = (1-sign), ie. negative=2, positive=0, zero=1.
* 2: Reserved for immortality bit

@eduardo-elizondo eduardo-elizondo Apr 7, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
eduardo-elizondo
eduardo-elizondo reviewed Apr 7, 2023
View reviewed changes
Comment thread 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);

@eduardo-elizondo eduardo-elizondo Apr 7, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
@markshannon @warsaw
pythonGH-101291: Rearrange the size bits in PyLongObject (pythonGH-10…
3a91146 
…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().
@skirpichev skirpichev mentioned this pull request Apr 24, 2023
Breaking changes in Python 3.12.0a7 release gmpy2/gmpy2#405
Closed
verhovsky
verhovsky reviewed Apr 29, 2023
View reviewed changes
Comment thread Include/cpython/longintrepr.h
@@ -80,7 +80,7 @@ typedef long stwodigits; /* signed variant of twodigits */
*/

@verhovsky verhovsky Apr 29, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@carljm 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 eduardo-elizondo mentioned this pull request Jun 19, 2023
gh-84436: Add static flag in PyLongObject's lv_tag #103403
Closed
@chris-eibl chris-eibl mentioned this pull request Dec 7, 2024
gh-127119: Faster check for small ints in long_dealloc #127620
Merged
@eendebakpt 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 skirpichev mentioned this pull request Apr 25, 2025
sum() several times slower on Python 3 64-bit #68264
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes
@tiran tiran Awaiting requested review from tiran
@isidentical isidentical Awaiting requested review from isidentical
+3 more reviewers
@lpereira lpereira lpereira left review comments
@eduardo-elizondo eduardo-elizondo eduardo-elizondo left review comments
@verhovsky verhovsky verhovsky left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@markshannon @scoder @mdickinson @lpereira @sobolevn @gvanrossum @eduardo-elizondo @verhovsky @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.