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.

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

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

(I think this was bhackett's idea.)

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.

\o/

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().

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 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.

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

Followup to unbreak non-threadsafe builds: https://hg.mozilla.org/integration/mozilla-inbound/rev/45840c6628d7

(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.

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

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

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

Backed out for causing bug 941805. https://hg.mozilla.org/integration/mozilla-inbound/rev/80286bae0d73

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

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

(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.

(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!

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

Part 2, removing useHelperThreads and letting DOM workers (and any other runtimes) use helper threads: https://hg.mozilla.org/integration/mozilla-inbound/rev/37a64fc4edb3