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

Ensure new non-main threads have their names set #140

Merged
merged 1 commit into from
Apr 17, 2024

Conversation

jamienicol
Copy link
Contributor

No description provided.

@jamienicol
Copy link
Contributor Author

Before: https://share.firefox.dev/3Ui2gF8
After: https://share.firefox.dev/4aSsmny

Ignore the "Thread-2", "Thread-3" etc in the after profile. Those are indeed the "correct" names due to https://bugzilla.mozilla.org/show_bug.cgi?id=1891726

@mstange
Copy link
Owner

mstange commented Apr 17, 2024

Oh yeah, this looks like an oversight.

I think this wasn't needed on Linux for the following reasons:

  • For new threads, we first inherit the name from the parent (while handling FORK records) and then update the name once we see the COMM record.
  • For existing threads when samply record does the profiling, we call register_existing_thread with the name from /proc/{pid}/task/{tid}/comm.
  • For existing threads when perf record does the profiling, perf synthesizes COMM events at the start of the trace.

So it seems either simpleperf does not synthesize COMM events, or maybe it puts the "thread map" in a different place.

Anyway, this change looks like a clear improvement, merging.

@mstange mstange merged commit bc64595 into mstange:main Apr 17, 2024
12 checks passed
@mstange
Copy link
Owner

mstange commented Apr 17, 2024

So it seems either simpleperf does not synthesize COMM events, or maybe it puts the "thread map" in a different place.

It seems the COMM events are there, but the MMAP2 events come first, so the thread already exists by the time we see the COMM events.

@jamienicol jamienicol deleted the thread-names branch April 22, 2024 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants