← 返回首页
GH-93503: Add thread-specific APIs to set profiling and tracing functions in the C-API by pablogsal · Pull Request #93504 · python/cpython · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Include my email address so I can be contacted
Cancel Submit feedback

Saved searches

Use saved searches to filter your results more quickly

Cancel Create saved search
Appearance settings
Resetting focus

GH-93503: Add thread-specific APIs to set profiling and tracing functions in the C-API#93504

Merged
pablogsal merged 7 commits into
python:mainfrom
pablogsal:gh-93503
Aug 24, 2022
Merged

GH-93503: Add thread-specific APIs to set profiling and tracing functions in the C-API#93504
pablogsal merged 7 commits into
python:mainfrom
pablogsal:gh-93503

Conversation

pablogsal commented Jun 5, 2022 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

pablogsal requested a review from markshannon as a code owner June 5, 2022 00:24
pablogsal changed the title GH-93503 Add thread-specific APIs to set profiling and tracing functions in the C-API GH-93503: Add thread-specific APIs to set profiling and tracing functions in the C-API Jun 5, 2022
pablogsal requested a review from vstinner June 5, 2022 00:26
vstinner reviewed Jun 6, 2022
View reviewed changes
Comment thread Doc/c-api/init.rst Outdated Show resolved Hide resolved
Comment thread Python/ceval.c Outdated Show resolved Hide resolved
Comment thread Python/ceval.c Outdated Show resolved Hide resolved
Comment thread Python/ceval.c Outdated Show resolved Hide resolved

markshannon commented Jun 6, 2022

Copy link
Copy Markdown
Member

I think this is too error prone, for a couple of reasons.

  • The main reason is that this sets the expectation that C-API functions taking PyThreadState can take any thread state.
    However, most C-API functions that take PyThreadState are likely to leave the VM in an invalid state if passed anything other than the current thread state.
  • What happens when the thread state belongs to a different sub-interpreter from obj?

To be fair, this isn't just an issue with these functions, but any function that takes a PyThreadState parameter.

#93503 is about adding an API to set tracing/profiling on all threads.
Why not add an API that does that?
void PyEval_SetThreadProfileAllThreads(Py_tracefunc func, PyObject *obj)

Also, I don't like offering operations in the C-API that aren't available in Python. We shouldn't be forcing people to write C.

pablogsal commented Jun 6, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

Why not add an API that does that?

I am fine with that. Do you want me to update this PR or create another one?

Also, I don't like offering operations in the C-API that aren't available in Python. We shouldn't be forcing people to write C.

That ship already sailed. Do mean that you would like to also expose such an API in the Python layer?

vstinner commented Jun 6, 2022

Copy link
Copy Markdown
Member

The threading module has setprofile() and settrace() functions.

pablogsal commented Jun 6, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

The threading module has setprofile() and settrace() functions.

But those only work for newly created threads as I noted in the issue. They are used only when new threads are bootstrapped. What we are trying to do is to add APIs that affect all current threads.

We also cannot change those for backwards compatibility, but we could place the new APIs in the thread module for Python access.

pablogsal commented Jun 6, 2022

Copy link
Copy Markdown
Member Author

@markshannon you ok with this plan?

  • Changing the proposed C APIs to set the trace/profile functions in all threads.
  • Add a python API in the thread module that calls the C API.

markshannon commented Jun 6, 2022

Copy link
Copy Markdown
Member

@markshannon you ok with this plan?

* Changing the proposed C APIs to set the trace/profile functions in all threads. * Add a python API in the thread module that calls the C API.

Yes. Sounds good to me.

vstinner commented Jun 6, 2022

Copy link
Copy Markdown
Member

But those only work for newly created threads as I noted in the issue. They are used only when new threads are bootstrapped. What we are trying to do is to add APIs that affect all current threads.

Would it make sense to add a parameter to these threading functions or to sys functions to apply the new tracing/profiling functions to existing threads?

pablogsal commented Jun 6, 2022

Copy link
Copy Markdown
Member Author

I am ambivalent, but the new parameter sounds good if you prefer it that way.

pablogsal commented Jun 6, 2022

Copy link
Copy Markdown
Member Author

Ok, I have made a first version of the idea proposed by @markshannon. Please @vstinner @markshannon review again.

markshannon reviewed Jun 6, 2022
View reviewed changes
Comment thread Doc/library/threading.rst Outdated Show resolved Hide resolved
vstinner reviewed Jun 6, 2022
View reviewed changes

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

I suggest all_threads=True rather than running_threads=True: in your implementation, threading.settrace() changes old and new threads, all threads, not only old ("running") threads.

With the PR, we have:

  • sys.settrace(): current thread
  • threading.settrace(): new threads
  • threading.settrace(all_threads=True): all threads (old, new and current)

Comment thread Doc/library/threading.rst Outdated Show resolved Hide resolved

pablogsal commented Aug 1, 2022

Copy link
Copy Markdown
Member Author

@vstinner I'm going to change this PR to follow @markshannon request and make it a separate function in Python instead of a parameter.

vstinner commented Aug 3, 2022

Copy link
Copy Markdown
Member

@vstinner I'm going to change this PR to follow @markshannon request and make it a separate function in Python instead of a parameter.

Sure, as you want. What would be the API in that case?

pablogsal commented Aug 4, 2022

Copy link
Copy Markdown
Member Author

I have updated the APIs to use separate functions.

pablogsal commented Aug 4, 2022

Copy link
Copy Markdown
Member Author

What would be the API in that case?

threading.settrace_all_threads and threading.setprofile_all_threads

vstinner reviewed Aug 4, 2022
View reviewed changes
Comment thread Python/ceval.c
}
}

void

vstinner Aug 4, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

It's not great that the function cannot report errors. While not returning -1 on error and pass the exception to the caller, rather than handling it with _PyErr_WriteUnraisableMsg()?

pablogsal Aug 4, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

I am mimicking what we do already in PyEval_SetProfile and to avoid surprises I think we should keep these two as synchronized as possible.

vstinner Aug 4, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

We cannot fix the API of old functions, but we can avoid past mistakes in newly added functions.

vstinner Aug 4, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

If you want to ignore exceptions on purpose, please mention it in the function documentation.

pablogsal Aug 4, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

If you want to ignore exceptions on purpose, please mention it in the function documentation.

Yeah, I want to do that because I don't want to stop setting the profile function on other threads if an exception in one fails. Unless you strongly disagree, I will document this.

Comment thread Python/ceval.c Show resolved Hide resolved
pablogsal requested a review from vstinner August 4, 2022 15:19
vstinner reviewed Aug 5, 2022
View reviewed changes

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Honestly, I don't know if HEAD_LOCK/HEAD_UNLOCK is needed, I'm not sure if the GIL protects us here. The PyThreadState_Next() API is not well documented for thread safety :-(

Comment thread Python/sysmodule.c Outdated Show resolved Hide resolved
pablogsal requested a review from vstinner August 23, 2022 18:52

pablogsal commented Aug 24, 2022

Copy link
Copy Markdown
Member Author

I plan to land this this week or the next one to avoid merge conflicts. @markshannon can you give it a last pass?

20 hidden items Load more…
markshannon reviewed Aug 24, 2022
View reviewed changes

markshannon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

One minor issue with the docs, otherwise LGTM.

Comment thread Doc/c-api/init.rst Show resolved Hide resolved
pablogsal merged commit e34c82a into python:main Aug 24, 2022
pablogsal deleted the gh-93503 branch August 24, 2022 22:21

vstinner commented Sep 8, 2022

Copy link
Copy Markdown
Member

Nice enhancement, thanks.

vstinner commented Sep 8, 2022

Copy link
Copy Markdown
Member

You should document the newly added C API functions to: https://docs.python.org/dev/whatsnew/3.12.html#c-api-changes

You should also document the new threading functions in the same document, in a threading section, near: https://docs.python.org/dev/whatsnew/3.12.html#improved-modules

pablogsal commented Sep 8, 2022

Copy link
Copy Markdown
Member Author

You should document the newly added C API functions to: docs.python.org/dev/whatsnew/3.12.html#c-api-changes

You should also document the new threading functions in the same document, in a threading section, near: docs.python.org/dev/whatsnew/3.12.html#improved-modules

#96681

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

markshannon markshannon left review comments
vstinner Awaiting requested review from vstinner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add public APIs to set trace and profile function in other threads.

4 participants

Footer

© 2026 GitHub, Inc.