Closed Bug 1523015 Opened 2 years ago Closed 1 year ago

Allow ARM64 Ion to be toggled by pref

Categories

(Core :: JavaScript Engine: JIT, defect, P2)

ARM64
Unspecified
defect

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr60 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- fixed

People

(Reporter: sstangl, Assigned: sstangl)

References

(Blocks 1 open bug)

Details

(Whiteboard: [arm64:m2])

Attachments

(2 files, 1 obsolete file)

The attached patch removes the code that hard-disables IonMonkey for ARM64. With this patch applied:

  1. Shell builds will run with Ion enabled by default. This includes on TBPL, where new failures will occur. I believe this is acceptable because there are already a few Baseline failures, the number of new IonMonkey failures is low (double-digits), and we are actively working on fixing them.

  2. Browser builds will run with Ion disabled by default, but it can be enabled manually by using about:config to change javascript.options.ion to boolean "true".

@nbp, this was requested by Steve, forwarding the requests of other managers. The hope is that we're in a state where things aren't too crashy, and so they can at least view the behavior personally on a subset of webpages.

Things like "unsigned division" still crash, but I'll fix that up next week.

Attachment #9039268 - Flags: review?(nicolas.b.pierron)

v2: Same as the other patch, but I just realized that if we're enabling Ion for shell, we might as well enable the tests also, since they pass.

Attachment #9039268 - Attachment is obsolete: true
Attachment #9039268 - Flags: review?(nicolas.b.pierron)
Attachment #9039269 - Flags: review?(nicolas.b.pierron)
Attachment #9039269 - Flags: review?(nicolas.b.pierron) → review+
Keywords: checkin-needed

Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0121f2af5821
Enable Ion on ARM64, but disable in-browser by pref. r=nbp

Keywords: checkin-needed

Backed out changeset 0121f2af5821 (bug 1523015) for spidermonkey failures at js/src/jit-test/tests/atomics/mutual-exclusion.js

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/67d7ef194fbee7c5925ef1d67c310e5257c188a3

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&selectedJob=224485151&revision=0121f2af5821a32deb329f1acf7e1ad4d7787560

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=224485151&repo=mozilla-inbound&lineNumber=4226

[task 2019-01-28T16:59:06.174Z] TEST-PASS | js/src/jit-test/tests/auto-regress/bug1183241.js | Success (code 3, args "") [0.2 s]
[task 2019-01-28T16:59:06.175Z] {"action": "test_start", "jitflags": "", "pid": 12958, "source": "jittests", "test": "auto-regress/bug1183241.js", "thread": "main", "time": 1548694746.004868}
[task 2019-01-28T16:59:06.176Z] {"action": "test_end", "extra": {"jitflags": "", "pid": 12958}, "jitflags": "", "message": "Success", "pid": 12958, "source": "jittests", "status": "PASS", "test": "auto-regress/bug1183241.js", "thread": "main", "time": 1548694746.174743}
[task 2019-01-28T16:59:06.190Z] Limit = 1
[task 2019-01-28T16:59:06.192Z] Hit MOZ_CRASH(NYI) at /builds/worker/workspace/build/src/js/src/jit/arm64/CodeGenerator-arm64.cpp:1734
[task 2019-01-28T16:59:06.193Z] Exit code: -11
[task 2019-01-28T16:59:06.194Z] FAIL - atomics/mutual-exclusion.js
[task 2019-01-28T16:59:06.195Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/atomics/mutual-exclusion.js | Hit MOZ_CRASH(NYI) at /builds/worker/workspace/build/src/js/src/jit/arm64/CodeGenerator-arm64.cpp:1734 (code -11, args "") [0.3 s]
[task 2019-01-28T16:59:06.195Z] {"action": "test_start", "jitflags": "", "pid": 12909, "source": "jittests", "test": "atomics/mutual-exclusion.js", "thread": "main", "time": 1548694745.928952}
[task 2019-01-28T16:59:06.196Z] {"action": "test_end", "extra": {"jitflags": "", "pid": 12909}, "jitflags": "", "message": "Hit MOZ_CRASH(NYI) at /builds/worker/workspace/build/src/js/src/jit/arm64/CodeGenerator-arm64.cpp:1734", "pid": 12909, "source": "jittests", "status": "FAIL", "test": "atomics/mutual-exclusion.js", "thread": "main", "time": 1548694746.190998}
[task 2019-01-28T16:59:06.197Z] INFO exit-status : -11
[task 2019-01-28T16:59:06.198Z] INFO timed-out : False
[task 2019-01-28T16:59:06.199Z] INFO stdout > Limit = 1
[task 2019-01-28T16:59:06.200Z] INFO stderr 2> Hit MOZ_CRASH(NYI) at /builds/worker/workspace/build/src/js/src/jit/arm64/CodeGenerator-arm64.cpp:1734
[task 2019-01-28T16:59:06.201Z] TEST-PASS | js/src/jit-test/tests/atomics/store-does-not-truncate-returnval.js | Success (code 0, args "") [0.2 s]
[task 2019-01-28T16:59:06.201Z] {"action": "test_start", "jitflags": "", "pid": 12947, "source": "jittests", "test": "atomics/store-does-not-truncate-returnval.js", "thread": "main", "time": 1548694745.985358}
[task 2019-01-28T16:59:06.201Z] {"action": "test_end", "extra": {"jitflags": "", "pid": 12947}, "jitflags": "", "message": "Success", "pid": 12947, "source": "jittests", "status": "PASS", "test": "atomics/store-does-not-truncate-returnval.js", "thread": "main", "time": 1548694746.191292}
[task 2019-01-28T16:59:06.277Z] TEST-PASS | js/src/jit-test/tests/auto-regress/bug1147907.js | Success (code 0, args "") [0.3 s]
[task 2019-01-28T16:59:06.277Z] {"action": "test_start", "jitflags": "", "pid": 12957, "source": "jittests", "test": "auto-regress/bug1147907.js", "thread": "main", "time": 1548694745.999065}
[task 2019-01-28T16:59:06.277Z] {"action": "test_end", "extra": {"jitflags": "", "pid": 12957}, "jitflags": "", "message": "Success", "pid": 12957, "source": "jittests", "status": "PASS", "test": "auto-regress/bug1147907.js", "thread": "main", "time": 1548694746.274499}
[task 2019-01-28T16:59:06.316Z] Hit MOZ_CRASH(NYI) at /builds/worker/workspace/build/src/js/src/jit/arm64/Lowering-arm64.cpp:381
[task 2019-01-28T16:59:06.316Z] Exit code: -11
[task 2019-01-28T16:59:06.316Z] FAIL - atomics/basic-tests.js
[task 2019-01-28T16:59:06.316Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/atomics/basic-tests.js | Hit MOZ_CRASH(NYI) at /builds/worker/workspace/build/src/js/src/jit/arm64/Lowering-arm64.cpp:381 (code -11, args "") [0.4 s]
[task 2019-01-28T16:59:06.316Z] {"action": "test_start", "jitflags": "", "pid": 12907, "source": "jittests", "test": "atomics/basic-tests.js", "thread": "main", "time": 1548694745.9149501}
[task 2019-01-28T16:59:06.316Z] {"action": "test_end", "extra": {"jitflags": "", "pid": 12907}, "jitflags": "", "message": "Hit MOZ_CRASH(NYI) at /builds/worker/workspace/build/src/js/src/jit/arm64/Lowering-arm64.cpp:381", "pid": 12907, "source": "jittests", "status": "FAIL", "test": "atomics/basic-tests.js", "thread": "main", "time": 1548694746.314398}
[task 2019-01-28T16:59:06.316Z] INFO exit-status : -11
[task 2019-01-28T16:59:06.316Z] INFO timed-out : False

Flags: needinfo?(sstangl)

There are also jit failures on Android AArch: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=224493440&repo=mozilla-inbound&lineNumber=1983

17:42:18 INFO - TEST-PASS | tests/jit-test/jit-test/tests/bug828119.js | Success (code 0, args "--no-baseline --no-ion --more-compartments") [0.2 s]
17:42:18 INFO - {"action": "test_start", "jitflags": "--no-baseline --no-ion --more-compartments", "pid": 429, "source": "jittests", "test": "bug828119.js", "thread": "main", "time": 1548697338.6115952}
17:42:18 INFO - {"action": "test_end", "extra": {"jitflags": "--no-baseline --no-ion --more-compartments"}, "jitflags": "--no-baseline --no-ion --more-compartments", "message": "Success", "pid": 429, "source": "jittests", "status": "PASS", "test": "bug828119.js", "thread": "main", "time": 1548697338.816862}
17:42:19 INFO - exception output: Assertion failed: bad label: 2:
17:42:19 INFO - abort@/data/local/tests/jit-tests/jit-tests/tests/bug830943.js:32:34
17:42:19 INFO - assert@/data/local/tests/jit-tests/jit-tests/tests/bug830943.js:37:9
17:42:19 INFO - _main@/data/local/tests/jit-tests/jit-tests/tests/bug830943.js:98:13
17:42:19 INFO - callMain@/data/local/tests/jit-tests/jit-tests/tests/bug830943.js:255:12
17:42:19 INFO - doRun@/data/local/tests/jit-tests/jit-tests/tests/bug830943.js:261:19
17:42:19 INFO - run@/data/local/tests/jit-tests/jit-tests/tests/bug830943.js:265:16
17:42:19 INFO - @/data/local/tests/jit-tests/jit-tests/tests/bug830943.js:268:1
17:42:19 INFO - uncaught exception: Assertion: Assertion failed: bad label: 2
17:42:19 INFO - (Unable to print stack trace)

(In reply to Cristina Coroiu [:ccoroiu] from comment #3)

Backed out changeset 0121f2af5821 (bug 1523015) for spidermonkey failures at js/src/jit-test/tests/atomics/mutual-exclusion.js

I think, as written by Sean, this is the whole point of enabling this feature sooner, is to keep track of these issues on our CI without having a special build.

(In reply to Sean Stangl [:sstangl] from comment #0)

@nbp, this was requested by Steve, forwarding the requests of other managers. The hope is that we're in a state where things aren't too crashy, and so they can at least view the behavior personally on a subset of webpages.

ni? Steve, to see if we can find a solution to keep track of these crashes on CI even if we know that they are a few expected crashes that we are going to fix in the upcoming weeks.

Flags: needinfo?(sdetar)

Sean said this might not be possible now, if that is the case we will just need to wait a coupe weeks until we at the point these crashes are not happening.

Flags: needinfo?(sdetar)
Pushed by sstangl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1b9f4cb229fa
Summary: Enable Ion on ARM64, but disable in-browser by pref. r=nbp

The failing jsapi-test contains the comment "Disable the JIT because if we don't this test fails. See bug 1160414."

This patch keeps the jit-tests disabled in both the shell and in the browser, so I don't see why that should change. Will require investigation.

Ah, the test accidentally turns on Ion, even though it should be disabled.

Pushed by sstangl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2b3012fa3cbf
Summary: Enable Ion on ARM64, but disable in-browser by pref. r=nbp
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Looks like this brought big Speedometer improvements on AARCH64! \0/

== Change summary for alert #19341 (as of Tue, 12 Feb 2019 21:00:25 GMT) ==

Improvements:

14% raptor-speedometer-geckoview android-hw-p2-8-0-android-aarch64 opt 18.22 -> 20.75

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=19341

It shouldn't have... it's supposed to be disabled!

Flags: needinfo?(sstangl)
Summary: Enable Ion on ARM64 but disable in-browser by pref → Allow ARM64 Ion to be toggled by pref
Depends on: 1528621

Is there anything that needs to happen here for 66?

Flags: needinfo?(sstangl)

Liz, yes -- in Bug 1528621.

Flags: needinfo?(sstangl)

huh? this didn't land on 66, so bug 1528621 shouldn't be needed there

Am I missing something in comment 19?

Flags: needinfo?(sstangl)

Oh, sorry, I didn't parse the 66 correctly. The pref wasn't enabled for 66.

Flags: needinfo?(sstangl)
You need to log in before you can comment on or make changes to this bug.