Repository navigation
Reduce critical section boilerplate in type slot implementations #114258
Description
Feature or enhancement
PEP 703 in large part relies on replacing the GIL with fine grained per-object locks. The primary way to acquire and release these locks is through the critical section API (#111569). In #111903, we added support for generating the necessary critical section calls in Argument Clinic wrapped methods. In #112205, this was extended to getters and setters as well.
However, some object behavior is implemented using "tp slots", which are not supported by Argument Clinic. These functions often need critical sections for thread-safety; that means more boilerplate code.
Here are some possible ideas on how to address this:
Add support for "tp slots" to Argument Clinic
We could add support for certain tp slots functions to Argument Clinic. This would provide a consistent way to add critical sections across both slots, getters/setters, and "regular" methods. The disadvantage is that "tp slots" don't need much argument parsing, so they may otherwise not be a great fit for Argument Clinic
Use C macros to wrap slot implementations with critical sections
We could implement an internal-only header that provides macros to wrap a given "tp slot" function with a critical section. For example, something like:
static PyObject *
list_richcompare_impl(PyObject *v, PyObject *w, int op)
{
// implement richcompare unchanged
}
// generates a list_richcompare() function with the correct Py_BEGIN/END_CRITICAL_SECTION()
_Py_CRITICAL_SECTION_TP_RICHCOMPARE(list_richcompare, list_richcompare_impl);I think this would not be too difficult to implement -- probably easier than modifying Argument Clinic. The disadvantage is the lack of uniformity around how we add critical sections to functions.
Keep the status quo
We can always just keep adding the Py_BEGIN/END_CRITICAL_SECTION() calls manually for tp slots.
Activity
erlend-aasland commentedon Jan 19, 2024
OTOH, we may want to expand Argument Clinic's feature set in the future12, and we already added stuff that were not a great fit3: getters and setters.
Footnotes
https://github.com/faster-cpython/ideas/issues/546 ↩
https://github.com/faster-cpython/ideas/issues/565 ↩
wrt. the original goals of Argument Clinic, that is ↩
corona10 commentedon Jan 19, 2024
I would like to suggest delaying the decision until we notice how many cases will be affected to be thread-safe.
The following scenario can be imagined from now on,
A. If every tp_xxx should be thread-safe (AFAIK, We don't want to declare crirical_sections for all tp_compare):
Declare critical_section implicitly before calling
tp_richcomparecpython/Objects/object.c
Line 902 in a34e4db
B. Not all of them, some of them:
Provide wrapping macros that can be easily used.
C. Very few of them:
Use our hands.. :)
erlend-aasland commentedon Jan 19, 2024
I'd expect (correct me if I'm wrong) most GC related slots to be critical sections: clear, traverse, and free. With more than a hundred heap types in Modules/, that means more than three hundred cases already. I also expect (correct me if I'm wrong) can throw in rich-compare, iter, next, repr. ISTM the best way forward is through a carefully designed feature set in Argument Clinic. That will make sure the code base is more maintainable. The consistency will make it easier to reason about the code. It will also be easier to make changes to the boiler-plate code.
+1 for Argument Clinic.
corona10 commentedon Jan 19, 2024
@colesbury cc @erlend-aasland
There will be the stop the world phase(Objects can not be touched at this phase) in free threading CPython, should we care about it?
colesbury commentedon Jan 19, 2024
I am mostly thinking about the non-GC related slots, although critical sections on certain
tp_finalizeslots would make sense. Thetp_traverseslot is only called during stop-the-world (no locking necessary). Thetp_clear()is only called on unreachable objects and doesn't support resurrection, so I'm inclined not bother with locking there either, but we might end up revisiting that.I counted about 14 slots on list that probably need locking or some other thread-safety. There's probably about as many on dict and some on set as well. And then there are other classes that provide
tp_richcompareortp_reprand need locking for those.I think it's worth doing, but I'm not sure how much work it is to get Argument Clinic to support these slots. It would be less overall work if it's implemented sooner, but it's also not the most urgent thing right now, so I'm not sure how we will prioritize it.
erlend-aasland commentedon Jan 20, 2024
I'll try to cook up a list of needed AC changes.
corona10 commentedon Feb 1, 2024
For the design,
If we decide to support this feature in AC.
The design should be decided first and my rough idea is using the reserved dunder method convention.
object.__tp_repr__=> tp_reprobject.__tp_richcompare__==> tp_richcompareobject.__tp_clear__=> tp_clearobject.__tp_finalize__=> tp_finalizeerlend-aasland commentedon Feb 1, 2024
Currently, the denylist is made up of the Python equivalents; see:
cpython/Tools/clinic/clinic.py
Lines 2573 to 2642 in 1390796
Since we will be allowing only a subset of this list, I think it would be beneficial to follow that established convention.
As for the feature, I've stated hashing out some refactorings1.
Footnotes
breaking up
state_modulename_name(), breaking uprender_function(), breaking upoutput_templates())... ↩pythongh-114258: Refactor Argument Clinic function name parser
gh-114258: Refactor Argument Clinic function name parser (#114930)
gh-114258: Argument Clinic: refactor getset implementation (#116170)
pythongh-114258: Argument Clinic: refactor getset implementation (pyt…
pythongh-114258: Argument Clinic: refactor getset implementation (pyt…
pythongh-114258: Argument Clinic: refactor getset implementation (pyt…