Build fails with --disable-ion

RESOLVED FIXED in mozilla27

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: julienw, Assigned: luke)

Tracking

unspecified
mozilla27
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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.
Flags: needinfo?(jgriffin)
(Reporter)

Comment 1

5 years ago
Created attachment 821716 [details] [diff] [review]
patch v1
Ion is only disabled for B2G 1.1 and older. v1.2+ (Gecko26+) have it enabled.
(Reporter)

Comment 3

5 years ago
Looks like bug 900669 landed this code.
Blocks: 900669
(Reporter)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
This fixes --disable-ion builds for me:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2becacaadf0b
(Reporter)

Comment 6

5 years ago
Ryan> Talked with Vivien, we'll probably disable ion on 1.2 because of bug 922440
(Reporter)

Comment 7

5 years ago
Luke> Great!
(Reporter)

Updated

5 years ago
Flags: needinfo?(jgriffin)
(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.
(Reporter)

Comment 9

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

Comment 10

5 years ago
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.
(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)
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.
(Reporter)

Comment 13

5 years ago
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 ?
(Assignee)

Comment 14

5 years ago
(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.
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.
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.
(Reporter)

Comment 17

5 years ago
(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)
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Build fixed for me too.
(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.
Assignee: nobody → luke
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.