1014183 - Massive-lua benchmark running in worker much slower than in shell (non-odin)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Alon Zakai (:azakai)]

The work in progress Massive benchmark for asm.js contains a lua-binarytrees benchmark. When run *without* odin/asm.js, it takes almost 10x longer than when odin is enabled. However, this does not happen in the shell: in the shell, without odin it takes just a little longer, which makes sense. Numbers:

                   asm     no-asm
browser (worker)   6.5      10.8
shell              6.5      51.1 (!)

(results in seconds, lower is better)

Do we have some optimizations disabled on code running in workers? Massive runs all tests in workers to avoid hanging the main thread. If we have a script size limit that is worker-only, that could explain this.

STR in browser:

1. clone https://github.com/kripken/Massive (master branch)
2. edit driver.js, in the array 'jobs' which defines the benchmarks, remove all the objects but the one starting with      benchmark: 'lua-binarytrees         (otherwise it will run all the other benchmarks too, which takes a while)
3. run a webserver (python -m SimpleHTTPServer)
4. browse to localhost:8000, run benchmark, see the result

STR for shell:

a. in the same repo, enter the lua/ subdir and run   benchmark-shell.js      , see the 'runtime' result (it also mentions 'startup' but that isn't important here)

Comment 1

[Boris Zbarsky [:bzbarsky]]

> If we have a script size limit that is worker-only, that could explain this.

We effectively do.

The worker JSRuntime is created with JS_NO_HELPER_THREADS.  So we don't do offthread ion compilation there and I believe that reduces the size of script we're willing to ion-compile.

Comment 2

[Alon Zakai (:azakai)]

Is that intentional or a temporary thing? It sounds like a problem for any large codebase running in workers, and we are trying to move more things to workers all the time, to improve responsiveness.

ccing Hannes as well as I think he is working on something related to helper threads.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I _assume_ the intent is for workers to not starve the main thread of helpers?  But I agree that the results are highly undesirable.

Comment 4

[Andrew McCreight (out of office until 8/21) [:mccr8]]

There's plenty of ideas for how to improve worker performance (see the bugs blocking bug 941783).  Ideally, the JS runtime in DOM workers would be just as awesome as they are on the main thread.

Comment 5

[Luke Wagner [:luke]]

Originally, helper threads were not used for workers because helper threads were not shared process-wide.  Now they are and Brian explains the current bug holding us back from using helper threads in workers in bug 981935 comment 8: we need bug 966646 which has a b2g-ICS-opt-emulator-only failure.  Before anything else, it'd probably be worth re-pushing that patch to try to see if the problem magically went away in the interim (a lot has changed).

Comment 6

[Andrew McCreight (out of office until 8/21) [:mccr8]]

> Before anything else, it'd probably be worth re-pushing that patch to try to see if the problem magically went away in the interim (a lot has changed).

I think that was bug 1006695, which has been fixed.  bhackett and I have both rebased the patch and done a try run.  The GC helper threads were properly set up for Nuwa, but the JS helper threads were not, and I guess the JS helper threads normally didn't run before the Nuwa split, but the GC ones did, so when the GC helpers became JS helpers, and triggered the Nuwa problem.

Comment 7

[Luke Wagner [:luke]]

Hah, I just saw that; what a nice coincidence.

Comment 8

[Brian Hackett [Laid off!]]

Well, I don't think the prospects of bug 966646 getting fixed anytime soon are any good (and I'm done with working on that bug) but that doesn't need to block this if we're willing to do something sufficiently disgusting and split the use of GC helper threads away from the use of other helper threads in the runtime configuration.  This should let DOM workers use off thread Ion compilation, parsing and source compression but they wouldn't do any GC stuff in the background.  Of course, doing this might also anger the b2g-ICS-emulator-opt gods and make this impossible to land.  If someone else wants to work on this that would be great, I just find the prospect of doing any work related to JS threading incredibly revolting right now.

Comment 9

[Brian Hackett [Laid off!]]

Actually, I'm being really immature about this.  I'll write a patch today to fix this per comment 8, we need to keep all the BS in bug 966646 from holding this up.

Comment 10

[Brian Hackett [Laid off!]]

Attachment: patch

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8428191 [details] [diff] [review]
patch

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

I don't think we need this any more.

Comment 12

[Florian Bender]

Alon, care to rerun the test with a fresh build and see if (and what) there's still work to do here?

Comment 13

[Alon Zakai (:azakai)]

Ok, I re-ran after this change. Things look much better - speed on workers is now the same as on the main thread on Massive. Great! Thanks bhackett and everyone else that fixed the blocking bugs.

This also makes Massive faster on Firefox-without-asm.js than Chrome (Firefox-with-asm.js was already faster).

So I think we can close this.