Closed
Bug 1206770
Opened 9 years ago
Closed 7 years ago
The shell --thread-count argument doesn't set the thread count
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: jonco, Assigned: lth)
Details
Attachments
(2 files)
5.11 KB,
patch
|
Details | Diff | Splinter Review | |
2.21 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
Instead it sets a fake CPU count, and the number of helper threads is calculated by adding four to this number. This is pretty confusing.
Reporter | ||
Comment 1•9 years ago
|
||
Here's a patch to rename the current --thread-count option to --cpu-count and add a --thread-count that does what you'd expect. The motivation behind all this is that to make simulated OOM testing thread safe we need to have the option of running with only a single helper thread.
Attachment #8663734 -
Flags: review?(jdemooij)
Comment 2•9 years ago
|
||
Do jit-tests pass with --thread-count=1? I'm wondering if this does the right thing, considering the comment in ThreadCountForCPUCount: // Create additional threads on top of the number of cores available, to // provide some excess capacity in case threads pause each other.
Reporter | ||
Comment 3•9 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #2) Yes that's a good point. There's a single failure - debug/Memory-drainAllocationsLog-13.js times out.
Comment 4•9 years ago
|
||
Maybe we can disable pausing if we have < x threads available?
Comment 5•9 years ago
|
||
Comment on attachment 8663734 [details] [diff] [review] fix-thread-count Review of attachment 8663734 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review flag for now.
Attachment #8663734 -
Flags: review?(jdemooij)
Reporter | ||
Comment 6•9 years ago
|
||
I looked into this some more, and it's not the pausing that is causing the problem. We have a parse task running on the single helper thread that is blocked waiting for the runtime's exclusive access lock. The GC is running on the main thread and holding that lock. It has queued a parallel GC task and is waiting for it to finish, but it can't even start because the parse task is running.
Reporter | ||
Comment 7•7 years ago
|
||
We found another way of making simulated OOM testing thread safe so closing this.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Assignee | ||
Comment 8•7 years ago
|
||
"Every word she writes is a lie, including 'And' and 'The'." We really should do something about this. Not only is --thread-count not the appropriate name, but the documentation is all wrong. It says --thread-count sets the number of helper threads to N-1, but it doesn't, it sets it to N+4.
Assignee: jcoppeard → lhansen
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Assignee | ||
Comment 9•7 years ago
|
||
Let me rephrase that. The documentations says --thread-count=N sets us up to use N auxiliary threads and that the default is numcores-1. I guess this depends on what you mean by 'auxiliary'. It really sets the core count to N, and the default value for the core count is the actual core count. Then the number of helper threads is N+4.
Assignee | ||
Comment 10•7 years ago
|
||
A simple halfway point: - add --core-count=N with the same semantics as --thread-count has now, but with the correct documentation - make --thread-count an alias of --core-count and document it as such This opens up for migrating away from --thread-count to --core-count, and eventually removing the former or giving it new semantics.
Attachment #8905941 -
Flags: review?(jdemooij)
Comment 11•7 years ago
|
||
From my understanding, this ambiguously-named '--thread-count' is for overriding the default value returned by GetCPUCount() and stored in HelperThreadState().cpuCount which is the number of logical processors (i.e., counting hyperthreads). I usually take "core" to mean physical processors so --core-count sounds a bit off. "CPU" is sufficiently ambiguous that it isn't totally wrong so, as long as cpuCount has its name, how about naming the flag '--cpu-count' for symmetry?
Assignee | ||
Comment 12•7 years ago
|
||
Potato-potahto, I'm fine either way as long as the blantant misinformation disappears :)
Comment 13•7 years ago
|
||
Comment on attachment 8905941 [details] [diff] [review] bug1206770-thread-count.patch Review of attachment 8905941 [details] [diff] [review]: ----------------------------------------------------------------- Forwarding to Luke since he already looked at this and is probably more familiar with the semantics.
Attachment #8905941 -
Flags: review?(jdemooij) → review?(luke)
Comment 14•7 years ago
|
||
Comment on attachment 8905941 [details] [diff] [review] bug1206770-thread-count.patch Review of attachment 8905941 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8905941 -
Flags: review?(luke) → review+
Comment 15•7 years ago
|
||
Pushed by lhansen@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1b55231e6628 Add --cpu-count with correct documentation, make --thread-count an alias of --cpu-count. r=luke
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1b55231e6628
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•