1458662 - Re-enable ARM64 regex JIT

Status: RESOLVED FIXED

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

Description

[Chris Peterson [:cpeterson]]

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

Comment 1

[Chris Peterson [:cpeterson]]

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.

Comment 2

[Lars T Hansen [:lth]]

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?

Comment 3

[Chris Peterson [:cpeterson]]

Jim, when you get a chance, can you try testing the ARM64 regex JIT again? Lars might have fixed the crashes you saw.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

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

Comment 5

[Chris Peterson [:cpeterson]]

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.

Comment 6

[Chris Peterson [:cpeterson]]

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?

Comment 7

[Matt Brubeck (:mbrubeck)]

Attachment: Bug 1458662 - Re-enable regex JIT on aarch64. r?jchen

MozReview-Commit-ID: 8d2pO69R2OD

Comment 8

[Phabricator Automation]

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

Comment 9

[Chris Peterson [:cpeterson]]

We don't need to uplift this fix to GV 62 Beta because we're not shipping an ARM64 build of Focus+GV yet.

Comment 10

[Matt Brubeck (:mbrubeck)]

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d6d39130d3c9
Re-enable regex JIT on aarch64. r=jchen

Comment 12

[Arthur Iakab [arthur_iakab]]

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

Comment 13

[Chris Peterson [:cpeterson]]

(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?

Comment 14

[Matt Brubeck (:mbrubeck)]

(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.