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

gh-125323: Remove some unsafe stackref decrefs by Fidget-Spinner · Pull Request #125324 · python/cpython · GitHub

Repository navigation

gh-125323: Remove some unsafe stackref decrefs - #125324

Merged
markshannon merged 3 commits into
python:mainfrom
Fidget-Spinner:fix_unsafe_refcounting
Oct 14, 2024
Merged

markshannon merged 3 commits into
python:mainfrom
Fidget-Spinner:fix_unsafe_refcounting

Conversation

Fidget-Spinner commented Oct 11, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Copy link
Copy Markdown
Member Author

@markshannon this needs benchmarking

markshannon commented Oct 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Does this really fix the unsafe decrefs, or just some of them?

It isn't just the _Py_DECREF_SPECIALIZED, it is also the DECREF_INPUTS_AND_REUSE_FLOAT and _Py_DECREF_NO_DEALLOC. (I see you've also removed the _Py_DECREF_NO_DEALLOC)

I can also see one unsafe Py_DECREFs in RERAISE, there may be more.

this needs benchmarking

There's not much point if we already know it's going to be slower.
We should replace _Py_DECREF_SPECIALIZED with PyStackRef_CLOSE_SPECIALIZED to avoid the slowdown.

Copy link
Copy Markdown
Member Author

I can also see one unsafe Py_DECREFs in RERAISE, there may be more.

I manually went through every Py_DECREF in bytecodes.c. That Py_DECREF is safe. It's a PyStackRef_AsPyObjectSteal then Py_DECREF, not a borrow.

Copy link
Copy Markdown
Member Author

it is also the DECREF_INPUTS_AND_REUSE_FLOAT ~and

I don't know how to fix that without tanking performance. The way they behave doesn't conform to the stackref semantics at all.

Fidget-Spinner changed the title gh-125323: Fix unsafe stackref decrefs gh-125323: Remove some unsafe stackref decrefs Oct 12, 2024

Copy link
Copy Markdown
Member

Failures are the usual suspects.

Copy link
Copy Markdown
Member

Performance is a wash, as I would have expected.

So, lets' leave the DECREF_INPUTS_AND_REUSE_FLOAT macros for another PR and merge this.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants


Back | FazBrowse Home | New Git URL