gh-127119: Faster check for small ints in long_dealloc#127620
Conversation
markshannon
left a comment
There was a problem hiding this comment.
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.
Sorry, something went wrong.
… 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) |
Sorry, something went wrong.
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 |
Sorry, something went wrong.
@eduardo-elizondo already spotted that back then #102464 (comment) and created #103403, especially to target |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
markshannon
left a comment
There was a problem hiding this comment.
A few very minor issues
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
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 |
Sorry, something went wrong.
|
I have made the requested changes; please review again |
Sorry, something went wrong.
|
Thanks for making the requested changes! @markshannon: please review the changes made to this pull request. |
Sorry, something went wrong.
markshannon
left a comment
There was a problem hiding this comment.
Looks good
Sorry, something went wrong.
long_deallocMicrobenchmark from #127120:
(benchmark is with PGO, without PGO I see no performance improvement)