1405374 - Register the JS HelperThread with the profiler

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Markus Stange [:mstange]]

The JS engine launches some threads for its own use. These threads currently are not registered with the Gecko Profiler.

The necessary profiler functions are not exposed to the JS engine (you can't include tools/profiler/public/GeckoProfiler.h from Spidermonkey).

Comment 1

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

Attachment: Register JS threads with the profiler

This is a little convoluted, but it seems to serve its purpose.

I initially was trying to label the purpose of each thread better (they'd still all be "JS Helper", but I'd push a pseudo frame every time they grabbed a task), but that ended up convoluted and mostly redundant with the call stack anyway.

I'm using unprotected global variables to store the callbacks. Perhaps I should have them in UnprotectedData<> or something? Not sure about the idioms there. (But they're only written on the main thread during xpconnect startup.)

Comment 2

[Jon Coppeard (:jonco)]

Comment on attachment 8984319 [details] [diff] [review]
Register JS threads with the profiler

Review of attachment 8984319 [details] [diff] [review]:
-----------------------------------------------------------------

I guess we register the threads when they first run since they are created before the callbacks are setup.  Seems fair enough.

::: js/public/ProfilingStack.h
@@ +310,5 @@
>  JS_FRIEND_API(void)
>  EnableContextProfilingStack(JSContext* cx, bool enabled);
>  
>  JS_FRIEND_API(void)
> +SetProfilingRegisterThreadCallback(void (*fn)(const char*, void*));

For consistency with out other APIs there should probably be an explicit type definition for the function pointer types.

::: js/src/vm/HelperThreads.h
@@ +386,5 @@
> +     * GeckoProfilerRuntime because helper threads don't always have access to
> +     * the runtime.
> +     */
> +    static void (*gRegisterThread)(const char*, void*);
> +    static void (*gUnregisterThread)();

Can these go in the GlobalHelperThreadState?  Maybe they could be WriteOnceData if they're only written in startup.

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] (PTO June 31) from comment #1)
Also, thanks for doing this.  I'm really interested to start using the profiler on helper threads.

Comment 4

[Markus Stange [:mstange]]

Comment on attachment 8984319 [details] [diff] [review]
Register JS threads with the profiler

Review of attachment 8984319 [details] [diff] [review]:
-----------------------------------------------------------------

The profiler-side changes look good.

Comment 5

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

Attachment: Register JS threads with the profiler,

Applied review feedback, patch is now much, much simpler. The concerns that led to me requesting mstange's review no longer apply, so I'm going to just land with jonco's review.

I wish I hadn't complicated this in the first place, it would have taken me a lot less time!

Comment 6

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

(In reply to Jon Coppeard (:jonco) from comment #2)
> Comment on attachment 8984319 [details] [diff] [review]
> Register JS threads with the profiler
> 
> Review of attachment 8984319 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I guess we register the threads when they first run since they are created
> before the callbacks are setup.  Seems fair enough.

Honestly? It's because initially I was doing things that required JSRuntime::geckoProfiler(), but nothing has the runtime during thread creation so I postponed until there *was* a runtime. When I stopped using geckoProfiler() for anything, I neglected to follow through on the simplifications possible. Trying that now...

> ::: js/public/ProfilingStack.h
> @@ +310,5 @@
> >  JS_FRIEND_API(void)
> >  EnableContextProfilingStack(JSContext* cx, bool enabled);
> >  
> >  JS_FRIEND_API(void)
> > +SetProfilingRegisterThreadCallback(void (*fn)(const char*, void*));
> 
> For consistency with out other APIs there should probably be an explicit
> type definition for the function pointer types.

You're right. And there's no real reason to have two separate callback registration functions, so I combined them in one.

> ::: js/src/vm/HelperThreads.h
> @@ +386,5 @@
> > +     * GeckoProfilerRuntime because helper threads don't always have access to
> > +     * the runtime.
> > +     */
> > +    static void (*gRegisterThread)(const char*, void*);
> > +    static void (*gUnregisterThread)();
> 
> Can these go in the GlobalHelperThreadState?  Maybe they could be
> WriteOnceData if they're only written in startup.

That seems much better, thanks.

Comment 7

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

(In reply to Steve Fink [:sfink] [:s:] (PTO June 31) from comment #5)
> Applied review feedback, patch is now much, much simpler. The concerns that
> led to me requesting mstange's review no longer apply, so I'm going to just
> land with jonco's review.

Oops. Mid-air collision; had r+ from mstange already anyway. Thanks!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aacc6666a49a
Register JS threads with the profiler, r=jonco

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

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