GeezerVison

!buildbot nogil

🤖 New build scheduled with the buildbot fleet by @Fidget-Spinner for commit 8cb139f 🤖

The command will test the builders whose names match following regular expression: nogil

The builders matched are:

• AMD64 Ubuntu NoGIL PR
• aarch64 Fedora Rawhide NoGIL PR
• x86-64 MacOS Intel ASAN NoGIL PR
• AMD64 Ubuntu NoGIL Refleaks PR
• AMD64 Fedora Rawhide NoGIL refleaks PR
• aarch64 Fedora Rawhide NoGIL refleaks PR
• PPC64LE Fedora Rawhide NoGIL refleaks PR
• AMD64 Windows Server 2022 NoGIL PR
• ARM64 MacOS M1 Refleaks NoGIL PR
• PPC64LE Fedora Rawhide NoGIL PR
• x86-64 MacOS Intel NoGIL PR
• ARM64 MacOS M1 NoGIL PR
• AMD64 Fedora Rawhide NoGIL PR

!buildbot nogil

🤖 New build scheduled with the buildbot fleet by @Fidget-Spinner for commit 700c2fd 🤖

The command will test the builders whose names match following regular expression: nogil

The builders matched are:

• AMD64 Ubuntu NoGIL PR
• aarch64 Fedora Rawhide NoGIL PR
• x86-64 MacOS Intel ASAN NoGIL PR
• AMD64 Ubuntu NoGIL Refleaks PR
• AMD64 Fedora Rawhide NoGIL refleaks PR
• aarch64 Fedora Rawhide NoGIL refleaks PR
• PPC64LE Fedora Rawhide NoGIL refleaks PR
• AMD64 Windows Server 2022 NoGIL PR
• ARM64 MacOS M1 Refleaks NoGIL PR
• PPC64LE Fedora Rawhide NoGIL PR
• x86-64 MacOS Intel NoGIL PR
• ARM64 MacOS M1 NoGIL PR
• AMD64 Fedora Rawhide NoGIL PR

!buildbot nogil

🤖 New build scheduled with the buildbot fleet by @Fidget-Spinner for commit cde931d 🤖

The command will test the builders whose names match following regular expression: nogil

The builders matched are:

• AMD64 Ubuntu NoGIL PR
• aarch64 Fedora Rawhide NoGIL PR
• x86-64 MacOS Intel ASAN NoGIL PR
• AMD64 Ubuntu NoGIL Refleaks PR
• AMD64 Fedora Rawhide NoGIL refleaks PR
• aarch64 Fedora Rawhide NoGIL refleaks PR
• PPC64LE Fedora Rawhide NoGIL refleaks PR
• AMD64 Windows Server 2022 NoGIL PR
• ARM64 MacOS M1 Refleaks NoGIL PR
• PPC64LE Fedora Rawhide NoGIL PR
• x86-64 MacOS Intel NoGIL PR
• ARM64 MacOS M1 NoGIL PR
• AMD64 Fedora Rawhide NoGIL PR

One possibility to keep things moving for the free-threading build is to generate a different generated_cases.h for the free-threading interpreter.
For that case, you could write all values to the stack before any decrefs or escaping calls.

It might be quite a lot of work, but I really think we should mark any escaping calls, so that the code generator knows what escapes. DECREF_INPUTS is already handled by the code generator, so no changes to the code should be necessary in bytecodes.c regarding reference count operations.

Ok I can port your old PR over to this one. I presume we're flushing the values to the stack before every escaping call?

@markshannon, writing all values to the stack is going to introduce an unnecessary performance regression.

This is holding up the free-threaded work for what seems like small aesthetic complaints. If the concern is about maintainer confusion, we can add comments or otherwise document the dozen or so places where this is used.

This is not just aesthetics.
Maintainability is important. By degrading maintainability, you are making more work for others. Particularly for my team.

You say that "writing all values to the stack will by slow". Only for those parts of the stack that are in memory.
Which is why it is important to leave the choice of which parts of the stack to spill to memory and which parts to spill to the stack up to the code generator, as it will differ for different tiers and different platforms.

the dozen or so places where this is used.

There are a hundred or more places where execution can escape from the interpreter, not counting DECREFs which add hundreds more. It seems unlikely that this PR correctly identifies all cases where a garbage collection can occur and correctly spills all the necessary values to the stack memory.

Even if it is correct, it stills lays traps for the unwary.
For example, _BINARY_OP contains the code:

    DECREF_INPUTS();
    ERROR_IF(res_o == NULL, error);
    res = PyStackRef_FromPyObjectSteal(res_o);

If this were changed to

    res = PyStackRef_FromPyObjectSteal(res_o);
    DECREF_INPUTS();
    ERROR_IF(PyStackRef_IsNull(res), error);

then it would be unsafe, yet there would be no warning or error from any tools.

The rewritten _BINARY_OP example is safe because it uses PyStackRef_FromPyObjectSteal. The concern is with calls to
PyStackRef_FromPyObjectNew.

There are a hundred or more places where execution can escape from the interpreter

We should not be thinking about this in terms of where calls escape from the interpreter. We should be thinking about this in terms of where PyStackRef_FromPyObjectNew() calls occur, and ensure that those are always written to the stack.

If you want to refactor PyStackRef_FromPyObjectNew so that it takes a pointer or similar that's fine.

Ok I can port your old PR over to this one. I presume we're flushing the values to the stack before every escaping call?

Sorry I forgot that this may introduce another perf regression on the free-threaded build, even if I limit it just to that. So I'm putting a pause on this. I think we should discuss this on Wednesday.

In general, stackrefs must be spilled to the in-memory stack around any escaping call.
To do this automatically, we need to:

1. Identify all escaping calls
2. Track assignments to output values
3. Raise an error if an escaping call occurs unless all or none of the output values have been assigned
4. Generate the spill around the call.
   • If none of the output values have been defined, we just spill the stack pointer after popping the inputs.
   • If all of the output values have been defined, we save the outputs to memory and save the stack pointer before making the escaping call.

Identifying all escaping calls.

We have a whitelist, all other calls are escaping. The list needs updating, but it's mostly right.

Track assignments to output values

Assignments are easy to spot name = .... Then we need do is see if name is an output variable. Any assignments in nested code, or multiple assignments, should be treated as an error.

It is easy enough to change the code move assignments out of branches, and don't want the code gen to have to do flow analysis.

Generating spills around calls.

Once we having identified the escaping call, we will need to walk backwards and forwards around the call to identify the whole statement.
Once we've done that we need to emit the spill before and the reload after (if any reload is needed)

In case that sounds too expensive, don't worry. It shouldn't be.

Spilling the stack pointer is cheap as registers often need to be spilled across calls anyway.
We need to save the output values to memory in most cases anyway, we are just doing it a little earlier.

Although escaping calls are common in bytecodes.c, there are not so common dynamically. Dynamically, only about 10% of instructions include an escaping call and for many of those the additional cost is negligible for the reasons given above.

@markshannon, that is not what we discussed and agreed to yesterday Wednesday. What we discussed was tracking PyStackRef_FromPyObjectNew calls from the code generator and spilling those immediately.

• We do not need or want to track stackrefs in general for the GC, just PyStackRef_FromPyObjectNew
• There's no point in looking for spilling calls and waiting to write the result of PyStackRef_FromPyObjectNew in multiple places.

Tracking just PyStackRef_FromPyObjectNew will solve your immediate problem, but it doesn't help with a broader deferred reference counting implementation nor with top-of-stack caching.

I think what I proposed handles all the use cases, without a significant performance impact.
Even if it turns out the performance impact is too high, the additional analysis with help make any solution more robust.

There's no point in looking for spilling calls and waiting to write the result of PyStackRef_FromPyObjectNew in multiple places.

Why would anything get written multiple times?

I think the original goal was to not block TOS caching and full deferred refcounting. With the latest changes, this is now true. The added goal of laying the foundations for the two should be left as another PR. The responsibility of this PR IMO, is to not burden stack caching and full deferred refcounting (which it should have achieved with the latest commit). Supporting more features is out of scope.

@markshannon - I'm most concerned because I thought we were on the same page on this approach after our last meeting.

I don't think starting with this approach precludes future changes, like what you've outlined above. This PR is a bottleneck for a lot of the free-threading deferred reference counting work, so I'd appreciate it if we can figure out how to unblock it.

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@markshannon, I've split off the changes to the interpreter generator into a separate PR:

• gh-118926: Spill deferred references to stack in cases generator #122748

I think that will make it easier to see the changes to the generated code. (And the merge conflicts were getting unwieldy.)