530634 - Workers: Number of backing OS threads should be override-able by a user pref

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, enhancement)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Anyone object to "dom.workers.numOSThreads"?

Comment 1

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

We need two numbers, a max and a cap.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Based on IRC discussion, we're going to add

  dom.workers.maxRunningThreads --- the maximum number of OS threads we'll allow to run concurrently, which ideally would be approximately the number of HW threads available on a given platform

  dom.workers.maxTotalThreads --- the maximum number of OS threads that are allowed to be allocated, across all workers.

"maxTotalThreads" will only be important for followup work ... in the current implementation, "maxRunningThreads" ~= "maxTotalThreads".

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: v0

Let the r-'ing begin!

Comment 4

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

Comment on attachment 414147 [details] [diff] [review]
v0

In general, make sure you're doing 2 space indents and using braces on single-line if/while statements.

>+#define THREADPOOL_THREAD_CAP 128

This is far too many threads for normal use. Thankfully I don't think it matters because I don't believe we should have an upper limit here. Let's have a sensible default in prefs (like 20 or so) and then respect whatever the user sets (making sure that it is >= the maxRunning number).

>+static const char* kPrefMaxScriptRunTime = "dom.max_script_run_time";

Please make these |static const char kPref[]'.

>-  RegisterPrefCallbacks();
>-
>   mThreadPool = do_CreateInstance(NS_THREADPOOL_CONTRACTID, &rv);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  RegisterPrefCallbacks();

Creating a thread pool shouldn't cause any pref callbacks, but even if it did I don't think we should support callbacks with a half-initialized thread service.

> nsDOMThreadService::GetOrInitService()

I don't like these changes... I don't think we should ever "leak" a half-initialized thread service by setting that pointer before Init has succeeded. I'm assuming you did that for threadsafety, but I think a main-thread assertion after the !gDOMThreadService check is sufficient. Nothing should call GetOrInit the first time off the main thread, and if something does then we need to fix it.

>+  else if (!strcmp(aPrefName, kPrefMaxRunningThreads) ||
>+           !strcmp(aPrefName, kPrefMaxTotalThreads)) {
>+    nsRefPtr<nsDOMThreadService> threadService =
>+      nsDOMThreadService::GetOrInitService();
>+    NS_ENSURE_STATE(threadService);

Just check for gDOMThreadService here. This should only happen on the main thread so it will be safe.

>+nsDOMThreadService::UpdateThreadAllocationLimits()

This won't work because of suspended workers. They bump the number of allowed threads and you can't change this out from under them.

Comment 5

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

Comment on attachment 414147 [details] [diff] [review]
v0

>+nsDOMThreadService::UpdateThreadAllocationLimits()

Also, I'm pretty sure that you need to be in the monitor before modifying the thread pool's limits.

Comment 6

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

Bug 649537 fixed this by allowing you to control the maximum number of OS threads per domain via preferences.