The shell --thread-count argument doesn't set the thread count

RESOLVED FIXED in Firefox 57

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: jonco, Assigned: lth)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments)

Reporter

Description

4 years ago
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

4 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)
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

4 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.
Maybe we can disable pausing if we have < x threads available?
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

4 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

2 years ago
We found another way of making simulated OOM testing thread safe so closing this.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
Assignee

Comment 8

2 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

2 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

2 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)
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

2 years ago
Potato-potahto, I'm fine either way as long as the blantant misinformation disappears :)
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 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

2 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
https://hg.mozilla.org/mozilla-central/rev/1b55231e6628
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.