1431353 - Investigate improving performance of off-thread parsing

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jon Coppeard (:jonco)]

We have an API to parse JS code in on a helper thread while leaving the main thread free to do other things.  I'd like us to make heavier use of this in the future, specifically for preloading module scripts.  This bug is about improving its performance.

Comment 1

[Jon Coppeard (:jonco)]

First of all, we currently only allow a single off-thread parse to be active at once.  I'd like to remove this limitation.

The comments say:

    // Don't allow simultaneous off thread parses, to reduce contention on the
    // atoms table. Note that wasm compilation depends on this to avoid
    // stalling the helper thread, as off thread parse tasks can trigger and
    // block on other off thread wasm compilation tasks.

(see https://searchfox.org/mozilla-central/source/js/src/vm/HelperThreads.cpp#1194 )

We have a per-zone atom cache that was added fairly recently (bug 1350760) so access to the atoms table should not be such a bottleneck now.

Luke, is the point about wasm compilation still valid?  What can we do to address this?

Comment 2

[Luke Wagner [:luke]]

Good news: this comment goes back to the beginning and it is to prevent a dining philosophers deadlock since parse tasks with asm.js could block on other helper tasks.  Since then, we (Lars) made scheduling a hair more sophisticated by adding the "isMaster" argument (which means "this task can block on other helper tasks completing) to checkTaskThreadLimit() which ensures that we never have all helper threads running master tasks.  We even set isMaster=true in canStartParseTask() with a comment for asm.js... but apparently we never bumped maxParseTasks() up from 1 :P

Comment 3

[Lars T Hansen [:lth]]

The master task management was inspired by off-thread parsing but was actually implemented for the sake of wasm tiered compilation, which initially had a master task.  (I'm fairly sure Luke later rewrote that code to avoid the master task for wasm.)  Feel free to increase the maximum number of parse tasks :)

(Insert standard observation about needing a more dynamic scheduler.)

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1431353-remove-parse-thread-limit

Great, here's a patch to remove this limit.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bug1431353-shell-off-thread-jobs

We should expose this via the shell so that it gets fuzz coverage.  Previously we could only compile a single script off-thread at a time.

This patch removes this limitation and gives each shell context a list of off-thread parse jobs.  Shell functions that start an off-thread job (offThreadCompileScript, offThreadCompileModule and offThreadDecodeScript) now return an integer ID.  This can then be passed to the functions that wait for the jobs to finish (runOffThreadScript, finishOffThreadModule and runOffThreadDecodedScript).

So as not to have to change all the existing test code I made the ID argument optional in the case where there's only a single pending job.

Comment 6

[Jan de Mooij [:jandem]]

Great to have this fixed!

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8943967 [details] [diff] [review]
bug1431353-shell-off-thread-jobs

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

Nice!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91e2856488e7
Remove limit on number of threads used for off-thread parsing r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/467d285e001c
Regoranise the shell interface to off-thread parsing to allow concurrent off-thread parse jobs r=luke

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/91e2856488e7
https://hg.mozilla.org/mozilla-central/rev/467d285e001c