Repository navigation
The DISPATCH() macro is not as efficient as it could be (move PyThreadState.use_tracing) #87926
Description
Activity
The DISPATCH() macro has two failings.
-
Its check for tracing involves too much pointer chaser.
-
The logic assumes that computed-gotos is the "fast path" which makes switch dispatch, and therefore Python on Windows unnecessarily slow.
-
hroncok commented
on May 10, 2021 hroncokmannequinMannequinI am afraid the "Speed up check for tracing in interpreter dispatch" brought some backwards incompatible changes:
yappi/_yappi.c:1261:9: error: ‘PyThreadState’ {aka ‘struct _ts’} has no member named ‘use_tracing’; did you mean ‘tracing’?
1261 | ts->use_tracing = 1;
| ^~~~~~~~~~~
| tracingThis is not mentioned in https://docs.python.org/3.10/whatsnew/3.10.html and I haven't noticed the use_tracing member being deprecated. I am confused. Should this happened?
hroncok commented
on May 10, 2021 hroncokmannequinMannequinFedora packages affected (that we know of now):
greenlet: https://bugzilla.redhat.com/show_bug.cgi?id=1957784
dipy: https://bugzilla.redhat.com/show_bug.cgi?id=1958203
yappi: https://bugzilla.redhat.com/show_bug.cgi?id=1958896
smartcols: https://bugzilla.redhat.com/show_bug.cgi?id=1958938At yappi/_yappi.c:1261 sets an undocumented field on a CPython internal data structure.
What did you believe that was supposed to do? use_tracing is not documented anywhere.
We could add the field back and ignore it, but I doubt that would help you much.
If there is no C-API function that supports your needs, feel free to suggest one.
hroncok commented
on May 10, 2021 hroncokmannequinMannequinDisclaimer: I have not written the code nor do I understand what is trying to achieve. I merely collect the data and report the problems to the package maintainers.
It just seems to me that a non-underscored (and hence public) member variable on a non-underscored (and hence public) structure should not suddenly go missing. Although, I am not familiar with the rules that define what part of the API falls under https://www.python.org/dev/peps/pep-0497/
Reacted by mueslohroncok commented
on May 10, 2021 hroncokmannequinMannequinscikit-learn: https://bugzilla.redhat.com/show_bug.cgi?id=1958976
gcc: sklearn/cluster/_k_means_fast.c
In file included from /usr/lib64/python3.10/site-packages/numpy/core/include/numpy/ndarraytypes.h:1944,
from /usr/lib64/python3.10/site-packages/numpy/core/include/numpy/ndarrayobject.h:12,
from /usr/lib64/python3.10/site-packages/numpy/core/include/numpy/arrayobject.h:4,
from sklearn/cluster/_k_means_fast.c:635:
/usr/lib64/python3.10/site-packages/numpy/core/include/numpy/npy_1_7_deprecated_api.h:17:2: warning: #warning "Using deprecated NumPy API, disable it with " "#define NPY_NO_DEPRECATED_API NPY_1_7_API_VERSION" [-Wcpp]
17 | #warning "Using deprecated NumPy API, disable it with " \
| ^~~~~~~
sklearn/cluster/_k_means_fast.c: In function ‘__Pyx_call_return_trace_func’:
sklearn/cluster/_k_means_fast.c:1596:15: error: ‘PyThreadState’ {aka ‘struct _ts’} has no member named ‘use_tracing’; did you mean ‘tracing’?
1596 | tstate->use_tracing = 0;
| ^~~~~~~~~~~
| tracing
sklearn/cluster/_k_means_fast.c:1602:15: error: ‘PyThreadState’ {aka ‘struct _ts’} has no member named ‘use_tracing’; did you mean ‘tracing’?
1602 | tstate->use_tracing = 1;
| ^~~~~~~~~~~
| tracingThe usage comes from https://github.com/cython/cython/blob/master/Cython/Utility/Profile.c
PEP-0497 is rejected; the active one is PEP-387, which says "backwards incompatibility" means preexisting code ceases to comparatively function after a change.
So, this does look like a backwards-incompatible change.Unfortunately, not all of the C API is documented, so unless it's explicitly marked private, people will use it :(
But what does "use it" mean?
What does settingtstate->use_tracing = 1do?
There is no documented behavior, so how do we know what assumptions people are making about what happens when they set some field to 1?As I said, we could keep the field and ignore it, but that seems worse.
I don't think the PEP meant to restrict individual struct member such as this. For example, we were able to switch from byte code to word code without violating the intended rules. Consider asking Brett and Benjamin for clarification. I would think that if a new function were introduced to provide a reliable way to determine whether tracing was enabled, that would suffice for external packages to have a minimally disruptive migration path.
I understand that some projects manually call the profile and/or trace functions, and temporarily set use_tracing 0 while calling these functions.
Some projects restore use_tracing to the correct value (compute the efficient value), some projects simply set use_tracing to 1.
I see 3 use cases:
- disable tracing temporarily (set use_tracing to 0)
- reenable tracing (compute use_tracing to the correct value)
- check if tracing is used (get use_tracing)
We can add 3 functions:
- PyThreadState_DisableTracing()
- PyThreadState_EnableTracing()
- PyThreadState_GetTracing()
PyThreadState_EnableTracing(tstate) would do something like:
tstate->cframe->use_tracing = (tstate->c_tracefunc || tstate->c_profilefunc);If we added these functions, I can then add an implementation for Python 3.9 and older to my https://github.com/pythoncapi/pythoncapi_compat project for backward compatibility.
The problem is that some projects also increase temporarily ts->tracing. Since I would like to make PyThreadState opaque, I would prefer to hide this access behind a function call as well. Maybe we need an API to call profile and/or trace functions?
--
According to the bugzilla compiler errors:
greenlet: https://bugzilla.redhat.com/show_bug.cgi?id=1957784
It has already been fixed:
It uses:
- "tstate->use_tracing = 0;"
- "tstate->use_tracing = (tstate->tracing <= 0 && (...)"
It uses:
- "tstate->use_tracing = 0;"
- "tstate->use_tracing = 1;"
- "tstate->use_tracing = (tstate->c_profilefunc || (...)"
- "return tstate->use_tracing && retval;"
- "if (tstate->use_tracing) {"
- "ts->use_tracing = 1;"
- "ts->use_tracing = 0;"
smartcols: https://bugzilla.redhat.com/show_bug.cgi?id=1958938
It uses "tstate->use_tracing = 0;".
scikit-learn: https://bugzilla.redhat.com/show_bug.cgi?id=1958976
It uses:
- "tstate->use_tracing = 0;"
- "tstate->use_tracing = 1;"
The usage comes from https://github.com/cython/cython/blob/master/Cython/Utility/Profile.c
Simplified code:
--------------
static int __Pyx_TraceSetupAndCall(...) { ... tstate->tracing++; tstate->use_tracing = 0;
if (tstate->c_tracefunc) retval = tstate->c_tracefunc(tstate->c_traceobj, *frame, PyTrace_CALL, NULL) == 0; if (retval && tstate->c_profilefunc) retval = tstate->c_profilefunc(tstate->c_profileobj, *frame, PyTrace_CALL, NULL) == 0; tstate-\>use_tracing = (tstate-\>c_profilefunc || (CYTHON_TRACE && tstate-\>c_tracefunc)); tstate-\>tracing--; ...}
int __Pyx_use_tracing = 0; #define __Pyx_TraceCall(funcname, srcfile, firstlineno, nogil, goto_error) \ if (nogil) { \ if (CYTHON_TRACE_NOGIL) { \ PyThreadState *tstate; \ PyGILState_STATE state = PyGILState_Ensure(); \ tstate = __Pyx_PyThreadState_Current; \ if (unlikely(tstate->use_tracing) && !tstate->tracing && \ (tstate->c_profilefunc || (CYTHON_TRACE && tstate->c_tracefunc))) { \ __Pyx_use_tracing = __Pyx_TraceSetupAndCall(&$frame_code_cname, &$frame_cname, tstate, funcname, srcfile, firstlineno); \ } \ PyGILState_Release(state); \ if (unlikely(__Pyx_use_tracing < 0)) goto_error; \ } \ } else { \ PyThreadState* tstate = PyThreadState_GET(); \ if (unlikely(tstate->use_tracing) && !tstate->tracing && \ (tstate->c_profilefunc || (CYTHON_TRACE && tstate->c_tracefunc))) { \ __Pyx_use_tracing = __Pyx_TraceSetupAndCall(&$frame_code_cname, &$frame_cname, tstate, funcname, srcfile, firstlineno); \ if (unlikely(__Pyx_use_tracing < 0)) goto_error; \ } \ }
- changed the title
[-]The DISPATCH() macro is not as efficient as it could be.[/-][+]The DISPATCH() macro is not as efficient as it could be (move PyThreadState.use_tracing)[/+]on May 10, 2021 30 remaining items
Ah, I think the docs need to be clarified a bit. Here's what I was missing:
The key thing to know here is that there are three state variables;
c_tracefunc,c_profilefunc(on the thread state), anduse_tracing(on the C frame).Normally
use_tracingis initialized to false if both functions are NULL, and true otherwise (if at least one of the functions is set).Disabling means setting
use_tracingto false regardless. Resetting means settinguse_tracingto the value computed above.There's also a fourth variable,
tstate->tracing, which indicates whether a tracing function is active (i.e., it has been called and hasn't exited yet). This can be incremented and decremented. But none of the proposed APIs affect it.Would it be reasonable to just put these APIs in pythoncapi_compat, instead of in the stdlib? (It would be yet one more selling point for people to start using that. :-)
bpo-43760: Add PyThreadState_EnterTracing() (GH-28542)
I created changes to use it:
- pythoncapi_compat: python/pythoncapi-compat@10fde24
- Cython: Profile.c uses PyThreadState_EnterTracing() cython/cython#4411
- greenlet: Use PyThreadState_EnterTracing() python-greenlet/greenlet#267
greenlet now uses PyThreadState_EnterTracing() and PyThreadState_LeaveTracing() rather than accessing directly use_tracing:
python-greenlet/greenlet@9b49da5
On Python 3.10, it implements these functions with:
---// bpo-43760 added PyThreadState_EnterTracing() to Python 3.11.0a2 #if PY_VERSION_HEX < 0x030B00A2 && !defined(PYPY_VERSION) static inline void PyThreadState_EnterTracing(PyThreadState *tstate) { tstate->tracing++; #if PY_VERSION_HEX >= 0x030A00A1 tstate->cframe->use_tracing = 0; #else tstate->use_tracing = 0; #endif } #endif // bpo-43760 added PyThreadState_LeaveTracing() to Python 3.11.0a2 #if PY_VERSION_HEX < 0x030B00A2 && !defined(PYPY_VERSION) static inline void PyThreadState_LeaveTracing(PyThreadState *tstate) { tstate->tracing--; int use_tracing = (tstate->c_tracefunc != NULL || tstate->c_profilefunc != NULL); #if PY_VERSION_HEX >= 0x030A00A1 tstate->cframe->use_tracing = use_tracing; #else tstate->use_tracing = use_tracing; #endif } #endif
This code was copied from my https://github.com/pythoncapi/pythoncapi_compat project. (I wrote the greenlet change.)
Is there anything left to do here?
IMO we are done, and I close the issue. See the issue #84128 for making PyThreadState opaque.
The DISPATCH() macro is not as efficient as it could be
This part was fixed early. Following comments were more about PyThreadState incompatible changes, how to migrate existing C extensions to Python 3.11 and how to design a new API which no longer access directly PyThreadState changes. In fact, that's already the topic of the issue #84128 that I created in 2020 and so we can continue the discussion there.
The main change related to PyThreadState was the use_tracing member which was moved. I added PyThreadState_EnterTracing() and PyThreadState_LeaveTracing() functions to Python 3.11 for that and so projects already use it.
I created #29121 to add PyThreadState_SetProfile() and PyThreadState_SetTrace() functions.
I abandoned this PR. @pablogsal added PyEval_SetProfileAllThreads() and PyEval_SetTraceAllThreads() functions to Python 3.12 (commit e34c82a) which should fit the use case, with a different design.
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields: