930570 - Build fails with --disable-ion

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Julien Wajsberg [:julienw]]

And we use --disable-ion on B2G...

Why TBPL has not caught this is probably because they use a custom userconfig which does _not_ disable Ion ? NI jgriffin for this.

I have a hacky patch for this.

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: patch v1

Comment 2

[(PTO June 19-July 7) Ryan VanderMeulen]

Ion is only disabled for B2G 1.1 and older. v1.2+ (Gecko26+) have it enabled.

Comment 3

[Julien Wajsberg [:julienw]]

Looks like bug 900669 landed this code.

Comment 4

[Julien Wajsberg [:julienw]]

Ryan> mmm it seems you're right, my gonk-misc seems to be in v1-train for some reason.

So this bug is still valid imo, but my config is invalid too.

Comment 5

[Luke Wagner [:luke]]

This fixes --disable-ion builds for me:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2becacaadf0b

Comment 6

[Julien Wajsberg [:julienw]]

Ryan> Talked with Vivien, we'll probably disable ion on 1.2 because of bug 922440

Comment 7

[Julien Wajsberg [:julienw]]

Luke> Great!

Comment 8

[(PTO June 19-July 7) Ryan VanderMeulen]

(In reply to Julien Wajsberg [:julienw] from comment #4)
> So this bug is still valid imo, but my config is invalid too.

Absolutely, if for no other reason than not breaking the various tier-3s that don't have a functioning JIT at the moment :)

(In reply to Julien Wajsberg [:julienw] from comment #6)
> Ryan> Talked with Vivien, we'll probably disable ion on 1.2 because of bug
> 922440

That seems like a very scary thing to change this late in the release cycle, especially because the No-Ion configuration hasn't been tested in quite some time now. Note that the *only* No-Ion testing being run *anywhere* on TBPL is Android NoIon jsreftests on b2g18*. That means no mochitest, jsreftest, jit-test, Marionette, etc tested in that configuration. Also, given that Baseline Compiler is tied to Ion these days, you would basically be completely disabling all JITs on v1.2, which I would imagine would regress JS performance to worse than v1.1-levels.

Comment 9

[Julien Wajsberg [:julienw]]

I won't argue about this, I don't know much and I won't be the one making that decision ;-)

Comment 10

[Luke Wagner [:luke]]

There was a rather large market/partner pressure to turn on Ion for benchmarketting purposes, so I'd be quite surprised if it was disabled at this point.

Comment 11

[Nicolas B. Pierron [:nbp]]

(In reply to Julien Wajsberg [:julienw] from comment #6)
> Ryan> Talked with Vivien, we'll probably disable ion on 1.2 because of bug
> 922440

1/ This is not Ion in fault but the baseline compiler. (read the comments)
2/ *I* will not approve the huge regression from switching from a JIT to the Interpreter.
3/ I am working on it right now.
4/ Our partners will not approve the huge regression from switching from a JIT to the Interpreter.

The flag --disable-ion is still useful for third parties who want to take time to port SpiderMonkey to a different architecture.

From what I understand the --disable-ion flag is fixed now, right?  So we can mark this bug as a duplicate of "no bug" (aka. https://hg.mozilla.org/integration/mozilla-inbound/rev/2becacaadf0b)

Comment 12

[Sean Stangl [:sstangl]]

Note that the choice is no longer between Ion and a slightly slower JIT (JaegerMonkey), but between Ion and a much slower JIT (Baseline), JaegerMonkey having been removed from the tree. If Baseline is turned off due to application startup concerns, then JS runs interpreter-only.

Were Ion to not be preffed on in 1.2, there would be a significant performance regression. So the only plausible way forward is to fix Baseline/Ion.

Comment 13

[Julien Wajsberg [:julienw]]

Sorry guys, please don't take an uninformed sentence as a decision. Nothing was decided, I think we (as in Vivien and I) didn't know the consequences of disabling Ion and/or the Baseline, and it's quite clear the more informed people (aka: the JS Engine engineers) will decide about this.

Luke, if https://hg.mozilla.org/integration/mozilla-inbound/rev/2becacaadf0b landed we might as well close this bug as fixed ?

Comment 14

[Luke Wagner [:luke]]

(In reply to Julien Wajsberg [:julienw] from comment #13)
Yep.  Please feel free to confirm, though; I just tested building the shell with --disable-ion.

Comment 15

[Landry Breuil (:gaston)]

chiming in, also saw my non-ion firefox desktop builds failing on sparc64 (see http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/588/steps/build/logs/stdio), will confirm tmrw that 2becacaadf0b fixes the whole build.

Comment 16

[Landry Breuil (:gaston)]

http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/590 is running with 2becacaadf0b qimport'ed on top of m-c.. we'll see how it goes.

Comment 17

[Julien Wajsberg [:julienw]]

(In reply to Luke Wagner [:luke] from comment #14)
> (In reply to Julien Wajsberg [:julienw] from comment #13)
> Yep.  Please feel free to confirm, though; I just tested building the shell
> with --disable-ion.

Just did it, this fixes it for me. Closing then.

(off topic, Landry, I love your domain name)

Comment 18

[Landry Breuil (:gaston)]

Build fixed for me too.

Comment 19

[(PTO June 19-July 7) Ryan VanderMeulen]

(In reply to Sean Stangl [:sstangl] from comment #12)
> Note that the choice is no longer between Ion and a slightly slower JIT
> (JaegerMonkey), but between Ion and a much slower JIT (Baseline),
> JaegerMonkey having been removed from the tree. If Baseline is turned off
> due to application startup concerns, then JS runs interpreter-only.

IIUC, it's worse than that - the --disable-ion flag controls *both* BC and Ion. So B2G would be running all JS in the interpreter only. Ouch.