Closed Bug 1458662 Opened 2 years ago Closed Last year

Re-enable ARM64 regex JIT

Categories

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

ARM64
Android
defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: cpeterson, Assigned: mbrubeck, Mentored)

References

(Blocks 2 open bugs)

Details

(Keywords: good-first-bug, Whiteboard: [geckoview:crow:p2])

Attachments

(1 file)

jchen disabled javascript.options.native_regexp for ARM64 Android in bug 1357874 because it was crashing.

https://searchfox.org/mozilla-central/rev/795575287259a370d00518098472eaa5b362bfa7/mobile/android/app/mobile.js#874-876
Jim says he doesn't have specific STR or tests to reproduce the crashes, other than just visiting some top sites or tests that use regexes.
A couple months back I did fix a bug in the regex engine (a nonvolatile register was not properly saved across a call into jitted code), so it may be worth retesting this just to see if it's still broken.

We're running with native regexes enabled when running JS shell tests, but I don't think we're running even all the shell tests on device.  Also, the bug I fixed was for a release build (high optimization levels), ISTR an unoptimized build did not trigger it.  So how are we running our tests?
Jim, when you get a chance, can you try testing the ARM64 regex JIT again? Lars might have fixed the crashes you saw.
Flags: needinfo?(nchen)
Seems to be okay now from quick testing. We should remove the lines at https://searchfox.org/mozilla-central/rev/795575287259a370d00518098472eaa5b362bfa7/mobile/android/app/mobile.js#874-876
Mentor: nchen
Flags: needinfo?(nchen)
Keywords: good-first-bug
Summary: Fix ARM64 regex JIT crashes and re-enable JIT → Re-enable ARM64 regex JIT
Marking as [geckoview:crow:p2] bug because we'd like to ship this in Crow, but we don't need it in Klar. If no one volunteers to fix this bug in the next few weeks, someone on the GeckoView team can fix it for Crow.
Whiteboard: [geckoview:klar] [geckoview:crow] → [geckoview:crow:p2]
Dropping priority P3 so GeckoView will re-triage this bug. Is it too late to re-enable the ARM64 regex JIT before Crow ships in July?
Priority: P3 → --
Priority: -- → P3
Assignee: nobody → mbrubeck
MozReview-Commit-ID: 8d2pO69R2OD
Comment on attachment 8998000 [details]
Bug 1458662 - Re-enable regex JIT on aarch64. r?jchen

Jim Chen [:jchen] [:darchons] has approved the revision.

https://phabricator.services.mozilla.com/D2796
Attachment #8998000 - Flags: review+
We don't need to uplift this fix to GV 62 Beta because we're not shipping an ARM64 build of Focus+GV yet.
I tested locally this in a local aarch64 build on various jsperf links I found that used lots of regexes.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=48e276583bc30f1b2024844ae8c0e385dc86ef71
Pushed by mbrubeck@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d6d39130d3c9
Re-enable regex JIT on aarch64. r=jchen
https://hg.mozilla.org/mozilla-central/rev/d6d39130d3c9
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(In reply to Matt Brubeck (:mbrubeck) from comment #10)
> I tested locally this in a local aarch64 build on various jsperf links I
> found that used lots of regexes.

Did you see any speedup on the jsperf tests after enabling the ARM64 regex JIT?
(In reply to Chris Peterson [:cpeterson] from comment #13)
> Did you see any speedup on the jsperf tests after enabling the ARM64 regex
> JIT?

Sorry, I didn't actually do before/after comparisons of the perf numbers...  We could do this in Fennec nightly by flipping the pref in about:config.
You need to log in before you can comment on or make changes to this bug.