Closed Bug 1431184 Opened 6 years ago Closed 6 years ago

Don't register JS worker threads multiple times

Categories

(Core :: Gecko Profiler, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
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.
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.
>  - 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 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.
Attachment #8943381 - Flags: review?(nfroyd) → review+
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.
Pushed by mstange@themasta.com:
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
https://hg.mozilla.org/mozilla-central/rev/296ef6baf20b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1434965
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: