1434224 - Remove excess helper threads

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Jon Coppeard (:jonco)]

We removed the ability to pause off-thread ion compilation threads in bug 1398140, but we still create extra threads because of it:

static size_t
ThreadCountForCPUCount(size_t cpuCount)
{
    // Create additional threads on top of the number of cores available, to
    // provide some excess capacity in case threads pause each other.
    static const uint32_t EXCESS_THREADS = 4;
    return cpuCount + EXCESS_THREADS;
}

We should remove this.

Comment 1

[Lars T Hansen [:lth]]

I'm not completely convinced we don't depend on this somewhere.  ISTR some argument I made about the thread count being at least two, and that this is important in some cases.

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Lars T Hansen [:lth] from comment #1)
Any idea what the dependency might be?  We could ensure the thread count is at least two if that's better.  Creating all these extra threads seems like a waste of resources, especially now we have e10s and have this for every process.

Comment 3

[Lars T Hansen [:lth]]

At the moment the code in the helperthreads system indicates that we do need two threads for tier-2 wasm compilations, because there's a master task that holds a thread while other threads do the compilation.  See StartOffThreadWasmTier2Generator and its callees.

(Technically we need an extra thread for every cluster of tier2 compilations but at the moment we only allow one of these.)

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1434224-remove-excess-threads

Comment 5

[Lars T Hansen [:lth]]

Comment on attachment 8947024 [details] [diff] [review]
bug1434224-remove-excess-threads

Review of attachment 8947024 [details] [diff] [review]:
-----------------------------------------------------------------

I *think* this is OK but we're going to ask Luke anyway.

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8947024 [details] [diff] [review]
bug1434224-remove-excess-threads

Review of attachment 8947024 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/HelperThreads.cpp
@@ +88,5 @@
>  {
> +    // We need at least two threads for tier-2 wasm compilations, because
> +    // there's a master task that holds a thread while other threads do the
> +    // compilation. (Technically we need an extra thread for every cluster of
> +    // tier2 compilations but at the moment we only allow one of these.)

Agreed on needing at least 2.  I think the parenthetical isn't needed though: if we allowed multiple to be in progress at a time, since they are noted to be master=true, then the scheduler would take into account the number of threads and prevent more than one master task from executing at a time.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33647efec4ca
Remove excess helper threads now ion compilations no longer pause each other r=lth r=luke

Comment 8

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/33647efec4ca