Closed Bug 1400383 Opened 4 years ago Closed 4 years ago

Clamp HelperThreadState.cpuCount to 8

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- wontfix

People

(Reporter: luke, Assigned: luke)

Details

Attachments

(1 file)

Attached patch clamp-num-cpusSplinter Review
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 on attachment 8908772 [details] [diff] [review]
clamp-num-cpus

Thoughts?
Attachment #8908772 - Attachment is patch: true
Attachment #8908772 - Flags: review?(lhansen)
Attachment #8908772 - Flags: review?(jdemooij)
Priority: -- → P3
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.
Attachment #8908772 - Flags: review?(lhansen) → review+
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.
Attachment #8908772 - Flags: review?(jdemooij) → review+
(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.
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?
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.
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.
https://hg.mozilla.org/mozilla-central/rev/06dfdd599999
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee: nobody → luke
You need to log in before you can comment on or make changes to this bug.