1533675 - Name all threads in Firefox

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Tarek Ziadé (:tarek)]

Some threads are started without a name (which is technically fine, the thread takes the process command name), but it makes it hard in GetProcInfo() to interpret thread activity.

Since it's just a label, let's see if we can track all unnamed threads and set meaningful names.

Comment 1

[Tarek Ziadé (:tarek)]

One first step is tom make sure that NS_SetCurrentThreadName() is call for every PR_CreateThread() call we own,

These are not set:

https://searchfox.org/mozilla-central/source/dom/media/GraphRunner.cpp#32
https://searchfox.org/mozilla-central/source/toolkit/recordreplay/ipc/ParentForwarding.cpp#397
https://searchfox.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#4270
https://searchfox.org/mozilla-central/source/widget/windows/AudioSession.cpp#260
https://searchfox.org/mozilla-central/source/security/nss/lib/ssl/sslsnce.c#1516
https://searchfox.org/mozilla-central/source/nsprpub/pr/src/misc/prtpool.c#253
https://searchfox.org/mozilla-central/source/nsprpub/pr/src/misc/prtpool.c#652
https://searchfox.org/mozilla-central/source/nsprpub/pr/src/misc/prtpool.c#663
https://searchfox.org/mozilla-central/source/nsprpub/pr/src/misc/prtpool.c#672
https://searchfox.org/mozilla-central/source/nsprpub/pr/src/misc/pralarm.c#159
https://searchfox.org/mozilla-central/source/nsprpub/pr/src/md/unix/uxproces.c#742
https://searchfox.org/mozilla-central/source/nsprpub/pr/src/cplus/rcthread.cpp#42

Comment 2

[Tarek Ziadé (:tarek)]

We should look in JS also, if some js::Thread are created without js::Thread::SetName() calls

Comment 3

[Tarek Ziadé (:tarek)]

Like here -> https://searchfox.org/mozilla-central/source/js/src/shell/js.cpp#4161
WorkerMain does not SetName

Comment 4

[Tarek Ziadé (:tarek)]

Do we want to add AUTO_PROFILER_REGISTER_THREAD calls everywhere as well? I am not sure.. asking the experts :)

Comment 5

[Tarek Ziadé (:tarek)]

We also need to track pthread_create() calls.

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Tarek Ziadé (:tarek) from comment #4)

Do we want to add AUTO_PROFILER_REGISTER_THREAD calls everywhere as well? I am not sure.. asking the experts :)

My first not-yet-expert reaction would be "yes please", so we can see them in profiles (if the user picks them by name, only a limited set is enabled by default, so the impact will remain small in most cases).

Ideally whoever coded these thread creations would be best placed to pick a name, and decide if a thread should not be profiled for some reason; I guess the appropriate reviewers will be able to make that call.

Comment 7

[Tarek Ziadé (:tarek)]

Thanks for the feedback Gerald. I guess I'll add for now the SetCurrentThreadName() calls and poke people around to see if we want to profile them

Comment 8

[Tarek Ziadé (:tarek)]

Attachment: Bug 1533675 - Name all threads in Firefox r?ehsan

This patch adds thread names where they are missing

Comment 9

[Tarek Ziadé (:tarek)]

This first patch fixes most threads I see in GetProcInfo() via GDB/LLDB so we can interpret the numbers. I will do a separate patch into the NSPR project

Comment 10

[Tarek Ziadé (:tarek)]

try build https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6212d319b401fc3eab2726223fa152fba9bfc08

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7fc2d7c0c634
Name all threads in Firefox r=Ehsan

Comment 12

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/7fc2d7c0c634