1431184 - Don't register JS worker threads multiple times

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement)

Description

[Markus Stange [:mstange]]

Our Web Worker code uses a thread pool where a single OS thread can be reused for different worker scripts during its lifetime. We only register these threads with the profiler for the duration that they're running a worker script. So the same OS thread can be registered with the profiler during multiple disjoint time ranges, and we expect the profiler to treat those different registrations as different conceptual threads.

This has multiple advantages:
 - The "thread name" of the conceptual thread can include the script URL:
   "DOM Worker <scriptURL>". This lets you create thread filter which match a
   part of the URL, so you have the option of profiling just the worker threads
   you're interested in.
 - We don't waste time sampling a worker thread while it's idle and has no script.

But it also has disadvantages:
 - The profiler platform doesn't actually know how to deal with different
   "conceptual threads" that share the same OS thread. This leads to surprising
   breakage in different places. For example, the contents in the profiler buffer
   are marked with ThreadId entries which use the OS thread id.
 - What we show in the profiler UI does not match reality, and might be confusing.

I don't think the advantages are large enough to warrant teaching the rest of the profiler platform to deal with conceptual threads. So I think we should just stop doing the special thing and just register the OS threads for their entire duration.

We can add pseudo stack entries that have the script URL.

Comment 1

[Markus Stange [:mstange]]

Attachment: Bug 1431184 - Register DOM Worker threads with the profiler for their entire lifetime, not just for the ranges during which they're running a worker script.

Review commit: https://reviewboard.mozilla.org/r/213710/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/213710/

Comment 2

[Markus Stange [:mstange]]

Comment on attachment 8943381 [details]
Bug 1431184 - Register DOM Worker threads with the profiler for their entire lifetime, not just for the ranges during which they're running a worker script.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213710/diff/1-2/

Comment 3

[Markus Stange [:mstange]]

Calling nsThread::Init with a string (instead of null) causes that nsThread to register itself with the profiler automatically. It also calls NS_SetCurrentThreadName automatically.

Comment 4

[Greg Tatum [:gregtatum] (REO Fx132)]

Also see https://github.com/devtools-html/perf.html/issues/540 for requests in the client to name workers. I'm not sure if there are opportunities to supplement the pseudostacks.

Comment 5

[Julien Wajsberg [:julienw]]

>  - What we show in the profiler UI does not match reality, and might be confusing.

Moreover if we have labels/markers/pseudostacks to identify the workers in the course of a thread, we could have an UX to expose them, either with the existing UX or otherwise.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8943381 [details]
Bug 1431184 - Register DOM Worker threads with the profiler for their entire lifetime, not just for the ranges during which they're running a worker script.

https://reviewboard.mozilla.org/r/213710/#review220018

I agree this scheme makes more sense, and we should be able to sort out the information about who is running what script someplace else.

::: commit-message-1d88c:1
(Diff revision 2)
> +Bug 1431184 - Register DOM Worker threads with the profiler for their entire lifetime, not just for the ranges during which they're running a worker script. r?froydnj
> +

I think it would not be a bad idea to put your explanation from comment 0 in as the "ong form" commit message here.  But I'll leave that up to you.

::: dom/workers/RuntimeService.cpp
(Diff revision 2)
> -  nsAutoCString threadName;
> +  using mozilla::ipc::BackgroundChild;
> -  threadName.AssignLiteral("DOM Worker '");
> -  threadName.Append(NS_LossyConvertUTF16toASCII(mWorkerPrivate->ScriptURL()));
> -  threadName.Append('\'');
> -
> -  AUTO_PROFILER_REGISTER_THREAD(threadName.get());

Oh man, so this would register and deregister threads constantly?  That seems...not good.

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8943381 [details]
Bug 1431184 - Register DOM Worker threads with the profiler for their entire lifetime, not just for the ranges during which they're running a worker script.

https://reviewboard.mozilla.org/r/213710/#review220018

> I think it would not be a bad idea to put your explanation from comment 0 in as the "ong form" commit message here.  But I'll leave that up to you.

Will do.

> Oh man, so this would register and deregister threads constantly?  That seems...not good.

Not constantly, only when the script changes. The primary thread runnable stays on the stack for that duration and spins a nested event loop.

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8943381 [details]
Bug 1431184 - Register DOM Worker threads with the profiler for their entire lifetime, not just for the ranges during which they're running a worker script.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213710/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/296ef6baf20b
Register DOM Worker threads with the profiler for their entire lifetime, not just for the ranges during which they're running a worker script. r=froydnj

Comment 10

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/296ef6baf20b