Repository navigation
Reduce critical section boilerplate in type slot implementations #114258
Description
Activity
The disadvantage is that "tp slots" don't need much argument parsing, so they may otherwise not be a great fit for Argument Clinic
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
-
wrt. the original goals of Argument Clinic, that is ↩
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 callingtp_richcompare
Line 902 in a34e4db
B. Not all of them, some of them:
Provide wrapping macros that can be easily used._Py_CRITICAL_SECTION_TP_RICHCOMPARE_IMPL(list_richcompare_impl) // ... .tp_richcompare = _Py_CRITICAL_SECTION_TP_RICHCOMPARE(list_richcompare_impl)
C. Very few of them:
Use our hands.. :)I would like to suggest delaying the decision until we notice how many cases will be affected to be thread-safe.
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.
Reacted by Donghee Na and Sam GrossI'd expect (correct me if I'm wrong) most GC related slots to be critical sections: clear, traverse, and free.
@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?Reacted by Sam GrossI 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.
Reacted by Erlend E. AaslandI'll try to cook up a list of needed AC changes.
Reacted by Sam GrossFor 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_finalize
[...] my rough idea is using the reserved dunder method convention.
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())... ↩
Reacted by Sam Gross and Donghee Na-
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:
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.cc @erlend-aasland, @corona10
Linked PRs