Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-43760: Add PyThreadState_EnterTracing() #28542

Merged
merged 1 commit into from Oct 15, 2021
Merged

Conversation

@vstinner
Copy link
Member

@vstinner vstinner commented Sep 24, 2021

Add PyThreadState_EnterTracing(), PyThreadState_LeaveTracing() and
PyThreadState_IsTracing() functions to the limited C API to suspend
and resume tracing and profiling.

Add an unit test on the PyThreadState C API to _testcapi.

Add also internal _PyThreadState_DisableTracing() and
_PyThreadState_ResetTracing().

https://bugs.python.org/issue43760

@vstinner
Copy link
Member Author

@vstinner vstinner commented Sep 24, 2021

@markshannon @pablogsal: Would you mind to check if my proposed C API is correct?

The documentation doesn't explain on purpose that the change is only applied to the current frame and how it is inherited or not to older and newer frames. Should we explain it?

@vstinner
Copy link
Member Author

@vstinner vstinner commented Sep 24, 2021

@pablogsal: If possible, I would prefer to land this C API in Python 3.10, since use_tracing was removed in Python 3.10. But since you told me that it's not going to happen (I'm not sure why, a new C API should not break anything), my PR targets Python 3.11.

But I have to add a compatibility anyway to https://github.com/pythoncapi/pythoncapi_compat for projects which want to use these new C API, to support old Python versions. In Python 3.9, these compatibility functions would use tstate->use_tracing. In Python 3.10, they would use tstate->cframe.use_tracing. In Python 3.11, they would not be declared, but use the new functions from the Python C API.

Without pythoncapi_compat, projects can use an #ifdef with 3 code paths:

  • Python <= 3.9: tstate->use_tracing
  • Python 3.10: tstate->cframe.use_tracing
  • Python 3.11: use the new C API

@pablogsal
Copy link
Member

@pablogsal pablogsal commented Sep 24, 2021

@pablogsal: If possible, I would prefer to land this C API in Python 3.10, since use_tracing was removed in Python 3.10. But since you told me that it's not going to happen (I'm not sure why, a new C API should not break anything), my PR targets Python 3.11.

Sorry Victor, but there is no way we can be adding any new APIs after feature freeze. What is more important: we cannot add a new C API in a release candidate. What is even more important: we cannot add a new C API in a release candidate that won't be tested by the community because the only release will directly be Python 3.10.0. All the rules I abide as release manager forces me to forbid backporting this to 3.10 unfortunately.

@vstinner
Copy link
Member Author

@vstinner vstinner commented Sep 24, 2021

Pablo: "Sorry Victor, but there is no way we can be adding any new APIs after feature freeze. What is more important: we cannot add a new C API in a release candidate."

pythoncapi_compat will provide a practical solution to have a single code base support Python 3.9 and older, 3.10 and 3.11 and newer (well, all Python versions), so no need for #ifdef.

Projects which don't want pythoncapi_compat like greenlet or Cython already have their own #ifdef code.

@vstinner
Copy link
Member Author

@vstinner vstinner commented Sep 24, 2021

I created a PR to add these 3 functions to pythoncapi_compat: pythoncapi/pythoncapi_compat#12

Example:

// [bpo-43760](https://bugs.python.org/issue43760) added PyThreadState_DisableTracing() to Python 3.11.0a1
#if PY_VERSION_HEX < 0x030B00A1 && !defined(PYPY_VERSION)
static inline void
PyThreadState_DisableTracing(PyThreadState *tstate)
{
#if PY_VERSION_HEX >= 0X030A00B1
    tstate->cframe->use_tracing = 0;
#else
    tstate->use_tracing = 0;
#endif
}
#endif

@markshannon
Copy link
Contributor

@markshannon markshannon commented Sep 24, 2021

The meaning of use_tracing is a fast check to tell the interpreter whether it needs to do slow checks for tracing.
We should not be providing an API to set it.

We already have an API to turn off tracing, PyEval_SetTrace(NULL, NULL). We don't need another one.

@vstinner
Copy link
Member Author

@vstinner vstinner commented Sep 24, 2021

We already have an API to turn off tracing, PyEval_SetTrace(NULL, NULL). We don't need another one.

The use case is to disable tracing and profiling temporarily, do something, and then reenable if (if needed, or keep it disabled). See: https://bugs.python.org/issue43760#msg393410

There is no PyEval_GetTrace() function nor PyEval_GetProfile() function. How do you reenable tracing and profiling with the existing C API?

CPython always had a feature to disable tracing and profiling even if a trace function or a profile function was set: it's the use_tracing flag. Are you suggesting to remove this feature? The features is used by at least 5 projects in the wild: https://bugs.python.org/issue43760#msg402150

@vstinner
Copy link
Member Author

@vstinner vstinner commented Sep 24, 2021

About the portability of this C API, while working on my pythoncapi_compat PR pythoncapi/pythoncapi_compat#12, I noticed that PyPy has no such API. Moreover, PyPy doesn't have PyEval_SetTrace() and PyEval_SetProfile() functions.

This PR is a solution specific to CPython, it's more about the backward compatibility, then designing a future proof C API working on any Python implementation.

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Sep 24, 2021

Can someone explain to me what pythoncapi_compat is?

@vstinner
Copy link
Member Author

@vstinner vstinner commented Sep 24, 2021

Can someone explain to me what pythoncapi_compat is?

It's like the six module, but for C extensions :-) I wrote it to have a single code base supporting old and new Python versions, especially supporting old and new versions of the Python C API. Projects are freed to use it or not ;-) As many projects chose to write their own compatibility layer for Python 2 and Python 3.

https://github.com/pythoncapi/pythoncapi_compat

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Sep 24, 2021

Ah cool. Makes sense.

Next question. What use does Cython have for this field? Doe it turn tracing om/off, or does it just need to read the flag?

@vstinner
Copy link
Member Author

@vstinner vstinner commented Sep 24, 2021

Guido: "Next question. What use does Cython have for this field? Doe it turn tracing om/off, or does it just need to read the flag?"

I wrote an analysis of use_tracing usage: https://bugs.python.org/issue43760#msg402594

@markshannon
Copy link
Contributor

@markshannon markshannon commented Oct 4, 2021

This will need updating to set use_tracing to 255, not 1. See #28723

@markshannon
Copy link
Contributor

@markshannon markshannon commented Oct 4, 2021

Presumably these functions are to turn off tracing when entering a tracing function. tstate->tracing should suffice for that, but I assume that Cython will want to set use_tracing for performance reasons.

Given that, rather than phrasing the API in terms of suspending tracing, we should phrase it term of entering the tracing function.
PyEnterTracing() and PyLeaveTracing(). Tracing is suspended if the number of calls to PyEnterTracing() exceeds the number of calls to PyLeaveTracing().

The semantics would be as follows:

void PyEnterTracing(tstate) {
     tstate->tracing++;
}

void PyLeaveTracing(tstate) {
     tstate->tracing--;
}

The actual implementation would update tstate->cframe->use_tracing as appropriate.

@vstinner vstinner changed the title bpo-43760: Add PyThreadState_DisableTracing() bpo-43760: Add PyThreadState_EnterTracing() Oct 13, 2021
@vstinner
Copy link
Member Author

@vstinner vstinner commented Oct 13, 2021

@markshannon: I rewrote the PR, would you mind to review it again?

I renamed functions of the public C API.

The public C API uses tracing++ and tracing--, whereas the internal C API only sets use_tracing.

I also elaborated the PyThreadState_IsTracing() documentation:

   Is tracing or profiling enabled in the Python thread state *tstate*?

   Return 0 if no tracing function and no profiling function is set.

   Return 0 if tracing and profiling is suspended by the
   :c:func:`PyThreadState_EnterTracing` function.

@encukou
Copy link
Member

@encukou encukou commented Oct 14, 2021

Consider adding this only to the public API, and only putting it in the stable ABI after it's proven to be useful and complete.

@vstinner
Copy link
Member Author

@vstinner vstinner commented Oct 15, 2021

@encukou:

Consider adding this only to the public API, and only putting it in the stable ABI after it's proven to be useful and complete.

Good idea. I excluded the new functions from the limited C API. I don't think that Cython needs the stable ABI for profiling now. There is a work-in-progress to restrict Cython to the limited C API, but as you said, we can add the Tracing API to the stable ABI later.

@vstinner
Copy link
Member Author

@vstinner vstinner commented Oct 15, 2021

I created a new PR in pythoncapi_compat to provide these new functions on Python 3.10 and older: pythoncapi/pythoncapi_compat#13

My implementation handles Python 3.10 which added PyThreadState.cframe. It avoids having 3 code paths:

  • Python 3.11.0a2 and newer which uses 0 or 255 for use_tracing
  • Python 3.10 which uses cframe
  • Python 3.9 and older

Add PyThreadState_EnterTracing() and PyThreadState_LeaveTracing()
functions to the limited C API to suspend and resume tracing and
profiling.

Add an unit test on the PyThreadState C API to _testcapi.

Add also internal _PyThreadState_DisableTracing() and
_PyThreadState_ResetTracing().
@vstinner
Copy link
Member Author

@vstinner vstinner commented Oct 15, 2021

I'm not sure about PyThreadState_IsTracing(): I prefer to remove it for now.

Cython __Pyx_IsTracing() function checks 4 members:

  • PyThreadState.use_tracing
  • PyThreadState.tracing
  • PyThreadState.c_tracefunc
  • PyThreadState.c_profilefunc

I'm not sure why Cython checks use_tracing and again checks c_tracefunc and c_profilefunc. Maybe it's to handle the case when CYTHON_TRACE is set to 0 at build time. If it's the case, maybe testing CYTHON_TRACE could be differently, outside __Pyx_IsTracing().

  #define __Pyx_IsTracing(tstate, check_tracing, check_funcs) \
     (unlikely((tstate)->cframe->use_tracing) && \
         (!(check_tracing) || !(tstate)->tracing) && \
         (!(check_funcs) || (tstate)->c_profilefunc || (CYTHON_TRACE && (tstate)->c_tracefunc)))

vstinner added a commit to vstinner/cython that referenced this issue Oct 15, 2021
Profile.c now uses PyThreadState_EnterTracing() and
PyThreadState_LeaveTracing() added to Python 3.11.0a2:
python/cpython#28542

Replace __Pyx_SetTracing() with __Pyx_EnterTracing() and
__Pyx_LeaveTracing().
@vstinner
Copy link
Member Author

@vstinner vstinner commented Oct 15, 2021

And here is a draft PR for Cython: cython/cython#4411

vstinner added a commit to vstinner/cython that referenced this issue Oct 15, 2021
Profile.c now uses PyThreadState_EnterTracing() and
PyThreadState_LeaveTracing() added to Python 3.11.0a2:
python/cpython#28542

When these functions are used, Cython no longer access directly to
PyThreadState.cframe.use_tracing.

Replace __Pyx_SetTracing() with __Pyx_EnterTracing() and
__Pyx_LeaveTracing().
vstinner added a commit to vstinner/cython that referenced this issue Oct 15, 2021
Profile.c now uses PyThreadState_EnterTracing() and
PyThreadState_LeaveTracing() added to Python 3.11.0a2:
python/cpython#28542

When these functions are used, Cython no longer set directly
PyThreadState.cframe.use_tracing.

Replace __Pyx_SetTracing() with __Pyx_EnterTracing() and
__Pyx_LeaveTracing().
@vstinner
Copy link
Member Author

@vstinner vstinner commented Oct 15, 2021

And here is a draft PR for Cython: cython/cython#4411

On Python 3.11.0a2 with this PR, Cython no longer sets directly PyThreadState.cframe.use_tracing. But its __Pyx_IsTracing() macro still reads directly PyThreadState.cframe.use_tracing.

@vstinner
Copy link
Member Author

@vstinner vstinner commented Oct 15, 2021

Azure Pipelines PR: test_sendfile_close_peer_in_the_middle_of_receiving() failed on Windows x64.

@vstinner vstinner merged commit 547d26a into python:main Oct 15, 2021
11 of 12 checks passed
@vstinner vstinner deleted the tstate branch Oct 15, 2021
@vstinner
Copy link
Member Author

@vstinner vstinner commented Oct 15, 2021

I created a new PR in pythoncapi_compat to provide these new functions on Python 3.10 and older: pythoncapi/pythoncapi_compat#13

I also removed PyThreadState_IsTracing(), final pythoncapi_compat change: pythoncapi/pythoncapi_compat@10fde24


PyAPI_FUNC(void) _PyThreadState_Init(
PyThreadState *tstate);
PyAPI_FUNC(void) _PyThreadState_DeleteExcept(
_PyRuntimeState *runtime,
PyThreadState *tstate);

static inline void
_PyThreadState_DisableTracing(PyThreadState *tstate)
Copy link
Contributor

@markshannon markshannon Oct 15, 2021

Could you rename this to "SuspendTracing" as it doesn't really disable it.

Copy link
Member Author

@vstinner vstinner Oct 15, 2021

What I like in the internal C API is that we have a great freedom to change anything :-) Sure, I will write a PR to rename functions.

Copy link
Member

@gvanrossum gvanrossum Oct 15, 2021

Hoe about Pause/Resume?

Copy link
Member Author

@vstinner vstinner Oct 15, 2021

I did a quick Google search. It seems like "suspend" is usually used to "suspend a whole operating time", whereas there are debugger and profilers with a "pause" button and "pause profiling" sounds to be "commonly" used.

My vote goes to PauseTracing/ResumeTracing.

@markshannon: What is your last word on naming? :-)

Copy link
Member Author

@vstinner vstinner Oct 18, 2021

I created #29032

}

static inline void
_PyThreadState_ResetTracing(PyThreadState *tstate)
Copy link
Contributor

@markshannon markshannon Oct 15, 2021

Likewise this should be "ResumeTracing"

@markshannon
Copy link
Contributor

@markshannon markshannon commented Oct 15, 2021

@vstinner Sorry my comments are a bit late. I hadn't realized you were going to merge this yet.

@vstinner
Copy link
Member Author

@vstinner vstinner commented Oct 15, 2021

@vstinner Sorry my comments are a bit late. I hadn't realized you were going to merge this yet.

Well, the situation was stuck since last May. I would like to move on to unblock the issue.

I would like to test my recent C API changes on third party projects, but I'm blocked by Cython which is not compatible yet with the current Python main branch.

scoder pushed a commit to cython/cython that referenced this issue Oct 18, 2021
Instead of __Pyx_SetTracing(), Profile.c now uses PyThreadState_EnterTracing() and PyThreadState_LeaveTracing(), which were added to Python 3.11.0a2:
python/cpython#28542

When these functions are used, Cython no longer sets directly PyThreadState.cframe.use_tracing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants