gh-127119: Faster check for small ints in long_dealloc by eendebakpt · Pull Request #127620 · python/cpython · GitHub

Use a faster check for small ints in long_dealloc
Remove check for small ints in the free-threaded build
Add reference to PEP 683

Microbenchmark from #127120:

bench_long: Mean +- std dev: [main_pgo_bm_long] 195 ns +- 5 ns -> [pr_v2_pgo_bm_long] 191 ns +- 4 ns: 1.02x faster
bench_alloc: Mean +- std dev: [main_pgo_bm_long] 425 us +- 37 us -> [pr_v2_pgo_bm_long] 394 us +- 20 us: 1.08x faster

Benchmark hidden because not significant (1): bench_collatz

Geometric mean: 1.04x faster

(benchmark is with PGO, without PGO I see no performance improvement)

Issue: Remove redundant small int check in long_dealloc #127119

This is technically not legal C.
Pointer comparisons between separately allocated blocks of memory are undefined.

Which is a shame, because this is clearly faster.

… into small_int_immortal_v2

Pointer comparisions are indeed not always well-defined. For reference: see https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf, section 6.5.8.6, or https://www.gnu.org/software/c-intro-and-ref/manual/html_node/Pointer-Comparison.html.

I updated the PR to use the immortality bit instead.

(maybe there are some more places where we need to update documentation on the immortality bit)

… into small_int_immortal_v2

Maybe include the benchmarks results (it's a 4% improvement which is still noticable IMO). You should mention that it only concerns PGO builds as well.

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

… into small_int_immortal_v2

Turns out the immortal bit was assumed to be zero in _PyLong_IsNonNegativeCompact. The updated _PyLong_IsNonNegativeCompact makes some of the operations with ints a (tiny) bit slower, so we have to measure performance a bit more carefull.

Turns out the immortal bit was assumed to be zero in _PyLong_IsNonNegativeCompact. The updated _PyLong_IsNonNegativeCompact makes some of the operations with ints a (tiny) bit slower, so we have to measure performance a bit more carefull.

@eduardo-elizondo already spotted that back then #102464 (comment) and created #103403, especially to target long_dealloc, see #103403 (comment).

@chris-eibl Thanks for that little bit of history.

@markshannon @eduardo-elizondo This PR is more or less identical to #103403 which has been closed. Unless you feel different about it now, I suggest we close this.

But your microbenchmarks for the initial version seemed promising.
Do you have numbers for the current version of the PR as well?

The results of the pyperformance fleet would be interesting, too, since you had to touch _PyLong_IsNonNegativeCompact.

I like that in your code the comment about Accidental De-Immortalizing is now exactly before _Py_SetImmortal.

In the previous version, I had to read carefully, especially because it starts with "This should never get called", which of course does not comment on the invocation of long_dealloc itself.

Every none-small-int will be deallocated, and for all of them we pay the price of _PyLong_IsCompact. If it is a compact int (very likely), then a bunch more code is executed. Especially for small int values we have to check whether they match the singletons to care about accidential de-immortaliziation. And we have to go down to the if (pylong == small_pylong) for all small int values for everything that derives from int.

I don't care much about IntEnums or their "predecessors" class _NamedIntConstant(int) , etc, since those most likely aren't deallocated before the end of the programm. But e.g. extensions that derive from int.

A few very minor issues

I don't think we should excluding the no-gil build.

You are right. According to https://peps.python.org/pep-0683/#stable-abi the no-gil implementation can work with older stable API extensions.

Likewise

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Co-authored-by: Mark Shannon <mark@hotpy.org>

@markshannon Adding the small int check for the free-threaded build uncovered a bug: we have to prevent copying the immortal bit when subclassing int. I addresed the bug in long_subtype_new and added an assert to _long_is_small_int to detect similar issues.

I have made the requested changes; please review again

Thanks for making the requested changes!

@markshannon: please review the changes made to this pull request.

Looks good

Faster check for small ints in long_dealloc

cfb70cb

bedevere-app Bot mentioned this pull request Dec 4, 2024

Remove redundant small int check in long_dealloc #127119

Closed

bedevere-app Bot added the awaiting review label Dec 4, 2024

📜🤖 Added by blurb_it.

426dd09

markshannon reviewed Dec 5, 2024

View reviewed changes

eendebakpt added 2 commits December 5, 2024 20:39

Use int immortality bit for small int check

4639642

Merge branch 'small_int_immortal_v2' of github.com:eendebakpt/cpython…

d9c26d3

… into small_int_immortal_v2

eendebakpt added 4 commits December 5, 2024 20:40

Merge branch 'main' into small_int_immortal_v2

2f73d47

fix compiler warnings

3b6e1fe

Merge branch 'small_int_immortal_v2' of github.com:eendebakpt/cpython…

03184a7

… into small_int_immortal_v2

compiler warnings

5eca812

picnixz reviewed Dec 5, 2024

View reviewed changes

eendebakpt and others added 3 commits December 5, 2024 21:04

some documentation updates

fedc102

Update Objects/longobject.c

c733d25

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

Merge branch 'small_int_immortal_v2' of github.com:eendebakpt/cpython…

829a595

… into small_int_immortal_v2

eendebakpt commented Dec 5, 2024

View reviewed changes

Comment thread Tools/gdb/libpython.py Outdated

eendebakpt added 3 commits December 5, 2024 21:06

Update Tools/gdb/libpython.py

39da0ea

tests!

5058e53

tests!

8a3d00f

eendebakpt marked this pull request as draft December 5, 2024 22:08

bedevere-app Bot removed the awaiting review label Dec 5, 2024

update _PyLong_IsNonNegativeCompact

878207a

eendebakpt marked this pull request as ready for review December 5, 2024 22:50

bedevere-app Bot added the awaiting review label Dec 5, 2024

chris-eibl reviewed Dec 6, 2024

View reviewed changes

Comment thread Objects/longobject.c

Merge branch 'main' into small_int_immortal_v2

d91b6e3

markshannon requested changes Dec 20, 2024

View reviewed changes

bedevere-app Bot added awaiting changes and removed awaiting review labels Dec 20, 2024

eendebakpt and others added 12 commits December 22, 2024 16:40

Update Modules/_testcapi/immortal.c

ebc7e17

Co-authored-by: Mark Shannon <mark@hotpy.org>

Update Modules/_testcapi/immortal.c

aaf110b

Co-authored-by: Mark Shannon <mark@hotpy.org>

Update Include/cpython/longintrepr.h

8fcc1d0

Co-authored-by: Mark Shannon <mark@hotpy.org>

Merge branch 'main' into small_int_immortal_v2

aff8812

review comments

ebb0bca

include pycore_long.h

8d8e794

header

4a07ba1

include pycore_long.h

1d5b2e0

debug build failure

cfb9120

Merge branch 'main' into small_int_immortal_v2

b606c09

fix bug when copying the immortal bit

b4c8444

add assert to make sure the immortal bit is not set on subclasses

f76c0e2

bedevere-app Bot added awaiting change review and removed awaiting changes labels Jan 8, 2025

bedevere-app Bot requested a review from markshannon January 8, 2025 21:33

eendebakpt mentioned this pull request Jan 28, 2025

gh-127119: Remove check on accidental deallocation of immortal objects for free-threaded build #127120

Closed

markshannon approved these changes Jan 29, 2025

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting change review labels Jan 29, 2025

markshannon merged commit a292216 into python:main Jan 29, 2025

bedevere-app Bot removed the awaiting merge label Jan 29, 2025

eendebakpt mentioned this pull request May 1, 2025

gh-84436: Implement Immortal Objects #19474

Merged

		@@ -0,0 +1 @@
		Slightly optimize the :class:`int` deallocator.

               _PyLong_ExactDealloc(PyObject *self)
               {
                   assert(PyLong_CheckExact(self));
+              #ifndef Py_GIL_DISABLED

gh-127119: Faster check for small ints in long_dealloc by eendebakpt · Pull Request #127620 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-127119: Faster check for small ints in long_dealloc - #127620

gh-127119: Faster check for small ints in long_dealloc#127620markshannon merged 39 commits intopython:mainpython/cpython:mainfrom eendebakpt:small_int_immortal_v2eendebakpt/cpython:small_int_immortal_v2Copy head branch name to clipboard

Conversation

eendebakpt commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

eendebakpt commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

picnixz Dec 5, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eendebakpt commented Dec 5, 2024

Uh oh!

Uh oh!

chris-eibl commented Dec 7, 2024

Uh oh!

eendebakpt commented Dec 7, 2024

Uh oh!

chris-eibl commented Dec 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

markshannon Dec 20, 2024

Choose a reason for hiding this comment

Uh oh!

eendebakpt Dec 22, 2024

Choose a reason for hiding this comment

Uh oh!

markshannon Dec 20, 2024

Choose a reason for hiding this comment

Uh oh!

bedevere-app Bot commented Dec 20, 2024

Uh oh!

eendebakpt commented Dec 23, 2024

Uh oh!

eendebakpt commented Jan 8, 2025

Uh oh!

bedevere-app Bot commented Jan 8, 2025

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gh-127119: Faster check for small ints in long_dealloc#127620
markshannon merged 39 commits into
python:mainfrom
eendebakpt:small_int_immortal_v2

eendebakpt commented Dec 4, 2024 •
edited
Loading

eendebakpt commented Dec 5, 2024 •
edited
Loading

chris-eibl commented Dec 8, 2024 •
edited
Loading