FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-139716: Make `PyStackRef_FromPyObjectSteal` function very lightweight for GIL build as on FT build by efimov-mikhail · Pull Request #143024 · python/cpython · GitHub

/ cpython Public

gh-139716: Make PyStackRef_FromPyObjectSteal function very lightweight for GIL build as on FT build - #143024

Open
efimov-mikhail wants to merge 2 commits into
python:mainfrom
efimov-mikhail:issue-139716-stackref-immortal-gil-2
Open

gh-139716: Make PyStackRef_FromPyObjectSteal function very lightweight for GIL build as on FT build#143024
efimov-mikhail wants to merge 2 commits into
python:mainfrom
efimov-mikhail:issue-139716-stackref-immortal-gil-2

Conversation

efimov-mikhail commented Dec 20, 2025
edited
Loading

Copy link
Copy Markdown
Member

This is another attempt to provide the same StackRef flagging scheme for all builds (GIL, FT, STACKREF_DEBUG).
Instead of adding _Py_IsImmortal check to PyStackRef_FromPyObjectSteal for FT builds as in #141675, we make no checks at all in this function.
As a result, immortal objects will be DECREF'ed with no effect at reference closing.

Copy link
Copy Markdown
Member Author

Copy link
Copy Markdown
Member Author

If this PR will be merged we can remove PyStackRef_FromPyObjectStealMortal at all.
OTOH, I'd like to provide this change in a separate PR.

Copy link
Copy Markdown
Member

This is another attempt to provide the same StackRef flagging scheme for all builds (GIL, FT, STACKREF_DEBUG). Instead of adding _Py_IsImmortal check to PyStackRef_FromPyObjectSteal for FR builds as in #141675, we make no checks at all in this function. As a result, immortal objects will be DECREF'ed with no effect at reference closing.

We can't rely on Py_DECREF to do the immortal check, as that is more expensive than simply checking a bit on a pointer. You have to do an atomic read of memory now.

efimov-mikhail commented Dec 20, 2025
edited
Loading

Copy link
Copy Markdown
Member Author

We can't rely on Py_DECREF to do the immortal check, as that is more expensive than simply checking a bit on a pointer. You have to do an atomic read of memory now.

It seems that I'm not quite understand, when we will do additional atomic reads.
Do you mean FT builds? For those nothing changes since Py_DECREF_MORTAL is Py_DECREF on them.
And there's no atomic reads at Py_DECREF on GIL builds.

Copy link
Copy Markdown
Member

Sorry I think I'm misunderstanding. This PR moves the immortal check from the stackref to the Py_DECREF right?

Copy link
Copy Markdown
Member Author

Sorry I think I'm misunderstanding. This PR moves the immortal check from the stackref to the Py_DECREF right?

Yes. If we steal stack reference from the immortal object then there will be no actual decrefing at Py_DECREF.

Copy link
Copy Markdown
Contributor

We can't rely on Py_DECREF to do the immortal check, as that is more expensive than simply checking a bit on a pointer. You have to do an atomic read of memory now.

Actually for immortal check, only ob_ref_local is read with relaxed order so it will get compiled to plain load. That will still be more instructions executed that checking a single bit though.

Copy link
Copy Markdown
Contributor
  1. Please benchmark this
  2. I think @markshannon preferred the current scheme

Copy link
Copy Markdown
Member

I like the idea of merging the two implementations, but why choose the slower scheme, not the faster one?
AFAICT, the FT variant has an extra memory access for the immortality check when doing a CLOSE or DUP.

github-actions Bot commented May 4, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

github-actions Bot added the stale Stale PR or inactive for long period of time. label May 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting review skip news stale Stale PR or inactive for long period of time.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL