Skip to content

gh-125323: Remove some unsafe stackref decrefs #125324

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 14, 2024

Conversation

Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented Oct 11, 2024

@Fidget-Spinner
Copy link
Member Author

@markshannon this needs benchmarking

@markshannon
Copy link
Member

markshannon commented Oct 12, 2024

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.

@Fidget-Spinner
Copy link
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.

@Fidget-Spinner
Copy link
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 Fidget-Spinner changed the title gh-125323: Fix unsafe stackref decrefs gh-125323: Remove some unsafe stackref decrefs Oct 12, 2024
@markshannon
Copy link
Member

Failures are the usual suspects.

@markshannon
Copy link
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.

@markshannon markshannon merged commit 4b358ee into python:main Oct 14, 2024
60 of 65 checks passed
@Fidget-Spinner Fidget-Spinner deleted the fix_unsafe_refcounting branch October 14, 2024 08:53
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