Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

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

Provide feedback

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

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
pablogsal merged 7 commits intopython:mainfrompablogsal:gh-93503
Aug 24, 2022

Conversation

pablogsal
Copy link
Member

@pablogsalpablogsal commentedJun 5, 2022
edited by bedevere-bot
Loading

@pablogsalpablogsal changed the titleGH-93503 Add thread-specific APIs to set profiling and tracing functions in the C-APIGH-93503: Add thread-specific APIs to set profiling and tracing functions in the C-APIJun 5, 2022
@pablogsalpablogsal requested a review fromvstinnerJune 5, 2022 00:26
@markshannon
Copy link
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 takingPyThreadState can take any thread state.
    However, most C-API functions that takePyThreadState 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 fromobj?

To be fair, this isn't just an issue with these functions, but any function that takes aPyThreadState 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
Copy link
MemberAuthor

pablogsal commentedJun 6, 2022
edited
Loading

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
Copy link
Member

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

@pablogsal
Copy link
MemberAuthor

pablogsal commentedJun 6, 2022
edited
Loading

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
Copy link
MemberAuthor

@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
Copy link
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
Copy link
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
Copy link
MemberAuthor

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

@pablogsal
Copy link
MemberAuthor

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

Copy link
Member

@vstinnervstinner left a comment

Choose a reason for hiding this comment

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

I suggestall_threads=True rather thanrunning_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)

@pablogsal
Copy link
MemberAuthor

@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
Copy link
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
Copy link
MemberAuthor

I have updated the APIs to use separate functions.

@pablogsal
Copy link
MemberAuthor

What would be the API in that case?

threading.settrace_all_threads andthreading.setprofile_all_threads

@@ -7033,6 +7033,20 @@ PyEval_SetProfile(Py_tracefunc func, PyObject *arg)
}
}

void
Copy link
Member

Choose a reason for hiding this comment

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

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()?

Copy link
MemberAuthor

@pablogsalpablogsalAug 4, 2022
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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.

Copy link
Member

@vstinnervstinner left a comment

Choose a reason for hiding this comment

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

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

@pablogsal
Copy link
MemberAuthor

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

Copy link
Member

@markshannonmarkshannon left a comment

Choose a reason for hiding this comment

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

One minor issue with the docs, otherwise LGTM.

@vstinner
Copy link
Member

Nice enhancement, thanks.

@vstinner
Copy link
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
Copy link
MemberAuthor

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

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@markshannonmarkshannonmarkshannon left review comments

@vstinnervstinnerAwaiting 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
@pablogsal@markshannon@vstinner@bedevere-bot

[8]ページ先頭

©2009-2025 Movatter.jp