Closed Bug 1323100 Opened 8 years ago Closed 7 years ago

Register the remaining threads with the profiler / TaskTracer

Categories

(Core :: Gecko Profiler, defect)

Product:
Core
Component:
Gecko Profiler
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

(Blocks 2 open bugs)

Details

Attachments

(27 files, 2 obsolete files)

Bug 1323100 - Register most of the remaining threadfunc threads with the profiler.
58 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Use NS_NewNamedThread in SingletonThreadHolder.
58 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Add nsThreadManager::NewNamedThread API.
58 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Use NS_NewNamedThread to name the proxy resolution thread.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Use NS_NewNamedThread for IndexedDB threads.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Use NS_NewNamedThread for the Android Audio thread.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Use NS_NewNamedThread for the Link Monitor thread.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Use NS_NewNamedThread for the Wifi Monitor thread.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Use NS_NewNamedThread for the Filewatcher IO thread.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Create a version of NS_NewNamedThread that accepts an nsACString.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Use NS_NewNamedThread for CryptoTask threads.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Assign names to all remaining threads that are created through NS_NewThread and create them using NS_NewNamedThread instead.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Create nsThreadPoolNaming::GetNextThreadName.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Use nsThreadPoolNaming::GetNextThreadName and NS_NewNamedThread for the mozStorage thread.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Use nsThreadPoolNaming::GetNextThreadName and NS_NewNamedThread in the DecodePool.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Use nsThreadPoolNaming::GetNextThreadName and NS_NewNamedThread in nsThreadPool.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Use nsThreadPoolNaming::GetNextThreadName for the DNS resolver thread.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Remove nsThreadPoolNaming::SetThreadPoolName because it's now unused.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Make NS_NewNamedThread use nsThreadManager::NewNamedThread.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Remove NS_SetThreadName which is now unused.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Register named threads with the profiler.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Stop double-registering the Socket Transport thread.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Stop double-registering the MediaStreamGraph thread with the profiler.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Stop double-registering the Media_Encoder thread with the profiler.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Stop double-registering the IPDL Background thread with the profiler.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Stop double-registering the LazyIdleThread with the profiler.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1323100 - Use AutoProfilerRegister to register chromium threads with the profiler.
59 bytes, text/x-review-board-request
froydnj
: review+
Details 
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.
Attachment #8818169 - Attachment is obsolete: true
Attachment #8818171 - Attachment is obsolete: true
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
Attachment #8820653 - Flags: review?(nfroyd) → review+
Comment on attachment 8820655 [details]
Bug 1323100 - Use NS_NewNamedThread for the Android Audio thread.

https://reviewboard.mozilla.org/r/100118/#review100628
Attachment #8820655 - Flags: review?(nfroyd) → review+
Comment on attachment 8820656 [details]
Bug 1323100 - Use NS_NewNamedThread for the Link Monitor thread.

https://reviewboard.mozilla.org/r/100120/#review100630
Attachment #8820656 - Flags: review?(nfroyd) → review+
Comment on attachment 8820657 [details]
Bug 1323100 - Use NS_NewNamedThread for the Wifi Monitor thread.

https://reviewboard.mozilla.org/r/100122/#review100636
Attachment #8820657 - Flags: review?(nfroyd) → review+
Comment on attachment 8820658 [details]
Bug 1323100 - Use NS_NewNamedThread for the Filewatcher IO thread.

https://reviewboard.mozilla.org/r/100124/#review100638
Attachment #8820658 - Flags: review?(nfroyd) → review+
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.
Attachment #8820659 - Flags: review?(nfroyd) → review+
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.
Attachment #8820662 - Flags: review?(nfroyd) → review+
Comment on attachment 8820663 [details]
Bug 1323100 - Use nsThreadPoolNaming::GetNextThreadName and NS_NewNamedThread for the mozStorage thread.

https://reviewboard.mozilla.org/r/100134/#review100690
Attachment #8820663 - Flags: review?(nfroyd) → review+
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`...?
Attachment #8820664 - Flags: review?(nfroyd) → review+
(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
Attachment #8820665 - Flags: review?(nfroyd) → review+
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
Attachment #8820666 - Flags: review?(nfroyd) → review+
(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
Attachment #8818168 - Flags: review?(nfroyd) → review+
Comment on attachment 8820660 [details]
Bug 1323100 - Use NS_NewNamedThread for CryptoTask threads.

https://reviewboard.mozilla.org/r/100128/#review100706
Attachment #8820660 - Flags: review?(nfroyd) → review+
Comment on attachment 8820654 [details]
Bug 1323100 - Use NS_NewNamedThread for IndexedDB threads.

https://reviewboard.mozilla.org/r/100116/#review100708
Attachment #8820654 - Flags: review?(nfroyd) → review+
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?
Attachment #8820661 - Flags: review?(nfroyd) → review+
Comment on attachment 8818170 [details]
Bug 1323100 - Add nsThreadManager::NewNamedThread API.

https://reviewboard.mozilla.org/r/98288/#review100716
Attachment #8818170 - Flags: review?(nfroyd) → review+
Comment on attachment 8820668 [details]
Bug 1323100 - Make NS_NewNamedThread use nsThreadManager::NewNamedThread.

https://reviewboard.mozilla.org/r/100144/#review100726
Attachment #8820668 - Flags: review?(nfroyd) → review+
Comment on attachment 8820667 [details]
Bug 1323100 - Remove nsThreadPoolNaming::SetThreadPoolName because it's now unused.

https://reviewboard.mozilla.org/r/100142/#review100728
Attachment #8820667 - Flags: review?(nfroyd) → review+
Comment on attachment 8820669 [details]
Bug 1323100 - Remove NS_SetThreadName which is now unused.

https://reviewboard.mozilla.org/r/100146/#review100730
Attachment #8820669 - Flags: review?(nfroyd) → review+
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.
Attachment #8820670 - Flags: review?(nfroyd) → review+
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.
Attachment #8818167 - Flags: review?(nfroyd) → review+
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?
Attachment #8820951 - Flags: review?(nfroyd) → review+
Comment on attachment 8820950 [details]
Bug 1323100 - Stop double-registering the LazyIdleThread with the profiler.

https://reviewboard.mozilla.org/r/100342/#review101318
Attachment #8820950 - Flags: review?(nfroyd) → review+
Comment on attachment 8820949 [details]
Bug 1323100 - Stop double-registering the IPDL Background thread with the profiler.

https://reviewboard.mozilla.org/r/100340/#review101320
Attachment #8820949 - Flags: review?(nfroyd) → review+
Comment on attachment 8820948 [details]
Bug 1323100 - Stop double-registering the Media_Encoder thread with the profiler.

https://reviewboard.mozilla.org/r/100338/#review101322
Attachment #8820948 - Flags: review?(nfroyd) → review+
Comment on attachment 8820947 [details]
Bug 1323100 - Stop double-registering the MediaStreamGraph thread with the profiler.

https://reviewboard.mozilla.org/r/100336/#review101324
Attachment #8820947 - Flags: review?(nfroyd) → review+
Comment on attachment 8820946 [details]
Bug 1323100 - Stop double-registering the Socket Transport thread.

https://reviewboard.mozilla.org/r/100334/#review101326
Attachment #8820946 - Flags: review?(nfroyd) → review+
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
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
(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.
Flags: needinfo?(mstange)
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1329466
Blocks: 1337558

(In reply to Markus Stange [:mstange] from comment #130)

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

I stumbled on this from https://searchfox.org/mozilla-central/rev/4bb2401ecbfce89af06fb2b4d0ea3557682bd8ff/netwerk/cache2/CacheIOThread.cpp#410-411 that references this bug. Do you remember why this caused leaks? Is there a follow-up bug on file? (I couldn't find one)

Flags: needinfo?(mstange)

I didn't remember this, but reading the bug, it looks like I didn't know why this caused leaks and just wanted to land it. It doesn't look like I filed a follow-up bug.

Flags: needinfo?(mstange)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: