Closed Bug 912589 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: luke, Assigned: luke)

References

Details

(Keywords: perf, Whiteboard: [c=benchmark p= s=2013.09.20 u=])

Attachments

(1 file, 1 obsolete file)

Attached patch bump numHelperThreads (obsolete) — Splinter Review
(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.
Attachment #799623 - Flags: review?(bhackett1024)
(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?
(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 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.
Attachment #799623 - Flags: review?(bhackett1024) → review+
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.
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
It looks like I tickled a preexisting bug that occurs when !defined(JS_ION) and >1 core (which now triggered for 1 core).
Attachment #799623 - Attachment is obsolete: true
Attachment #802328 - Flags: review?(bhackett1024)
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.
Attachment #802328 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/3ade0212edd0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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.
Depends on: 915167
Keywords: perf
Whiteboard: [c=benchmark p= s=2013.09.20 u=]
You need to log in before you can comment on or make changes to this bug.