gh-115999: Add free-threaded specialization for UNPACK_SEQUENCE

[Eclips4]

• Issue: Make the specializing interpreter thread-safe in --disable-gil builds #115999

[brandtbucher]

Sort of unrelated to this PR, but I'm surprised constant unpacking like this isn't peepholed into LOAD_CONST, STORE_FAST, LOAD_CONST, STORE_FAST. Maybe because it's actually a longer instruction sequence than the original? It certainly does less work.

@iritkatriel, thoughts?

[mpage]

Thanks for doing this! The changes look good. I think the current implementations of UNPACK_SEQUENCE_{TUPLE,TWO_TUPLE} are already thread-safe since tuples are immutable. The implementation of UNPACK_SEQUENCE_LIST isn't thread-safe (there is nothing preventing another thread from adding or removing items from the list while the instruction is executing) so we're going to need a slightly different implementation in the free-threaded build.

I would suggest taking a critical section around the list size check and the code that pushes items onto the stack. Something like:

inst(UNPACK_SEQUENCE_LIST, (unused/1, seq -- values[oparg])) {
    PyObject *seq_o = PyStackRef_AsPyObjectBorrow(seq);
    DEOPT_IF(!PyList_CheckExact(seq_o));
    int should_deopt = 0;
    Py_BEGIN_CRITICAL_SECTION(seq_o);
    should_deopt = PyList_GET_SIZE(seq_o) != oparg;
    if (!should_deopt) {
        STAT_INC(UNPACK_SEQUENCE, hit);
        PyObject **items = _PyList_ITEMS(seq_o);
        for (int i = oparg; --i >= 0; ) {
            *values++ = PyStackRef_FromPyObjectNew(items[i]);
        }
    }
    Py_END_CRITICAL_SECTION();
    DEOPT_IF(should_deopt);
    DECREF_INPUTS();
}

[bedevere-app]

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

[mpage]

LGTM!

[markshannon]

Could this be rewritten as something like:

    if (PyList_GET_SIZE(seq_o) != oparg) {
         END_CRITICAL_SECTION
         DEOPT_IF(true);
    }

[mpage]

We can't use the macros because they introduce and close a new scope.

I don't think there's precedence in CPython for using the critical section functions manually (everything uses the macros), but if it's important that we maintain the structure you're suggesting we could do something like

#ifdef Py_GIL_DISABLED
PyCriticalSection _py_cs;
PyCriticalSection_Begin(&_py_cs, seq_o);
#endif
if (PyList_GET_SIZE(seq_o) != oparg) {
    #ifdef Py_GIL_DISABLED
    PyCriticalSection_End(&_py_cs);
    #endif
    DEOPT_IF(true);
}
STAT_INC(UNPACK_SEQUENCE, hit);
PyObject **items = _PyList_ITEMS(seq_o);
for (int i = oparg; --i >= 0; ) {
    *values++ = PyStackRef_FromPyObjectNew(items[i]);
}
#ifdef Py_GIL_DISABLED
PyCriticalSection_End(&_py_cs);
#endif
DECREF_INPUTS();

Note that the preprocessor guards are not necessary for correctness; the critical section functions are a no-op in default builds.

[markshannon]

This doesn't fit into the general pattern of specialized instructions:

• One or more guards
• Zero or one actions

See the docs for why this matters.

Has this been benchmarked?

[mpage]

Has this been benchmarked?

Performance is neutral overall on the default build. Though strangely the results for the unpack sequence benchmark look like this change is significantly faster. I'm not sure how reliable that benchmark is, however.

Performance is improved by 2% overall on the free-threaded build.

[mpage]

@markshannon - Please have another look at this.

[mpage]

LGTM!

[mpage]

@Eclips4 - I think it would be fine to merge this PR. We can put up follow up PRs if any subsequent changes are required.