1400383 - Clamp HelperThreadState.cpuCount to 8

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Luke Wagner [:luke]]

Attachment: clamp-num-cpus

The vast majority of FF devices have <=8 cores:
  https://hardware.metrics.mozilla.com/#goto-cpu-and-memory)
so this patch mostly won't affect anyone.  But for users with more cores:
 - Having so many threads clutters up gdb and crash reports and grabs a lot of stack memory.
 - I think it's extremely rare for SM to have more than 3 cores worth of work; pretty much only wasm compilation can use more.
 - For wasm compilation, I tend to see slowdowns past --cpu-count=8 (up to 2x!)  Probably this is from NUMA and SM not even attempting to be NUMA-aware but we're just not optimized for high core counts in general (esp if you look at HelperThreads.cpp).

A Right(tm)er fix would be to query the number of cores in a single NUMA node, but I think just using "8" is Good Enough for today's devices (happy to revisit later).  Mostly this will just make developers with beefy build machines lives' easier by giving more realistic perf numbers w/o having to take special steps to set cpuCount.

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 8908772 [details] [diff] [review]
clamp-num-cpus

Thoughts?

Comment 2

[Lars T Hansen [:lth]]

Comment on attachment 8908772 [details] [diff] [review]
clamp-num-cpus

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

I ran into the same issue (slowdown at CPU counts > 8) last week on my Xeon NUMA system and I'm generally in favor of this.  But like other scheduler policy matters we probably need to have some kind of plan for how to maintain constants like these going forward.  (Bug 1380033 introduces several more.)  I would expect '8' to grow over time, indeed, high-end AMD Threadripper implementations have 12 and 16 cores, and Intel i9 parts have 10 and 12 cores, and I think (not able to dig down deeply enough right now) these are non-NUMA designs.

So: I'm OK with landing this now because I think it's the right decision now, but we need a bug filed for a more sophisticated policy in the nearish future.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8908772 [details] [diff] [review]
clamp-num-cpus

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

Being conservative with this makes sense to me.

Comment 4

[Luke Wagner [:luke]]

(In reply to Lars T Hansen [:lth] from comment #2)
> I would expect '8' to grow over time, indeed, high-end AMD
> Threadripper implementations have 12 and 16 cores, and Intel i9 parts have
> 10 and 12 cores, and I think (not able to dig down deeply enough right now)
> these are non-NUMA designs.

Agreed; using GetLogicalProcessorInformationEx() and some sort of /sys/ parsing we should be able to get the max number of processors in a single NUMA node.

Comment 5

[Jon Coppeard (:jonco)]

The GC makes use of cpuCount when starting tasks for sweeping and compacting activities and can use up to |cpuCount| tasks:

http://searchfox.org/mozilla-central/source/js/src/jsgc.cpp#2593
http://searchfox.org/mozilla-central/source/js/src/jsgc.cpp#5947

I agree we shouldn't have so many threads, but could we keep cpuCount as the actual CPU count and change threadCount instead?

Comment 6

[Luke Wagner [:luke]]

One concern I had with leaving cpuCount unchanged was that we might be implicitly assuming threadCount >= cpuCount, assuming that threadCount was cpuCount "with some extra".  Also, if we're using cpuCount to throttle the amount of work we spawn (which we seem to do in multiple places), then it seems like we'd be throttling incorrectly with threadCount clamped to 8.

Comment 7

[Luke Wagner [:luke]]

Note, I think the bot that auto-posts hg URLs and makes #developers comments on commits is not working; I actually landed this patch earlier today (before comment 5):
  https://hg.mozilla.org/integration/mozilla-inbound/rev/06dfdd599999
I say this just so, when the bot comes alive and presumably posts here, you don't think I'm ignoring your comment :)  Happy to continue discussion.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/06dfdd599999