927685 - Turn --ion-parallel-compile=on by default

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Terrence Cole [:terrence]]

The browser uses parallel compilation by default and the shell does not. Now that --enable-threadsafe is on by default for the shell, it makes sense to switch this to match the browser.

Comment 1

[Jan de Mooij [:jandem]]

We should be careful with jit-tests. In particular, I think we should use --ion-parallel-compile=off for the --ion-eager runs we have with jit_test.py --tbpl because:

(1) With --ion-eager we want to Ion-compile as much as possible. With parallel compilation it's possible for the script to finish before the background compilation is done and this does not give us a chance to enter Ion code.

(2) We should still run tests without parallel compilation. This matters for workers and single core hardware.

Comment 2

[Terrence Cole [:terrence]]

Attachment: default_parallel_compile-v0.diff

https://tbpl.mozilla.org/?tree=Try&rev=89501ccefd37

This toggles the pref and updates --tbpl as requested. Since I was touching the flags anyway, I moved them to a common location and added support for --tbpl to the jsreftests harness. To pay for the new complexity I removed --jitflags: it appears to have been actively broken since we've removed jaegermonkey anyway, so I don't think anyone will mind.

Jason, if you are sr+ for this, I will send a message out to js-internals before landing. Sorry I missed that last time!

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 822529 [details] [diff] [review]
default_parallel_compile-v0.diff

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

Thanks, Terrence.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 822529 [details] [diff] [review]
default_parallel_compile-v0.diff

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

Thanks for doing this!

::: js/src/shell/js.cpp
@@ +5566,5 @@
>      if (op->getBoolOption("ion-compile-try-catch"))
>          jit::js_IonOptions.compileTryCatch = true;
>  
>  #ifdef JS_THREADSAFE
> +    bool parallelCompilation = true;

This doesn't handle --ion-parallel-compile=off correctly. I think you want something like this:

    bool parallelCompilation = true;
    if (const char *str = op->getStringOption("ion-parallel-compile")) {
        if (strcmp(str, "off") == 0)
            parallelCompilation = false;
        else if (strcmp(str, "on") != 0)
            return OptionFailure("ion-parallel-compile", str);
    }

And remove the "In shell builds, parallel compilation is only enabled with an explicit option." comment in this function.

OffThreadIonCompilationEnabled also checks helperThreadCount() != 0 so I think we can safely remove that check here.

r=me with that.

Comment 5

[Terrence Cole [:terrence]]

A month later and this is finally green.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c14453ff8764

Comment 6

[Terrence Cole [:terrence]]

And allow --ion-parallel-compile to be recognized on non-threadsafe builds so that SM(r) doesn't break.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c63eaabaefb1

Comment 7

[Ryan VanderMeulen [:RyanVM]]

I had to back this out due to frequent jit-test failures in the parallel tests.
https://hg.mozilla.org/integration/mozilla-inbound/rev/20e4d6b3c819

https://tbpl.mozilla.org/php/getParsedLog.php?id=31514590&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=31516240&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=31516449&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=31516979&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=31515941&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=31517053&tree=Mozilla-Inbound

Comment 8

[Niko Matsakis [:nmatsakis]]

The failures seem to pertain to bailouts. Marking this bug as needinfo shu / pnkfelix / myself so that one of us can look into it shortly.

Comment 9

[Shu-yu Guo [:shu]]

Surprise surprise, I can't reproduce this locally. However, reading through the logs and seeing the different kinds of error messages makes me think that all of these failures is due to OMTC not finish compiling before the parallel test finishes.

If OMTC is on, ForkJoin runs the function passed to it sequentially while waiting for OMTC to finish. If it finishes executing before OMTC does, tests that expect successful parallel execution will get a "status=warmup" message, and tests that expect failure/bailout will get a "compilation required" message, which is what we see in those logs.

Not sure what the best way to fix this is: we can always wait for OMTC if a "mode" is passed in to ForkJoin, or we can wait for OMTC only if we're running inside a test harness.

Comment 10

[Niko Matsakis [:nmatsakis]]

Shu -- that is not how it's supposed to work. There is a "compile" phase that is supposed to block until compilation completes, in anticipation of precisely this problem. After compile phase completes, we then reiterate, this time expecting compiled versions to be ready. Maybe that compile phase isn't working right.

Comment 11

[Shu-yu Guo [:shu]]

(In reply to Niko Matsakis [:nmatsakis] from comment #10)
> Shu -- that is not how it's supposed to work. There is a "compile" phase
> that is supposed to block until compilation completes, in anticipation of
> precisely this problem. After compile phase completes, we then reiterate,
> this time expecting compiled versions to be ready. Maybe that compile phase
> isn't working right.

Pushed a try with spewing forced on and the backout un-backed out: https://tbpl.mozilla.org/?tree=Try&rev=d0e96b7be1ab

Maybe it'll tell us something new.

Comment 12

[Shu-yu Guo [:shu]]

That'll teach me to push to try off of inbound instead of central...

Comment 13

[Shu-yu Guo [:shu]]

Attachment: Fix race between off-main-thread-compilation and PJS.

OMTC can race with the PJS compilation model. This patch has PJS wait in
non-normal (i.e. testing) modes for all OMTC workers to finish, trigger the
operation callbacks, and service the callbacks before continuing.

Comment 14

[Shu-yu Guo [:shu]]

Stupid bzexport auto-assigning the bug on patch upload

Comment 15

[Shu-yu Guo [:shu]]

Attachment: Fix race between off-main-thread-compilation and PJS.

Oops, left a deubg printf in.

Comment 16

[Shu-yu Guo [:shu]]

Terrence, if 949916 sticks, could you try relanding your patches?

Comment 17

[Felix S. Klock II [:pnkfelix, :fklock]]

i trust you guys are handling this, clearing my "needinfo?"

Comment 18

[Terrence Cole [:terrence]]

And relanding: https://hg.mozilla.org/integration/mozilla-inbound/rev/df3c2a1e86d3

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/df3c2a1e86d3