941805 - Make the pool of JS workers be per-process rather than per-runtime

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew McCreight [:mccr8]]

This would allow DOM workers to use JS workers in their own runtime, which is increasingly important to ensure that JS runs as effectively as possible, without hugely increasing the total number of threads.  bhackett said this would require moving the JS runtime pointer from the worker itself to the work task thing.

Comment 1

[Jan de Mooij [:jandem]]

This would be really nice. Bug 927516 helps but using helper threads would make workers behave a lot more like the main thread.

Comment 2

[Jason Orendorff [:jorendorff]]

Yep, this would be nice. From an API perspective, would this mean using global state in any JSAPI-user-visible way?

Comment 3

[Andrew McCreight [:mccr8]]

(I think this was bhackett's idea.)

Comment 4

[Brian Hackett [Laid off!]]

Attachment: patch.txt

This patch moves WorkerThreadState from JSRuntime into a single process wide global, gWorkerThreadState.  A fair number of minor related changes and cleanup to the worker code are involved, mainly to beef up thread safety checking around uses of WorkerThreadState members.  This change is invisible to JSAPI users.

This doesn't actually change DOM workers to use the worker thread state, though.  That can be done in followup, along with some associated cleanup like removing the isWorkerRuntime stuff.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

\o/

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8364741 [details] [diff] [review]
patch.txt

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

Looks good. But please do make sure to change gWorkerThreadState to a function call. That way if we want to add assertions to it or something, it will be easier.

::: js/src/builtin/TestingFunctions.cpp
@@ +1405,5 @@
>  static bool
>  WorkerThreadCount(JSContext *cx, unsigned argc, jsval *vp)
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
> +    args.rval().setInt32(cx->runtime()->useHelperThreads() ? gWorkerThreadState.threadCount : 0);

Won't useHelperThreads() be obsolete with this patch? Why do we still need this test?

::: js/src/jit/Ion.cpp
@@ +1604,5 @@
>      // on the main thread in some cases. Do not compile off thread during an
>      // incremental GC, as this may trip incremental read barriers.
>      //
> +    // Require cpuCount > 1 so that Ion compilation jobs and main-thread
> +    // execution are not competing for the same resources.

Is this part of the refactoring, or something new?

::: js/src/jscntxt.cpp
@@ +1050,5 @@
>    : ContextFriendFields(rt),
>      contextKind_(kind),
>      perThreadData(pt),
>      allocator_(nullptr)
> +{}

These should go on separate lines.

::: js/src/jsworkers.cpp
@@ +102,5 @@
>   */
>  static void
>  FinishOffThreadIonCompile(jit::IonBuilder *builder)
>  {
> +    gWorkerThreadState.ionFinishedList().append(builder);

I don't understand why we still can't have a per-compartment finished list. It seems more efficient and simpler that way. What are we winning by doing it this way? Uniformity of the data structures?

@@ +418,3 @@
>          return true;
>  
> +    threads = (WorkerThread*) js_pod_calloc<WorkerThread>(threadCount);

Space before the *. Also, I don't think you need the cast.

::: js/src/jsworkers.h
@@ +54,3 @@
>  
> +  private:
> +    // The below lists are all protected by |lock|.

Maybe "The lists below...".

@@ +72,1 @@
>      mozilla::Atomic<uint32_t> asmJSCompilationInProgress;

It seems like it would be a nice cleanup to make ParallelCompilationGuard be a member of WorkerThreadState. Then this field could be private and we could do some friending.

@@ +209,5 @@
>       */
>      void *asmJSFailedFunction;
>  };
>  
> +extern WorkerThreadState gWorkerThreadState;

I think it would be better to access this through a function call, even if it's just an inline function that returns this variable.

::: js/src/vm/Runtime.h
@@ +629,5 @@
> +            pt->runtime_ = rt;
> +        }
> +
> +        ~AutoEnterRuntime()
> +        {

Brace should be on the same line as ~AutoEnterRuntime().

Comment 7

[Brian Hackett [Laid off!]]

useHelperThreads() and isWorkerRuntime() are both things that I'll remove in a patch shortly.  The Ion finished list was moved to the worker thread state to, yeah, be consistent with the other worklists / finished lists.

https://hg.mozilla.org/integration/mozilla-inbound/rev/acadb164dbc5

Comment 8

[Brian Hackett [Laid off!]]

Attachment: remove useHelperThreads, isWorkerRuntime

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8369006 [details] [diff] [review]
remove useHelperThreads, isWorkerRuntime

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

Doesn't this mean we'll allocate a GC thread for each DOM worker? That seems like a step backwards.

Comment 10

[Brian Hackett [Laid off!]]

Well, it would be nicer if there was only a single GC background sweeping thread, or background sweeping was done using JS helper threads.

Comment 11

[Brian Hackett [Laid off!]]

Followup to unbreak non-threadsafe builds:

https://hg.mozilla.org/integration/mozilla-inbound/rev/45840c6628d7

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Brian Hackett (:bhackett) (Vacation until early February) from comment #10)
> Well, it would be nicer if there was only a single GC background sweeping
> thread, or background sweeping was done using JS helper threads.

Until we do one of those, I think we should hold off on this patch. Sharing the GC thread seems a little easier, although it would be nice to unify everything.

Comment 13

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/?showall=1&tree=Mozilla-Inbound&rev=acadb164dbc5&jobname=warnaserr - this made the shell warnings-as-errors builds pretty failure-prone.

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/acadb164dbc5
https://hg.mozilla.org/mozilla-central/rev/45840c6628d7

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Wait, this wasn't supposed to land yet, was it?

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing bug 941805.

https://hg.mozilla.org/integration/mozilla-inbound/rev/80286bae0d73

Comment 17

[Ed Morley [:emorley]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #16)
> Backed out for causing bug 941805.

Bug 966782

Comment 18

[Ed Morley [:emorley]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/80286bae0d73

Comment 19

[Brian Hackett [Laid off!]]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #15)
> Wait, this wasn't supposed to land yet, was it?

This attempted landing was only the first part, which doesn't yet change DOM workers to use the thread pool.

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Brian Hackett (:bhackett) from comment #19)
> This attempted landing was only the first part, which doesn't yet change DOM
> workers to use the thread pool.

Ok, thanks!

Comment 21

[Brian Hackett [Laid off!]]

Reland the first part:

https://hg.mozilla.org/integration/mozilla-inbound/rev/995f7402235b

The problem with the earlier patch seems to be that we never explicitly rejoined the spawned threads, and when they were joined or whatever at process shutdown NSPR had already been shut down and we sometimes got confused.

https://tbpl.mozilla.org/?tree=Try&rev=f32f5fd92b70

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/995f7402235b

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6c899a1064f3

Comment 24

[Brian Hackett [Laid off!]]

Part 2, removing useHelperThreads and letting DOM workers (and any other runtimes) use helper threads:

https://hg.mozilla.org/integration/mozilla-inbound/rev/37a64fc4edb3

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/37a64fc4edb3