818247 - Rivertrail creates a threadpool for every runtime regardless of JS_NO_HELPER_THREADS setting

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Rivertrail creates a threadpool for every runtime regardless of JS_NO_HELPER_THREADS setting. This is especially bad for DOM workers (they end up getting (#cores)+1 threads). We need a way to prevent this for workers.

Comment 1

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

A good heuristic is to create threads only if RiverTrail is actually used. We also have rt->useHelperThreads(), which is false on DOM workers and true for the main JS runtime.

Comment 2

[Niko Matsakis [:nmatsakis]]

I had thought to make the spawning of threads lazy. I can add a patch.

Comment 3

[Niko Matsakis [:nmatsakis]]

Attachment: Don't start worker threads if runtime tells us not to; in any case, start threads lazily.

Comment 4

[Niko Matsakis [:nmatsakis]]

Pushed to try server: http://tbpl.mozilla.org/?tree=Try&rev=f26d8f2b89b8

Comment 5

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

Comment on attachment 688485 [details] [diff] [review]
Don't start worker threads if runtime tells us not to; in any case, start threads lazily.

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

Could you clarify what's supposed to happen if rt->useHelperThreads() is false? In that case it seems like the user will get an OOM, which seems wrong to me.

::: js/src/vm/ThreadPool.cpp
@@ +193,5 @@
> +{
> +}
> +
> +bool
> +ThreadPool::init()

Is there any reason this couldn't be done in the constructor? We only put fallible stuff in init() methods.

@@ +203,5 @@
> +        numWorkers_ = 0;
> +
> +#ifdef DEBUG
> +    char *pathreads = getenv("PATHREADS");
> +    if (pathreads != NULL)

This can just be |if (char *pathreads = getenv("PATHREADS"))|

@@ +226,5 @@
>  #ifdef JS_THREADSAFE
> +    if (workers_.length() == 0) {
> +        // Allocate workers array and then start the worker threads.
> +        // Ensure that the field numWorkers_ always tracks the number of
> +        // *successfully initialized* workers.

I'm a little confused here. As far as I can tell, numWorkers_ isn't updated if we fail, which seems to belie the comment.

@@ +261,5 @@
>  {
>      runtime_->assertValidThread();
>  
> +    if (numWorkers_ == 0)
> +        return false;

If numWorkers_ == 0, does that mean that the user will get an OOM error? That seems like weird behavior for the DOM workers case.

@@ +268,4 @@
>          return false;
>  
>      // Find next worker in round-robin fashion.
> +    size_t id = JS_ATOMIC_INCREMENT(&nextId_) % numWorkers_;

Please use numWorkers() instead of numWorkers_.

@@ +275,5 @@
>  bool
>  ThreadPool::submitAll(TaskExecutor *executor)
>  {
> +    runtime_->assertValidThread();
> +

submitOne checks |numWorker_==0|, but this function doesn't. Shouldn't they both check?

@@ +277,5 @@
>  {
> +    runtime_->assertValidThread();
> +
> +    if (!lazyStartWorkers())
> +        return false;

As far as I can tell, the return value of this function is ignored in vm/ForkJoin.cpp.

@@ +279,5 @@
> +
> +    if (!lazyStartWorkers())
> +        return false;
> +
> +    for (size_t id = 0; id < numWorkers_; id++) {

Please use numWorkers() instead of numWorkers_.

::: js/src/vm/ThreadPool.h
@@ +88,5 @@
>  
>      bool init();
>  
>      // Return number of worker threads in the pool.
> +    size_t numWorkers() { return numWorkers_; }

It seems like it would be better to stick with the old code here (|workers_.length()|) since that's the number of workers that were actually started, even in the case of an error.

Comment 6

[Niko Matsakis [:nmatsakis]]

Thanks, I'll incorporate your edits.  Some responses below:

> Could you clarify what's supposed to happen if rt->useHelperThreads() is false?
> In that case it seems like the user will get an OOM, which seems wrong to me.

I'll add a comment.  In that case, no worker threads are started. As described in the ThreadPool.h comment, this is a legal state.

> Is there any reason this couldn't be done in the constructor? We only put fallible
> stuff in init() methods.

The code to compute the number of workers is in init() because the relevant fields in the runtime are not initialized yet during the constructor.

> I'm a little confused here. As far as I can tell, numWorkers_ isn't updated if we > fail, which seems to belie the comment.

Yes, the comment is wrong. numWorkers_ never changes, but the array may or may have the same length in the event of error.

> submitOne checks |numWorker_==0|, but this function doesn't. Shouldn't they both
> check?

No, only submitOne() should check.  The header file indicates why: submitAll() does work on all the workers (which may be 0).  The fork-join code, which uses submitAll(), also uses the main thread to do work, so there is always at least one worker.  

> As far as I can tell, the return value of this function is ignored in 
> vm/ForkJoin.cpp.

Yes, that's a bug.

> It seems like it would be better to stick with the old code here 
> (|workers_.length()|) since that's the number of workers that were actually
> started, even in the case of an error.

My intention was that if an error occurs the system does not proceed.  This function can be called before the threads have started and used to allocate buffers of the correct length and so forth, so the rest of the code relies on there being numWorkers_ worker threads precisely, not less.

Comment 7

[Niko Matsakis [:nmatsakis]]

Attachment: Don't start worker threads if runtime tells us not to; in any case, start threads lazily.

Comment 8

[Niko Matsakis [:nmatsakis]]

Try run for new patch: http://tbpl.mozilla.org/?tree=Try&rev=3e817da373e0

Comment 9

[Mozilla RelEng Bot]

Try run for f26d8f2b89b8 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f26d8f2b89b8
Results (out of 306 total builds):
    exception: 49
    success: 91
    warnings: 8
    failure: 158
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f26d8f2b89b8

Comment 10

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

Comment on attachment 688507 [details] [diff] [review]
Don't start worker threads if runtime tells us not to; in any case, start threads lazily.

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

Thanks for fixing this.

::: js/src/vm/ThreadPool.cpp
@@ +230,5 @@
> +
> +#ifndef JS_THREADSAFE
> +    return true;
> +#else
> +    if (workers_.length() == 0) {

Can you do it like this:

if (!workers_.empty())
    return true;

...create the workers...

That saves one level of indentation.

@@ +236,5 @@
> +        // Note that numWorkers_ is the number of *desired* workers,
> +        // but workers_.length() is the number of *successfully
> +        // initialized* workers.
> +        for (size_t workerId = 0; workerId < numWorkers(); workerId++) {
> +            ThreadPoolWorker *worker = js_new<ThreadPoolWorker>(workerId, this);

This can return NULL (for OOM), so that needs to be handled as well.

@@ +242,5 @@
> +                js_delete(worker);
> +                terminateWorkersAndReportOOM(cx);
> +                return false;
> +            }
> +            if (!worker->start()) {

Don't you need to delete |worker| in this case too?

@@ +258,5 @@
> +void
> +ThreadPool::terminateWorkersAndReportOOM(JSContext *cx)
> +{
> +    terminateWorkers();
> +    JS_ASSERT(workers_.length() == 0);

You can just assert worker_.empty().

Comment 11

[Niko Matsakis [:nmatsakis]]

I made the changes you suggested.

One response:

> Don't you need to delete |worker| in this case too?

No, because in that case worker is added into the workers_ array and hence is deleted by |terminateWorkersAndReportOOM()|.  I added a comment to that effect.

Comment 12

[Niko Matsakis [:nmatsakis]]

Try run for 3rd version of patch: http://tbpl.mozilla.org/?tree=Try&rev=939bb9de4c50

Comment 13

[Niko Matsakis [:nmatsakis]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/9a0801a8e9bf

Comment 14

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

Thanks Niko!

Comment 15

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

Oh, we usually only mark FIXED when the patch has merged over to mozilla-central. That should happen automatically (via sheriffs) in the next day or so.

Comment 16

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/9a0801a8e9bf

Comment 17

[Mozilla RelEng Bot]

Try run for 3e817da373e0 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=3e817da373e0
Results (out of 311 total builds):
    success: 288
    warnings: 21
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-3e817da373e0

Comment 18

[Mozilla RelEng Bot]

Try run for 939bb9de4c50 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=939bb9de4c50
Results (out of 312 total builds):
    success: 286
    warnings: 24
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-939bb9de4c50

Comment 19

[Mozilla RelEng Bot]

Try run for f26d8f2b89b8 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f26d8f2b89b8
Results (out of 307 total builds):
    exception: 49
    success: 91
    warnings: 8
    failure: 159
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f26d8f2b89b8