Closed Bug 1405374 Opened 4 years ago Closed 3 years ago

Register the JS HelperThread with the profiler

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: mstange, Assigned: sfink)

References

(Blocks 1 open bug)

Details

(Whiteboard: [js:tech-debt][js:testability])

Attachments

(1 file, 1 obsolete file)

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).
Priority: -- → P3
Whiteboard: [js:tech-debt][js:testability]
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.)
Attachment #8984319 - Flags: review?(mstange)
Attachment #8984319 - Flags: review?(jcoppeard)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
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.
Attachment #8984319 - Flags: review?(jcoppeard) → review+
(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 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.
Attachment #8984319 - Flags: review?(mstange) → review+
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!
Attachment #8984319 - Attachment is obsolete: true
(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.
Attachment #8984619 - Flags: review+
(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!
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aacc6666a49a
Register JS threads with the profiler, r=jonco
https://hg.mozilla.org/mozilla-central/rev/aacc6666a49a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Depends on: 1468928
Blocks: 1476793
Regressions: 1607008
You need to log in before you can comment on or make changes to this bug.