Skip to content

Conversation

@Fidget-Spinner
Copy link
Member

@Fidget-SpinnerFidget-Spinner commented Oct 11, 2024

@Fidget-Spinner
Copy link
MemberAuthor

@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_FLOATand _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
MemberAuthor

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
MemberAuthor

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-SpinnerFidget-Spinner changed the title gh-125323: Fix unsafe stackref decrefsgh-125323: Remove some unsafe stackref decrefsOct 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.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@Fidget-Spinner@markshannon