1528621 - IonMonkey enabled by mistake on Fennec Nightly.

Status: RESOLVED FIXED

(Firefox Build System :: General, defect, P1)

Description

[Calixte Denizet (:calixte)]

This bug is for crash report bp-9b7b9461-5551-48f6-ab38-ce8850190214.

Top 5 frames of crashing thread:

0 libxul.so js::jit::PatchJump js/src/jit/arm64/Assembler-arm64.cpp:377
1 libxul.so js::jit::IonCacheIRCompiler::compile js/src/jit/IonCacheIRCompiler.cpp:630
2 libxul.so js::jit::IonIC::attachCacheIRStub js/src/jit/IonCacheIRCompiler.cpp:2568
3 libxul.so js::jit::IonSetPropertyIC::update js/src/jit/IonIC.cpp:282
4  @0x7b627373d8 

There are 32 crashes (from 23 installations) in nightly 67 starting with buildid 20190213102848. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1520478.

[1] https://hg.mozilla.org/mozilla-central/rev?node=ba0fe524ced9

Comment 1

[Jan de Mooij [:jandem]]

ARM64 crashes in Ion IC code.

Are we sure the Ion pref is working as expected on ARM64? We have (1) these crashes and (2) unexpected perf improvements when we added the pref https://bugzilla.mozilla.org/show_bug.cgi?id=1523015#c15

It's possible some people enabled Ion manually but we should double check.

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #1)

[…] https://bugzilla.mozilla.org/show_bug.cgi?id=1523015#c15

It's possible some people enabled Ion manually but we should double check.

I noticed that on Fennec Nightlies it was enabled by default. I presume this is an issue with the pre-processing of all.js which might be pre-processed with different architecture guards than on Windows ARM64.

Comment 3

[Jan de Mooij [:jandem]]

At least the pref issue should be fixed ASAP.

Comment 4

[Sean Stangl [:sstangl]]

It appears that _ARM64_ is only set on platforms that use NSPR, of which Android is not one.

Per config/external/nspr/pr/moz.build.

Comment 5

[Sean Stangl [:sstangl]]

Mike, would you happen to know where architecture information is supposed to be surfaced in #defines?

Comment 6

[(no longer active)]

If you need _ARM64_ on Android, you can do something like bug 1481505 for Android (around here for example: https://searchfox.org/mozilla-central/rev/93905b660fc99a5d52b683690dd26471daca08c8/old-configure.in#627

Comment 7

[Mike Hommey [:glandium]]

old-configure.in has a AC_DEFINE(_ARM64_), so it should, in fact, be defined.

Comment 8

[Mike Hommey [:glandium]]

Oh, that's for fennec, yeah, _ARM64_ is only defined for windows.

Comment 9

[Lars T Hansen [:lth]]

Elsewhere (https://searchfox.org/mozilla-central/source/js/src/jit/AtomicOperations.h#379) I have been using

#elif defined(aarch64) || defined(_M_ARM64)

and the #ifdef nest in question has an #error in the final #else, so this probably works on all our platforms.

Comment 10

[Mike Hommey [:glandium]]

Those are C/C++ defines, defined by the compiler. The preferences file is not C/C++, and doesn't get the compiler defines.

Comment 11

[Chris Peterson [:cpeterson]]

Changing whiteboard tag from [arm64:m2] to [arm64:m3] because this bug doesn't block enabling Ion in Nightly. We actually need to temporarily disable Ion to fix this crash.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Steven, could you please help with this issue? Thanks

Comment 13

[Steven DeTar [:sdetar]]

Sylvestre, could you help me understand what needs to be done here? It seems Nicolas identified the fix is needed in the build system and moved the bug to that queue. Was that the right approach?

Comment 14

[Steven DeTar [:sdetar]]

I needinfo'd the wrong person, sorry about that.

Comment 15

[Chris Peterson [:cpeterson]]

(In reply to Steven DeTar [:sdetar] from comment #13)

Sylvestre, could you help me understand what needs to be done here? It seems Nicolas identified the fix is needed in the build system and moved the bug to that queue. Was that the right approach?

Comment 16

[Sylvestre Ledru [:Sylvestre]]

I discussed with Steven on slack about that

Comment 17

[Chris Peterson [:cpeterson]]

Mike, can you fix this bug to define the right ARM64 macros on Android or suggest another engineer who can help? This bug is causing unnecessary crashes in Fennec Nightly because the ARM64 Ion JIT is not ready to be enabled. Nathan fixed a similar problem for ARM64 Windows in bug 1481505.

Which macros do we want defined for both ARM64 Windows and Android?

mozilla-central #ifdefs currently reference:

_ARM64_ (14 instances)
_M_ARM64 (93 instances)
__aarch64__ (336 instances)
__arm64__ (1 instance in ctypes)

Comment 18

[Mike Hommey [:glandium]]

Attachment: Bug 1528621 - Disable ionmonkey on aarch64 as originally intended.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6b6cba576de
Disable ionmonkey on aarch64 as originally intended. r=dmajor

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

From https://bugzilla.mozilla.org/show_bug.cgi?id=1523015#c18, sounds this also needs to be on beta.
Can you request beta uplift? Thanks!

Comment 21

[Oana Pop-Rus]

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

Comment 22

[Ionuț Goldan [:igoldan]]

This brought one of the benchmarks to previous values:

== Change summary for alert #19726 (as of Thu, 28 Feb 2019 22:41:35 GMT) ==

Regressions:

13% raptor-speedometer-geckoview android-hw-p2-8-0-android-aarch64 opt 21.36 -> 18.55

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

Comment 23

[Chris Peterson [:cpeterson]]

(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #22)

Regressions:

13% raptor-speedometer-geckoview android-hw-p2-8-0-android-aarch64 opt 21.36 -> 18.55

This regression is expected because we're temporarily turning off the Ion JIT. The good news is that we know the Ion JIT will give us a ~13% Speedometer improvement (like we saw in bug 1523015 comment 15 when the Ion JIT pref was accidentally turned on). :)

Comment 24

[Mike Hommey [:glandium]]

Clearing ni per bug 1523015 comment 21.

Comment 25

[Chris Peterson [:cpeterson]]

We don't need to uplift to 66 Beta because we are not publishing ARM64 Fennec Beta builds on Google Play. We are only publishing ARM64 Fennec Nightly (67) builds at this time.