gh-127119: Faster check for small ints in long_dealloc #127620
Conversation
… 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) |
| @@ -0,0 +1 @@ | |||
| Slightly optimize the :class:`int` deallocator. | |||
There was a problem hiding this comment.
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 |
@eduardo-elizondo already spotted that back then #102464 (comment) and created #103403, especially to target |
|
@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. The results of the pyperformance fleet would be interesting, too, since you had to touch I like that in your code the comment about Accidental De-Immortalizing is now exactly before 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 Every none-small-int will be deallocated, and for all of them we pay the price of I don't care much about |
Objects/longobject.c
Outdated
| } | ||
|
|
||
| void | ||
| _PyLong_ExactDealloc(PyObject *self) | ||
| { | ||
| assert(PyLong_CheckExact(self)); | ||
| #ifndef Py_GIL_DISABLED |
There was a problem hiding this comment.
I don't think we should excluding the no-gil build.
There was a problem hiding this comment.
You are right. According to https://peps.python.org/pep-0683/#stable-abi the no-gil implementation can work with older stable API extensions.
Objects/longobject.c
Outdated
| _Py_FREELIST_FREE(ints, self, PyObject_Free); | ||
| return; | ||
| } | ||
| #ifndef Py_GIL_DISABLED |
|
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 |
Co-authored-by: Mark Shannon <mark@hotpy.org>
Co-authored-by: Mark Shannon <mark@hotpy.org>
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 |
|
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. |
long_deallocMicrobenchmark from #127120:
(benchmark is with PGO, without PGO I see no performance improvement)