The DISPATCH() macro is not as efficient as it could be (move PyThreadState.use_tracing)

[markshannon]

BPO	43760
Nosy	@gvanrossum, @rhettinger, @scoder, @vstinner, @encukou, @ambv, @markshannon, @hroncok, @pablogsal, @miss-islington, @erlend-aasland, @Xtrem532
PRs	bpo-43760: Streamline dispatch sequence for machines without computed gotos. #25244 bpo-43760: Speed up check for tracing in interpreter dispatch #25276 [3.10] bpo-43760: Ensure that older Cython generated code compiles under 3.10. #28474 [3.10] bpo-43760: Ensure that older Cython generated code compiles under 3.10 #28498 bpo-43760: Document PyThreadState.use_tracing removal #28527 [3.10] bpo-43760: Document PyThreadState.use_tracing removal (GH-28527) #28529 bpo-43760: Add PyThreadState_EnterTracing() #28542 bpo-43760: Check for tracing using 'bitwise or' instead of branch in dispatch. #28723 bpo-43760: Rename _PyThreadState_DisableTracing() #29032

^{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:

assignee = None
closed_at = None
created_at = <Date 2021-04-07.09:35:49.770>
labels = ['interpreter-core', 'expert-C-API', '3.10', 'performance']
title = 'The DISPATCH() macro is not as efficient as it could be (move PyThreadState.use_tracing)'
updated_at = <Date 2021-11-08.17:16:37.213>
user = 'https://github.com/markshannon'

bugs.python.org fields:

activity = <Date 2021-11-08.17:16:37.213>
actor = 'vstinner'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Interpreter Core', 'C API']
creation = <Date 2021-04-07.09:35:49.770>
creator = 'Mark.Shannon'
dependencies = []
files = []
hgrepos = []
issue_num = 43760
keywords = ['patch']
message_count = 46.0
messages = ['390410', '390522', '390951', '393379', '393384', '393385', '393389', '393390', '393401', '393403', '393404', '393407', '393410', '393425', '393459', '393466', '393666', '402150', '402162', '402216', '402225', '402231', '402234', '402235', '402358', '402360', '402362', '402415', '402416', '402426', '402432', '402434', '402481', '402496', '402525', '402594', '402603', '403166', '403217', '404020', '404024', '404025', '404173', '404197', '404600', '405968']
nosy_count = 13.0
nosy_names = ['gvanrossum', 'rhettinger', 'jpe', 'scoder', 'vstinner', 'petr.viktorin', 'lukasz.langa', 'Mark.Shannon', 'hroncok', 'pablogsal', 'miss-islington', 'erlendaasland', 'Xtrem532']
pr_nums = ['25244', '25276', '28474', '28498', '28527', '28529', '28542', '28723', '29032']
priority = None
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue43760'
versions = ['Python 3.10']

[markshannon]

The DISPATCH() macro has two failings.

1. Its check for tracing involves too much pointer chaser.

2. The logic assumes that computed-gotos is the "fast path" which makes switch dispatch, and therefore Python on Windows unnecessarily slow.

[markshannon]

New changeset 28d28e0 by Mark Shannon in branch 'master':
bpo-43760: Streamline dispatch sequence for machines without computed gotos. (GH-25244)
28d28e0

[markshannon]

New changeset 9e7b207 by Mark Shannon in branch 'master':
bpo-43760: Speed up check for tracing in interpreter dispatch (bpo-25276)
9e7b207

[hroncok]

I 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;
| ^~~~~~~~~~~
| tracing

This 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]

Fedora 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=1958938

[markshannon]

At 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.

[markshannon]

If there is no C-API function that supports your needs, feel free to suggest one.

[hroncok]

Disclaimer: 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/

[hroncok]

scikit-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;
| ^~~~~~~~~~~
| tracing

The usage comes from https://github.com/cython/cython/blob/master/Cython/Utility/Profile.c

[encukou]

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 :(

[markshannon]

But what does "use it" mean?
What does setting tstate->use_tracing = 1 do?
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.

[rhettinger]

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.

[vstinner]

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:

• python-greenlet/greenlet@6c5f096
• python-greenlet/greenlet@352b974

It uses:

• "tstate->use_tracing = 0;"
• "tstate->use_tracing = (tstate->tracing <= 0 && (...)"

dipy: https://bugzilla.redhat.com/show_bug.cgi?id=1958203

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) {"

yappi: https://bugzilla.redhat.com/show_bug.cgi?id=1958896

• "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;                               \
      }                                                                                  \
  }

---