1476793 - Idle stacks in the JS Helper thread are not marked as idle

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement, P1)

Description

[Markus Stange [:mstange]]

Bug 1405374 has added the ability for JS Helper threads to be profiled by the Gecko Profiler. However, the JS Helper threads currently cannot push label frames to the ProfilingStack, so they also cannot mark samples as belonging to the IDLE category. Once we have a thread activity graph in perf.html, this would cause the thread to look permanently active even when it's idle.

The JS engine can't use tools/profiler/public/GeckoProfiler.h, but it *can* use the ProfilingStack (which is defined in js/public/ProfilingStack.h) - it just needs to get a pointer to the current thread's ProfilingStack. And with a bit more code duplication we can also give it RAII- and macro-based profiler label pushing.

Comment 1

[Markus Stange [:mstange]]

Attachment: Bug 1476793 - Teach HelperThread how to push/pop profiler label frames, and use this capability to push an IDLE frame when the thread is idle.

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

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 8993141 [details]
Bug 1476793 - Teach HelperThread how to push/pop profiler label frames, and use this capability to push an IDLE frame when the thread is idle.

https://reviewboard.mozilla.org/r/257964/#review264860

::: js/src/vm/HelperThreads.cpp:50
(Diff revision 1)
>  GlobalHelperThreadState* gHelperThreadState = nullptr;
>  
>  } // namespace js
>  
> +
> +// Macros used by the AUTO_PROFILER_* macros below.

This comment was copied from GeckoProfiler.h, but isn't necessary here.

However, it would be good to have a comment explaining that these macros are identical in function to the same-named ones in GeckoProfiler.h, but they are defined separately because SpiderMonkey can't use GeckoProfiler.h.

Comment 3

[Markus Stange [:mstange]]

Comment on attachment 8993141 [details]
Bug 1476793 - Teach HelperThread how to push/pop profiler label frames, and use this capability to push an IDLE frame when the thread is idle.

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

Comment 4

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8993141 [details]
Bug 1476793 - Teach HelperThread how to push/pop profiler label frames, and use this capability to push an IDLE frame when the thread is idle.

https://reviewboard.mozilla.org/r/257964/#review265278

Excellent, thank you for doing the work here!

::: js/src/vm/HelperThreads.cpp:49
(Diff revision 2)
> +// These macros are identical in function to the same-named ones in
> +// GeckoProfiler.h, but they are defined separately because SpiderMonkey can't
> +// use GeckoProfiler.h.
> +#define PROFILER_RAII_PASTE(id, line) id ## line
> +#define PROFILER_RAII_EXPAND(id, line) PROFILER_RAII_PASTE(id, line)
> +#define PROFILER_RAII PROFILER_RAII_EXPAND(raiiObject, __LINE__)
> +#define AUTO_PROFILER_LABEL(label, category) \
> +  HelperThread::AutoProfilerLabel PROFILER_RAII(this, label, __LINE__, \
> +                                                js::ProfilingStackFrame::Category::category)
> +

I will be happy when most of this stuff gets moved to mozglue...

::: js/src/vm/HelperThreads.cpp:1879
(Diff revision 2)
>  
>      JS::RegisterThreadCallback callback = HelperThreadState().registerThread;
>      if (callback) {
> +        profilingStack =
> -        callback("JS Helper", reinterpret_cast<void*>(GetNativeStackBase()));
> +            callback("JS Helper", reinterpret_cast<void*>(GetNativeStackBase()));
>          registered = true;

Up to you -- do you prefer keeping the separate registered boolean, or just using profilingStack now?

::: js/src/vm/HelperThreads.cpp:2380
(Diff revision 2)
> +    if (profilingStack) {
> +        profilingStack->pushLabelFrame(label, nullptr, this, line, category);
> +    }

Until we finally get around to consolidating coding styles, this shouldn't have braces.

::: js/src/vm/HelperThreads.cpp:2387
(Diff revision 2)
> +    if (profilingStack) {
> +        profilingStack->pop();
> +    }

nor this

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 8993141 [details]
Bug 1476793 - Teach HelperThread how to push/pop profiler label frames, and use this capability to push an IDLE frame when the thread is idle.

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

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8993141 [details]
Bug 1476793 - Teach HelperThread how to push/pop profiler label frames, and use this capability to push an IDLE frame when the thread is idle.

https://reviewboard.mozilla.org/r/257964/#review265278

> Up to you -- do you prefer keeping the separate registered boolean, or just using profilingStack now?

I removed the separate boolean.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ada1690b63e9
Teach HelperThread how to push/pop profiler label frames, and use this capability to push an IDLE frame when the thread is idle. r=njn,sfink

Comment 8

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/ada1690b63e9