912589 - consider bumping up JSRuntime::helperThreadCount to js::GetCPUCount()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: bump numHelperThreads

(Right now it's GetCPUCount()-1 unless explicitly overridden.)  Conceivably, this could hurt main thread latency if the main thread and all workers threads are fully saturated, but it could help by putting spare cycles to use.  I tested this locally on octane using
  taskset 0x3 js --ion-parallel-compile=on run.js
(so that is 2 cpus) and hardcoding numHelperThreads() to 1 or 2.  Although there is a lot of variance, I see a distinct improvement from around ~16700 to ~16200.  Repeating the experiment for 4 cores shows no change.

The other reason I'd like to do this is for asm.js compilation on low #-core machines (say the dual-core FFOS phones).  During asm.js compilation, the main thread does parsing/MIR-generation and spawns compilation jobs as each function finishes parsing.  However, to avoid creating a ton of simultaneously-allocated MIRGraphs, the main thread blocks when there are sufficiently more jobs than workers threads.  Since compilation is much slower than parsing/emitting (roughly 3x), this means that one core will often be idle on a dual core.  Even on a quad-core Nexus 4, I see a 1 second speedup from bumping helperThreadCount() from 3 to 4.

Comment 1

[Nicolas B. Pierron [:nbp]]

(In reply to Luke Wagner [:luke] from comment #0)
> Even on a quad-core Nexus 4, I see a 1 second
> speedup from bumping helperThreadCount() from 3 to 4.

What is the memory impact of having 3 / 4 compiling threads instead of 1?

Comment 2

[Luke Wagner [:luke]]

(In reply to Nicolas B. Pierron [:nbp] from comment #1)
When there are N cores, we'll have N-1 threads right now (only for the main-thread JSRuntime, for workers, iiuc we disable helper threads), so the question is what is the memory impact of a single additional thread.  That I don't know.

Comment 3

[Brian Hackett [Laid off!]]

Comment on attachment 799623 [details] [diff] [review]
bump numHelperThreads

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

This looks fine, but the first paragraph in your description confuses me a bit.  If the octane score changed from 16700 to 16200 that would be a regression, and I don't know why we would see a variance in Ion compilation behavior with the change since we only let one thread do normal Ion compilation at once anyways.  Do you see a difference in score on octane-codeload?  That could be influenced by source compression I guess.

Comment 4

[Luke Wagner [:luke]]

Oops, I must've switched the numbers.  I rebuilt and re-ran, but now I don't see any difference (both average around 16200), so I think my new machine is rather irregular.

Comment 5

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39bba9d8dbac

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android NoIon (and *only* NoIon) jsreftest crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e5420f52425

https://tbpl.mozilla.org/php/getParsedLog.php?id=27593977&tree=Mozilla-Inbound

Comment 7

[Luke Wagner [:luke]]

Seems to be a !defined(JS_ION)-only crash.  Based on the crash address, I'm suspecting the cx->helperThreadCount()-query in ScriptSource::setSourceCopy.  Let's try:
  https://tbpl.mozilla.org/?tree=Try&rev=8d0bc042fb48

Comment 8

[Luke Wagner [:luke]]

Attachment: bump numHelperThreads v.2

It looks like I tickled a preexisting bug that occurs when !defined(JS_ION) and >1 core (which now triggered for 1 core).

Comment 9

[Brian Hackett [Laid off!]]

Comment on attachment 802328 [details] [diff] [review]
bump numHelperThreads v.2

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

::: js/src/vm/Runtime.h
@@ +1594,5 @@
>      }
>  
>      /* Number of helper threads which should be created for this runtime. */
>      size_t helperThreadCount() const {
>  #ifdef JS_THREADSAFE

I think it would be better if this was #ifdef JS_WORKER_THREADS instead.  That would remove the need for the ScriptSource::setSourceCopy change.

Comment 10

[Luke Wagner [:luke]]

Good point
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ade0212edd0

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3ade0212edd0

Comment 12

[Nicolas B. Pierron [:nbp]]

Have you checked on a computer which has only one core, such as a Unagi?
Does that has any impact there?  Sunspider and Kraken have regressed recently, I will need to bisect to figure if this is related.