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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: jonco, Assigned: lth)

Details

Attachments

(2 files)

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.
Attached patch fix-thread-countSplinter Review
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.
(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)
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.
We found another way of making simulated OOM testing thread safe so closing this.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
"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 → ---
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.
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?
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+
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: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.