Closed
Bug 1400383
Opened 7 years ago
Closed 7 years ago
Clamp HelperThreadState.cpuCount to 8
Categories
(Core :: JavaScript Engine, enhancement, P3)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | wontfix |
People
(Reporter: luke, Assigned: luke)
Details
Attachments
(1 file)
1.77 KB,
patch
|
lth
:
review+
jandem
:
review+
|
Details | Diff | Splinter 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.
![]() |
Assignee | |
Comment 1•7 years ago
|
||
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)
Updated•7 years ago
|
status-firefox57:
--- → wontfix
Priority: -- → P3
Comment 2•7 years ago
|
||
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 3•7 years ago
|
||
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+
![]() |
Assignee | |
Comment 4•7 years ago
|
||
(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•7 years ago
|
||
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?
![]() |
Assignee | |
Comment 6•7 years ago
|
||
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.
![]() |
Assignee | |
Comment 7•7 years ago
|
||
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•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•6 years ago
|
Assignee: nobody → luke
You need to log in
before you can comment on or make changes to this bug.
Description
•