Registering a thread with the profiler using profiler_register_thread has two advantages: - We can profile them - and TaskTracer knows the thread's name. My plan in this bug is to: - Add profiler_register_thread calls in all the thread root functions that have PR_SetCurrentThreadName but no profiler_register_thread call - Change callers of NS_SetThreadName to use NS_NewNamedThread instead, and remove NS_SetThreadName - Make NS_SetThreadName automatically register the new thread with the profiler.

Comment on attachment 8818169 [details] Bug 1323100 - Remove aThread argument from nsThreadPoolNaming::SetThreadPoolName, which is the last user of NS_SetThreadName. https://reviewboard.mozilla.org/r/98286/#review98844 ::: xpcom/glue/nsThreadUtils.cpp:466 (Diff revision 1) > +nsThreadPoolNaming::NewThreadWithThreadPoolName(const nsACString& aPoolName, > + nsIThread** aThread) > +{ > + nsCString name(aPoolName); > + name.AppendLiteral(" #"); > + name.AppendInt(++mCounter, 10); // The counter is declared as volatile when you are here, can you please change the comment from "volatile" to "atomic"? thanks.

After these patches, if I use Activity Monitor on macOS to get a sample of my Firefox process, I see the following threads which are not registered with the profiler: - js::Thread threads, e.g. the JS helper threads which do things like concurrent GC, source compression, WASM compilation, and the MachExceptionHandler threads and the JS Watchdog thread - The breakpad exception handler thread, created using pthread_create in ExceptionHandler::Setup - The SharedMemoryBasic PortServerThread, created using pthread_create in SetupMachMemory - com.apple.NSEventThread, probably created inside Cocoa for NSScrollView "APZ" - Idle DOM worker threads, which only name themselves and register themselves when they have a WorkerThreadPrimaryRunnable - libavcodec frame_worker_thread threads, created using pthread_create in ff_frame_thread_init - Grand Central Dispatch threads - CVDisplayLink threads, created by CVDisplayLinkCreateWithActiveCGDisplays in OSXDisplay::EnableVsync - The profiler Sampler thread There are a few more results for pthread_create inside mozilla-central. Also, with these patches we don't profile PR_CreateThread threads that are created inside NSPR and NSS. I didn't see any of those in Activity Monitor though.

Comment on attachment 8820653 [details] Bug 1323100 - Use NS_NewNamedThread to name the proxy resolution thread. https://reviewboard.mozilla.org/r/100114/#review100624

Comment on attachment 8820655 [details] Bug 1323100 - Use NS_NewNamedThread for the Android Audio thread. https://reviewboard.mozilla.org/r/100118/#review100628

Comment on attachment 8820656 [details] Bug 1323100 - Use NS_NewNamedThread for the Link Monitor thread. https://reviewboard.mozilla.org/r/100120/#review100630

Comment on attachment 8820657 [details] Bug 1323100 - Use NS_NewNamedThread for the Wifi Monitor thread. https://reviewboard.mozilla.org/r/100122/#review100636

Comment on attachment 8820658 [details] Bug 1323100 - Use NS_NewNamedThread for the Filewatcher IO thread. https://reviewboard.mozilla.org/r/100124/#review100638

Comment on attachment 8820659 [details] Bug 1323100 - Create a version of NS_NewNamedThread that accepts an nsACString. https://reviewboard.mozilla.org/r/100126/#review100640 ::: xpcom/glue/nsThreadUtils.h:106 (Diff revision 1) > + nsIRunnable* aInitialEvent = nullptr, > + uint32_t aStackSize = nsIThreadManager::DEFAULT_STACK_SIZE) > +{ > + static_assert(LEN <= 16, > + "Thread name must be no more than 16 characters"); > + return NS_NewNamedThread(nsDependentCString(aName), aResult, aInitialEvent, aStackSize); Might as well make this `nsDependentCString(aName, LEN)` or somesuch to avoid a `strlen` call at runtime.

Comment on attachment 8820659 [details] Bug 1323100 - Create a version of NS_NewNamedThread that accepts an nsACString. https://reviewboard.mozilla.org/r/100126/#review100640 > Might as well make this `nsDependentCString(aName, LEN)` or somesuch to avoid a `strlen` call at runtime. Good idea. I think it would need to be `nsDependentCString(aName, LEN - 1)` because `LEN` includes the null terminator.

Comment on attachment 8820662 [details] Bug 1323100 - Create nsThreadPoolNaming::GetNextThreadName. https://reviewboard.mozilla.org/r/100132/#review100672 This is fine, but please make the `GetNextThreadName` changes suggested below, and audit the series for those `.BeginReading()` calls. ::: xpcom/glue/nsThreadUtils.cpp:443 (Diff revision 1) > +nsCString > +nsThreadPoolNaming::GetNextThreadName(const char* aPoolName) I think it'd be better to have: `nsCString GetNextThreadName(const nsACString&)` and `template<size_t N> GetNextThreadName(const char (&)[N])` where the latter is a thin forwarding wrapper, and the former implements the logic here. That would make some of the callsites in later patches nicer (no `.BeginReading` calls), and should make the common literal string case slightly faster (string length already known). ::: xpcom/glue/nsThreadUtils.cpp:456 (Diff revision 1) > + > void > nsThreadPoolNaming::SetThreadPoolName(const nsACString& aPoolName, > nsIThread* aThread) > { > - nsCString name(aPoolName); > + nsCString name = GetNextThreadName(aPoolName.BeginReading()); e.g. this `.BeginReading()` call can be removed.

Comment on attachment 8820663 [details] Bug 1323100 - Use nsThreadPoolNaming::GetNextThreadName and NS_NewNamedThread for the mozStorage thread. https://reviewboard.mozilla.org/r/100134/#review100690

Comment on attachment 8820664 [details] Bug 1323100 - Use nsThreadPoolNaming::GetNextThreadName and NS_NewNamedThread in the DecodePool. https://reviewboard.mozilla.org/r/100136/#review100670 ::: image/DecodePool.cpp (Diff revision 1) > -#ifdef MOZ_ENABLE_PROFILER_SPS > - // InitCurrentThread() has assigned the thread name. > - profiler_register_thread(PR_GetThreadName(PR_GetCurrentThread()), &stackBaseGuess); > -#endif // MOZ_ENABLE_PROFILER_SPS Why are these bits getting deleted? AFAICT `nsThread` doesn't automatically register threads with the profiler, nor does `nsThreadManager`...?

(In reply to Nathan Froyd [:froydnj] from comment #38) > ::: image/DecodePool.cpp > (Diff revision 1) > > -#ifdef MOZ_ENABLE_PROFILER_SPS > > - // InitCurrentThread() has assigned the thread name. > > - profiler_register_thread(PR_GetThreadName(PR_GetCurrentThread()), &stackBaseGuess); > > -#endif // MOZ_ENABLE_PROFILER_SPS > > Why are these bits getting deleted? AFAICT `nsThread` doesn't automatically > register threads with the profiler, nor does `nsThreadManager`...? Sigh, rb UI again. The rest of the changes are OK, but I feel like there's something obvious I'm missing about this bit and would like to understand what's going on.

Comment on attachment 8820665 [details] Bug 1323100 - Use nsThreadPoolNaming::GetNextThreadName and NS_NewNamedThread in nsThreadPool. https://reviewboard.mozilla.org/r/100138/#review100692

Comment on attachment 8820664 [details] Bug 1323100 - Use nsThreadPoolNaming::GetNextThreadName and NS_NewNamedThread in the DecodePool. https://reviewboard.mozilla.org/r/100136/#review100670 > Why are these bits getting deleted? AFAICT `nsThread` doesn't automatically register threads with the profiler, nor does `nsThreadManager`...? The second to last patch in this series will register all named threads with the profiler. Sorry, I should have mentioned that in the commit message of this patch, or maybe reordered the patches to not lose functionality in the intermediate states.

Comment on attachment 8820666 [details] Bug 1323100 - Use nsThreadPoolNaming::GetNextThreadName for the DNS resolver thread. https://reviewboard.mozilla.org/r/100140/#review100698

(In reply to Markus Stange [:mstange] from comment #41) > > Why are these bits getting deleted? AFAICT `nsThread` doesn't automatically register threads with the profiler, nor does `nsThreadManager`...? > > The second to last patch in this series will register all named threads with > the profiler. Ah, that would be the obvious thing I was missing, then! > Sorry, I should have mentioned that in the commit message of this patch, or > maybe reordered the patches to not lose functionality in the intermediate > states. No problem!

Comment on attachment 8818168 [details] Bug 1323100 - Use NS_NewNamedThread in SingletonThreadHolder. https://reviewboard.mozilla.org/r/98284/#review100704

Comment on attachment 8820660 [details] Bug 1323100 - Use NS_NewNamedThread for CryptoTask threads. https://reviewboard.mozilla.org/r/100128/#review100706

Comment on attachment 8820654 [details] Bug 1323100 - Use NS_NewNamedThread for IndexedDB threads. https://reviewboard.mozilla.org/r/100116/#review100708

Comment on attachment 8820661 [details] Bug 1323100 - Assign names to all remaining threads that are created through NS_NewThread and create them using NS_NewNamedThread instead. https://reviewboard.mozilla.org/r/100130/#review100710 This is fine; just want to make sure I understand the naming of test threads as well. ::: dom/media/gtest/TestMP4Reader.cpp:47 (Diff revision 1) > } > > void Init() { > nsCOMPtr<nsIThread> thread; > nsCOMPtr<nsIRunnable> r = NewRunnableMethod(this, &TestBinding::ReadMetadata); > - nsresult rv = NS_NewThread(getter_AddRefs(thread), r); > + nsresult rv = NS_NewNamedThread("ReadMetadata", getter_AddRefs(thread), r); Do we need to name all the threads in tests, too? Is that necessary so we can assume that all `nsThread` threads have a name, and the profiler assumes that any thread it's profiler has a name, so we have to name threads in tests so the profiler doesn't blow up?

Comment on attachment 8818170 [details] Bug 1323100 - Add nsThreadManager::NewNamedThread API. https://reviewboard.mozilla.org/r/98288/#review100716

Comment on attachment 8820668 [details] Bug 1323100 - Make NS_NewNamedThread use nsThreadManager::NewNamedThread. https://reviewboard.mozilla.org/r/100144/#review100726

Comment on attachment 8820667 [details] Bug 1323100 - Remove nsThreadPoolNaming::SetThreadPoolName because it's now unused. https://reviewboard.mozilla.org/r/100142/#review100728

Comment on attachment 8820669 [details] Bug 1323100 - Remove NS_SetThreadName which is now unused. https://reviewboard.mozilla.org/r/100146/#review100730

Comment on attachment 8820670 [details] Bug 1323100 - Register named threads with the profiler. https://reviewboard.mozilla.org/r/100148/#review100732 ::: xpcom/threads/nsThread.cpp:461 (Diff revision 1) > SetupCurrentThreadForChaosMode(); > > if (initData->name.Length() > 0) { > PR_SetCurrentThreadName(initData->name.BeginReading()); > + > + profiler_register_thread(initData->name.BeginReading(), &stackTop); This is going to conflict with other threads that manually call `profiler_register_thread`, isn't it? I see at least the IPDL background thread calling this, and I see `MOZ_ASSERT`s in `mozilla_sampler_register_thread` that suggest calling `mozilla_sampler_register_thread` twice on the same thread will result in problems... ::: xpcom/threads/nsThread.cpp:527 (Diff revision 1) > mozilla::IOInterposer::UnregisterCurrentThread(); > > // Inform the threadmanager that this thread is going away > nsThreadManager::get().UnregisterCurrentThread(*self); > > + profiler_unregister_thread(); This is idempotent if we haven't registered the threads, correct? Reading `mozilla_sampler_register_thread` suggests that it is, but a confirmation of my reading would be nice.

Comment on attachment 8818167 [details] Bug 1323100 - Register most of the remaining threadfunc threads with the profiler. https://reviewboard.mozilla.org/r/98282/#review100734 Do we ever worry about multiple threads registering the same name with the profiler? Do we handle that case gracefully or even detect it? ::: dom/media/webaudio/blink/HRTFDatabaseLoader.cpp:155 (Diff revision 2) > + char stackTop; > + > PR_SetCurrentThreadName("HRTFDatabaseLdr"); > + profiler_register_thread("HRTFDatabaseLdr", &stackTop); You know, maybe we should just have a: ``` class AutoProfilerRegister final MOZ_STACK_CLASS { public: AutoProfilerRegister(const char* aName) { profiler_register_thread(aName, &mStackTop); } ~AutoProfilerRegister() { profiler_unregister_thread(); } private: const char* const mName; AutoProfilerRegister(const AutoProfilerRegister&) = delete; AutoProfilerRegister& operator=(const AutoProfilerRegister&) = delete; }; ``` or somesuch.

Thanks for the fast reviews!

Comment on attachment 8820661 [details] Bug 1323100 - Assign names to all remaining threads that are created through NS_NewThread and create them using NS_NewNamedThread instead. https://reviewboard.mozilla.org/r/100130/#review100710 > Do we need to name all the threads in tests, too? Is that necessary so we can assume that all `nsThread` threads have a name, and the profiler assumes that any thread it's profiler has a name, so we have to name threads in tests so the profiler doesn't blow up? Nothing really requires this, no. My goal here was to reduce the number of code search results you get when you search for NS_NewThread to something very close to zero, so that people will gravitate to using NS_NewNamedThread by default if they add new threads. After this patch, there are no users of NS_NewThread in mozilla-central. There are 3 users of NS_NewThread in comm-central. So if we fix those three, we could even delete NS_NewThread completely. How would you feel about that?

Comment on attachment 8820670 [details] Bug 1323100 - Register named threads with the profiler. https://reviewboard.mozilla.org/r/100148/#review100732 > This is going to conflict with other threads that manually call `profiler_register_thread`, isn't it? I see at least the IPDL background thread calling this, and I see `MOZ_ASSERT`s in `mozilla_sampler_register_thread` that suggest calling `mozilla_sampler_register_thread` twice on the same thread will result in problems... You're right. I had only tested these patches in an opt build, so when I pushed them to try I predictably hit this assertion all over the place. I'm going to add more patches to remove those existing profiler_register_thread calls for threads that are launched using NS_NewNamedThread. > This is idempotent if we haven't registered the threads, correct? Reading `mozilla_sampler_register_thread` suggests that it is, but a confirmation of my reading would be nice. I came to the same conclusion when I read the code.

Comment on attachment 8818167 [details] Bug 1323100 - Register most of the remaining threadfunc threads with the profiler. https://reviewboard.mozilla.org/r/98282/#review100734 > You know, maybe we should just have a: > > ``` > class AutoProfilerRegister final MOZ_STACK_CLASS > { > public: > AutoProfilerRegister(const char* aName) > { > profiler_register_thread(aName, &mStackTop); > } > ~AutoProfilerRegister() > { > profiler_unregister_thread(); > } > private: > const char* const mName; > AutoProfilerRegister(const AutoProfilerRegister&) = delete; > AutoProfilerRegister& operator=(const AutoProfilerRegister&) = delete; > }; > ``` > > or somesuch. You know, we absolutely should. Not sure why I hadn't thought of this. We don't even need a separate stackTop variable; the AutoProfilerRegister object itself will be the first thing on the stack, so we can even pass `this` as the stackTop.

Comment on attachment 8818167 [details] Bug 1323100 - Register most of the remaining threadfunc threads with the profiler. https://reviewboard.mozilla.org/r/98282/#review100734 We do not detect it but we do handle it gracefully. The profile will just contain multiple threads with the same name. The name is never used to index / key a thread, as far as I know.

Comment on attachment 8820951 [details] Bug 1323100 - Use AutoProfilerRegister to register chromium threads with the profiler. https://reviewboard.mozilla.org/r/100344/#review101316 ::: ipc/chromium/src/base/thread.cc:156 (Diff revision 1) > RefPtr<ThreadQuitTask> task = new ThreadQuitTask(); > message_loop_->PostTask(task.forget()); > } > > void Thread::ThreadMain() { > - char aLocal; > + mozilla::AutoProfilerRegister registerThread(name_.c_str()); Where is this defined? Did you slip it in to one of the earlier patches?

Comment on attachment 8820950 [details] Bug 1323100 - Stop double-registering the LazyIdleThread with the profiler. https://reviewboard.mozilla.org/r/100342/#review101318

Comment on attachment 8820949 [details] Bug 1323100 - Stop double-registering the IPDL Background thread with the profiler. https://reviewboard.mozilla.org/r/100340/#review101320

Comment on attachment 8820948 [details] Bug 1323100 - Stop double-registering the Media_Encoder thread with the profiler. https://reviewboard.mozilla.org/r/100338/#review101322

Comment on attachment 8820947 [details] Bug 1323100 - Stop double-registering the MediaStreamGraph thread with the profiler. https://reviewboard.mozilla.org/r/100336/#review101324

Comment on attachment 8820946 [details] Bug 1323100 - Stop double-registering the Socket Transport thread. https://reviewboard.mozilla.org/r/100334/#review101326

Comment on attachment 8820951 [details] Bug 1323100 - Use AutoProfilerRegister to register chromium threads with the profiler. https://reviewboard.mozilla.org/r/100344/#review101316 > Where is this defined? Did you slip it in to one of the earlier patches? I slipped it into the "Register most of the remaining threadfunc threads with the profiler" patch because that's the one where you made the suggestion to add AutoProfilerRegister.

Ah, OK. Can you file a followup bug to use AutoProfilerRegister more widely, too? (Assuming there are other places where we can use it, and you haven't slipped those changes into this patchset already. :)

Comment on attachment 8820951 [details] Bug 1323100 - Use AutoProfilerRegister to register chromium threads with the profiler. https://reviewboard.mozilla.org/r/100344/#review101316 > I slipped it into the "Register most of the remaining threadfunc threads with the profiler" patch because that's the one where you made the suggestion to add AutoProfilerRegister. I've filed bug 1326221 about using AutoProfilerRegister in more places.

Pushed by mstange@themasta.com: https://hg.mozilla.org/integration/autoland/rev/0c466e5e8933 Use NS_NewNamedThread to name the proxy resolution thread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/88d17bcd8205 Use NS_NewNamedThread for IndexedDB threads. r=froydnj https://hg.mozilla.org/integration/autoland/rev/54acf4ef8e84 Use NS_NewNamedThread for the Android Audio thread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/865d1b81c804 Use NS_NewNamedThread for the Link Monitor thread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/93419cb4f29f Use NS_NewNamedThread for the Wifi Monitor thread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/f4235b97257f Use NS_NewNamedThread for the Filewatcher IO thread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/1297ded6a01d Create a version of NS_NewNamedThread that accepts an nsACString. r=froydnj https://hg.mozilla.org/integration/autoland/rev/23d69df98a66 Use NS_NewNamedThread in SingletonThreadHolder. r=froydnj https://hg.mozilla.org/integration/autoland/rev/a3c9b45aa917 Use NS_NewNamedThread for CryptoTask threads. r=froydnj https://hg.mozilla.org/integration/autoland/rev/757539e7039a Assign names to all remaining threads that are created through NS_NewThread and create them using NS_NewNamedThread instead. r=froydnj https://hg.mozilla.org/integration/autoland/rev/9f554991549d Create nsThreadPoolNaming::GetNextThreadName. r=froydnj https://hg.mozilla.org/integration/autoland/rev/c0340dfe4766 Use nsThreadPoolNaming::GetNextThreadName and NS_NewNamedThread for the mozStorage thread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/3d53787293f6 Use nsThreadPoolNaming::GetNextThreadName and NS_NewNamedThread in the DecodePool. r=froydnj https://hg.mozilla.org/integration/autoland/rev/681cacbbaa3b Use nsThreadPoolNaming::GetNextThreadName and NS_NewNamedThread in nsThreadPool. r=froydnj https://hg.mozilla.org/integration/autoland/rev/a6d75c1cd724 Use nsThreadPoolNaming::GetNextThreadName for the DNS resolver thread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/940933b13b36 Remove nsThreadPoolNaming::SetThreadPoolName because it's now unused. r=froydnj https://hg.mozilla.org/integration/autoland/rev/06550f7eca62 Add nsThreadManager::NewNamedThread API. r=froydnj https://hg.mozilla.org/integration/autoland/rev/413dbd31725b Make NS_NewNamedThread use nsThreadManager::NewNamedThread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/061110f1ae12 Remove NS_SetThreadName which is now unused. r=froydnj https://hg.mozilla.org/integration/autoland/rev/1e3b3eddbe26 Register named threads with the profiler. r=froydnj https://hg.mozilla.org/integration/autoland/rev/c36524e4e919 Stop double-registering the Socket Transport thread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/12fb4c533659 Stop double-registering the MediaStreamGraph thread with the profiler. r=froydnj https://hg.mozilla.org/integration/autoland/rev/5572f3b63215 Stop double-registering the Media_Encoder thread with the profiler. r=froydnj https://hg.mozilla.org/integration/autoland/rev/b6953e3f5739 Stop double-registering the IPDL Background thread with the profiler. r=froydnj https://hg.mozilla.org/integration/autoland/rev/1b0855bb0c38 Stop double-registering the LazyIdleThread with the profiler. r=froydnj https://hg.mozilla.org/integration/autoland/rev/d6d25e8bd001 Register most of the remaining threadfunc threads with the profiler. r=froydnj https://hg.mozilla.org/integration/autoland/rev/84fb749698ab Use AutoProfilerRegister to register chromium threads with the profiler. r=froydnj

All backed out in https://hg.mozilla.org/integration/autoland/rev/dbe5b7ff43b6 for clipboard leak failures.

Pushed by mstange@themasta.com: https://hg.mozilla.org/integration/autoland/rev/5dbb824fd978 Use NS_NewNamedThread to name the proxy resolution thread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/6695c396ef15 Use NS_NewNamedThread for IndexedDB threads. r=froydnj https://hg.mozilla.org/integration/autoland/rev/061f8e2d0000 Use NS_NewNamedThread for the Android Audio thread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/e26f9d68b74c Use NS_NewNamedThread for the Link Monitor thread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/6675e4fd50f0 Use NS_NewNamedThread for the Wifi Monitor thread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/55b1ef1a16cf Use NS_NewNamedThread for the Filewatcher IO thread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/831f4de80f62 Create a version of NS_NewNamedThread that accepts an nsACString. r=froydnj https://hg.mozilla.org/integration/autoland/rev/c04743218bc5 Use NS_NewNamedThread in SingletonThreadHolder. r=froydnj https://hg.mozilla.org/integration/autoland/rev/a1c171f8f925 Use NS_NewNamedThread for CryptoTask threads. r=froydnj https://hg.mozilla.org/integration/autoland/rev/699f7f888ea3 Assign names to all remaining threads that are created through NS_NewThread and create them using NS_NewNamedThread instead. r=froydnj https://hg.mozilla.org/integration/autoland/rev/1fb590677ace Create nsThreadPoolNaming::GetNextThreadName. r=froydnj https://hg.mozilla.org/integration/autoland/rev/40249b284066 Use nsThreadPoolNaming::GetNextThreadName and NS_NewNamedThread for the mozStorage thread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/c5ca13e76e13 Use nsThreadPoolNaming::GetNextThreadName and NS_NewNamedThread in the DecodePool. r=froydnj https://hg.mozilla.org/integration/autoland/rev/4baecf3669b0 Use nsThreadPoolNaming::GetNextThreadName and NS_NewNamedThread in nsThreadPool. r=froydnj https://hg.mozilla.org/integration/autoland/rev/3a7ce80e8657 Use nsThreadPoolNaming::GetNextThreadName for the DNS resolver thread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/3a5216789011 Remove nsThreadPoolNaming::SetThreadPoolName because it's now unused. r=froydnj https://hg.mozilla.org/integration/autoland/rev/00453dfb7172 Add nsThreadManager::NewNamedThread API. r=froydnj https://hg.mozilla.org/integration/autoland/rev/cb8d544864b8 Make NS_NewNamedThread use nsThreadManager::NewNamedThread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/8c1328db2d0c Remove NS_SetThreadName which is now unused. r=froydnj https://hg.mozilla.org/integration/autoland/rev/7b54195f4383 Register named threads with the profiler. r=froydnj https://hg.mozilla.org/integration/autoland/rev/8ba2c7c2d262 Stop double-registering the Socket Transport thread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/4e441df8f70a Stop double-registering the MediaStreamGraph thread with the profiler. r=froydnj https://hg.mozilla.org/integration/autoland/rev/5daf48da151e Stop double-registering the Media_Encoder thread with the profiler. r=froydnj https://hg.mozilla.org/integration/autoland/rev/4fcf1517da8d Stop double-registering the IPDL Background thread with the profiler. r=froydnj https://hg.mozilla.org/integration/autoland/rev/ba3a6bce2ba5 Stop double-registering the LazyIdleThread with the profiler. r=froydnj https://hg.mozilla.org/integration/autoland/rev/3e64cdf12bb6 Register most of the remaining threadfunc threads with the profiler. r=froydnj https://hg.mozilla.org/integration/autoland/rev/069375097856 Use AutoProfilerRegister to register chromium threads with the profiler. r=froydnj

I had to back this all out again for leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=65567501&repo=autoland https://hg.mozilla.org/integration/autoland/rev/2c8782a9a73c

(In reply to Markus Stange [:mstange] from comment #0) > Registering a thread with the profiler using profiler_register_thread has > two advantages: > - We can profile them > - and TaskTracer knows the thread's name. > > My plan in this bug is to: > - Add profiler_register_thread calls in all the thread root functions that > have PR_SetCurrentThreadName but no profiler_register_thread call > - Change callers of NS_SetThreadName to use NS_NewNamedThread instead, and > remove NS_SetThreadName > - Make NS_SetThreadName automatically register the new thread with the > profiler. I really like the idea here. Not only fix the current situation, but make it impossible to add new things the wrong way. I hope we can sort out the leak regressions soon and get this landed.

I've disabled profiling for the CacheIO thread and will try to land these patches one more time.

Pushed by mstange@themasta.com: https://hg.mozilla.org/integration/autoland/rev/2535b71892be Use NS_NewNamedThread to name the proxy resolution thread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/01e1e73fa4b8 Use NS_NewNamedThread for IndexedDB threads. r=froydnj https://hg.mozilla.org/integration/autoland/rev/13ec6406c83d Use NS_NewNamedThread for the Android Audio thread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/b2b811183537 Use NS_NewNamedThread for the Link Monitor thread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/13ac84511de5 Use NS_NewNamedThread for the Wifi Monitor thread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/0abb589c0d44 Use NS_NewNamedThread for the Filewatcher IO thread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/fc5c8871c310 Create a version of NS_NewNamedThread that accepts an nsACString. r=froydnj https://hg.mozilla.org/integration/autoland/rev/2c3c3a0f9292 Use NS_NewNamedThread in SingletonThreadHolder. r=froydnj https://hg.mozilla.org/integration/autoland/rev/35295d2aeadf Use NS_NewNamedThread for CryptoTask threads. r=froydnj https://hg.mozilla.org/integration/autoland/rev/6e204f4e079b Assign names to all remaining threads that are created through NS_NewThread and create them using NS_NewNamedThread instead. r=froydnj https://hg.mozilla.org/integration/autoland/rev/28586a45a1e9 Create nsThreadPoolNaming::GetNextThreadName. r=froydnj https://hg.mozilla.org/integration/autoland/rev/f958fdc5af59 Use nsThreadPoolNaming::GetNextThreadName and NS_NewNamedThread for the mozStorage thread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/2fb2007cfc87 Use nsThreadPoolNaming::GetNextThreadName and NS_NewNamedThread in the DecodePool. r=froydnj https://hg.mozilla.org/integration/autoland/rev/770d1a1cbbb4 Use nsThreadPoolNaming::GetNextThreadName and NS_NewNamedThread in nsThreadPool. r=froydnj https://hg.mozilla.org/integration/autoland/rev/08afc56e7b3a Use nsThreadPoolNaming::GetNextThreadName for the DNS resolver thread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/1fdd373a8c77 Remove nsThreadPoolNaming::SetThreadPoolName because it's now unused. r=froydnj https://hg.mozilla.org/integration/autoland/rev/3d561ea06af2 Add nsThreadManager::NewNamedThread API. r=froydnj https://hg.mozilla.org/integration/autoland/rev/340b31b26270 Make NS_NewNamedThread use nsThreadManager::NewNamedThread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/1049cca342c8 Remove NS_SetThreadName which is now unused. r=froydnj https://hg.mozilla.org/integration/autoland/rev/bc1bf391dcb9 Register named threads with the profiler. r=froydnj https://hg.mozilla.org/integration/autoland/rev/a1c4f28d2a54 Stop double-registering the Socket Transport thread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/5d14f1e4b6fd Stop double-registering the MediaStreamGraph thread with the profiler. r=froydnj https://hg.mozilla.org/integration/autoland/rev/7947e6f167f2 Stop double-registering the Media_Encoder thread with the profiler. r=froydnj https://hg.mozilla.org/integration/autoland/rev/f7f7a07b088f Stop double-registering the IPDL Background thread with the profiler. r=froydnj https://hg.mozilla.org/integration/autoland/rev/8bcaf0985568 Stop double-registering the LazyIdleThread with the profiler. r=froydnj https://hg.mozilla.org/integration/autoland/rev/f9d6169e1bc6 Register most of the remaining threadfunc threads with the profiler. r=froydnj https://hg.mozilla.org/integration/autoland/rev/bd11222227ee Use AutoProfilerRegister to register chromium threads with the profiler. r=froydnj

https://hg.mozilla.org/mozilla-central/rev/2535b71892be https://hg.mozilla.org/mozilla-central/rev/01e1e73fa4b8 https://hg.mozilla.org/mozilla-central/rev/13ec6406c83d https://hg.mozilla.org/mozilla-central/rev/b2b811183537 https://hg.mozilla.org/mozilla-central/rev/13ac84511de5 https://hg.mozilla.org/mozilla-central/rev/0abb589c0d44 https://hg.mozilla.org/mozilla-central/rev/fc5c8871c310 https://hg.mozilla.org/mozilla-central/rev/2c3c3a0f9292 https://hg.mozilla.org/mozilla-central/rev/35295d2aeadf https://hg.mozilla.org/mozilla-central/rev/6e204f4e079b https://hg.mozilla.org/mozilla-central/rev/28586a45a1e9 https://hg.mozilla.org/mozilla-central/rev/f958fdc5af59 https://hg.mozilla.org/mozilla-central/rev/2fb2007cfc87 https://hg.mozilla.org/mozilla-central/rev/770d1a1cbbb4 https://hg.mozilla.org/mozilla-central/rev/08afc56e7b3a https://hg.mozilla.org/mozilla-central/rev/1fdd373a8c77 https://hg.mozilla.org/mozilla-central/rev/3d561ea06af2 https://hg.mozilla.org/mozilla-central/rev/340b31b26270 https://hg.mozilla.org/mozilla-central/rev/1049cca342c8 https://hg.mozilla.org/mozilla-central/rev/bc1bf391dcb9 https://hg.mozilla.org/mozilla-central/rev/a1c4f28d2a54 https://hg.mozilla.org/mozilla-central/rev/5d14f1e4b6fd https://hg.mozilla.org/mozilla-central/rev/7947e6f167f2 https://hg.mozilla.org/mozilla-central/rev/f7f7a07b088f https://hg.mozilla.org/mozilla-central/rev/8bcaf0985568 https://hg.mozilla.org/mozilla-central/rev/f9d6169e1bc6 https://hg.mozilla.org/mozilla-central/rev/bd11222227ee