793740 - ARMv6 builds should specify JM usage, even when running on ARMv7 boards

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Alex Keybl [:akeybl]]

ARMv6 builds should specify JM usage, even when running on ARMv7 boards.

Comment 1

[Alex Keybl [:akeybl]]

dvander - do you have an estimate of when this can land? This blocks a final call on whether or not to release the ARMv6 build as part of FF16.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Ideally this would be done by shipping different browser prefs with ARMv6 builds, by setting javascript.options.typeinference to false. I don't know how to do that, so if no one else does, another option would be to use a build-time flag in js/src.

Marty, are there any compiler #defines we can use to differentiate ARMv6 builds from ARMv7?

Comment 3

[Marty Rosenberg [:mjrosenb]]

in the arm assembler from methodjit/jsc, there are a few of these:
#if WTF_ARM_ARCH_VERSION >= 7
but I'm pretty sure that not all armv7-specific code is correctly wrapped, but if we actually build and run on armv6 targets,  guess it is good enough?

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

Thanks, Marty, this should do it then.

Comment 5

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 664664 [details] [diff] [review]
fix

Review of attachment 664664 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, Do we have any way of testing this? I've been burned in the past by WTF macros not actually being defined in a manner that makes sense.

Comment 6

[Justin Wood (:Callek)]

(In reply to Marty Rosenberg [:mjrosenb] from comment #5)
> Comment on attachment 664664 [details] [diff] [review]
> fix
> 
> Review of attachment 664664 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me, Do we have any way of testing this? I've been burned in
> the past by WTF macros not actually being defined in a manner that makes
> sense.

Armv6 builds are built on our try, though we don't have any production armv6 devices running those tests atm. We do however have Armv7-devices running the unittests of these armv6 builds -- so if its testable, even by force-failing for just try, thats how.

Comment 7

[Justin Wood (:Callek)]

(In reply to Marty Rosenberg [:mjrosenb] from comment #3)
> in the arm assembler from methodjit/jsc, there are a few of these:
> #if WTF_ARM_ARCH_VERSION >= 7
> but I'm pretty sure that not all armv7-specific code is correctly wrapped,
> but if we actually build and run on armv6 targets,  guess it is good enough?

Actually its not -- the reason we're even worrying about this BUILD side, is because we're currently testing on armv7 devices, our armv6 builds.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Try builds with this patch are here: ftp://ftp.mozilla.org/pub/mobile/try-builds/[e-mail]-0d4ce027c42c/try-android-armv6/

I don't have an Android phone so I can't test this apk. Basically all we should need to verify that the patch works is that running SunSpider is significantly slower.

Comment 9

[Justin Wood (:Callek)]

(In reply to David Anderson [:dvander] from comment #8)
> Try builds with this patch are here:
> ftp://ftp.mozilla.org/pub/mobile/try-builds/[e-mail]-
> 0d4ce027c42c/try-android-armv6/
> 
> I don't have an Android phone so I can't test this apk. Basically all we
> should need to verify that the patch works is that running SunSpider is
> significantly slower.

Testing on my S3, First --> install ftp://ftp.mozilla.org/pub/mobile/try-builds/[e-mail]-0d4ce027c42c/try-android --> reboot phone --> launch Nightly, run Sunspider --> E-mail myself url of results.

Then Uninstall Nightly, reinstall from ftp://ftp.mozilla.org/pub/mobile/try-builds/[e-mail]-0d4ce027c42c/try-android-armv6 --> reboot phone --> launch nightly, run sunspider --> e-mail myself url of results.

Load first URL on desktop, paste into text box second url, result:



TEST                   COMPARISON            FROM                 TO             DETAILS

=============================================================================

** TOTAL **:           1.76x as fast     3370.6ms +/- 2.3%   1909.7ms +/- 2.7%     significant

=============================================================================

  3d:                  1.35x as fast      543.8ms +/- 5.6%    403.2ms +/- 6.6%     significant
    cube:              1.28x as fast      195.8ms +/- 11.7%    152.5ms +/- 10.8%     significant
    morph:             2.62x as fast      128.7ms +/- 5.9%     49.2ms +/- 11.8%     significant
    raytrace:          -                  219.3ms +/- 7.8%    201.5ms +/- 9.6% 

  access:              3.85x as fast      455.8ms +/- 6.0%    118.5ms +/- 11.9%     significant
    binary-trees:      2.55x as fast       51.5ms +/- 12.4%     20.2ms +/- 14.0%     significant
    fannkuch:          2.41x as fast      120.7ms +/- 3.3%     50.0ms +/- 5.2%     significant
    nbody:             6.42x as fast      178.4ms +/- 8.9%     27.8ms +/- 43.6%     significant
    nsieve:            5.13x as fast      105.2ms +/- 7.1%     20.5ms +/- 4.4%     significant

  bitops:              2.22x as fast      199.1ms +/- 8.0%     89.6ms +/- 6.3%     significant
    3bit-bits-in-byte: 2.17x as fast       12.6ms +/- 29.7%      5.8ms +/- 5.2%     significant
    bits-in-byte:      3.66x as fast       53.0ms +/- 8.1%     14.5ms +/- 3.5%     significant
    bitwise-and:       2.07x as fast       78.5ms +/- 21.9%     37.9ms +/- 11.0%     significant
    nsieve-bits:       1.75x as fast       55.0ms +/- 1.9%     31.4ms +/- 11.7%     significant

  controlflow:         -                   20.5ms +/- 16.9%     16.9ms +/- 15.2% 
    recursive:         -                   20.5ms +/- 16.9%     16.9ms +/- 15.2% 

  crypto:              ??                 251.1ms +/- 11.7%    268.1ms +/- 12.0%     not conclusive: might be *1.068x as slow*
    aes:               -                  160.3ms +/- 18.9%    134.1ms +/- 12.6% 
    md5:               *1.61x as slow*     53.4ms +/- 4.3%     86.1ms +/- 6.5%     significant
    sha1:              ??                  37.4ms +/- 33.4%     47.9ms +/- 36.3%     not conclusive: might be *1.28x as slow*

  date:                -                  340.8ms +/- 8.7%    324.1ms +/- 3.3% 
    format-tofte:      -                  173.8ms +/- 8.0%    165.9ms +/- 4.3% 
    format-xparb:      -                  167.0ms +/- 12.1%    158.2ms +/- 5.9% 

  math:                3.30x as fast      364.2ms +/- 5.5%    110.4ms +/- 12.5%     significant
    cordic:            4.23x as fast      105.7ms +/- 17.6%     25.0ms +/- 1.9%     significant
    partial-sums:      3.54x as fast      203.1ms +/- 7.7%     57.4ms +/- 24.9%     significant
    spectral-norm:     1.98x as fast       55.4ms +/- 6.3%     28.0ms +/- 15.5%     significant

  regexp:              -                   81.9ms +/- 19.0%     70.5ms +/- 23.6% 
    dna:               -                   81.9ms +/- 19.0%     70.5ms +/- 23.6% 

  string:              2.19x as fast     1113.4ms +/- 3.7%    508.4ms +/- 8.9%     significant
    base64:            1.76x as fast       73.2ms +/- 11.1%     41.6ms +/- 16.7%     significant
    fasta:             9.20x as fast      559.3ms +/- 4.8%     60.8ms +/- 27.6%     significant
    tagcloud:          1.22x as fast      195.7ms +/- 6.0%    160.1ms +/- 14.9%     significant
    unpack-code:       -                  192.5ms +/- 11.9%    183.2ms +/- 12.3% 
    validate-input:    1.48x as fast       92.7ms +/- 8.8%     62.7ms +/- 8.4%     significant

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Thank you for testing, Callek! It looks like this works so I'll check it in shortly.

Comment 11

[Alex Keybl [:akeybl]]

(In reply to David Anderson [:dvander] from comment #10)
> Thank you for testing, Callek! It looks like this works so I'll check it in
> shortly.

Please also nominate for uplift to m-a and m-b as soon as possible. We're working on a very short timeline to react to broken tests here.

Comment 12

[Armen [:armenzg]]

What is the status of this bug? Did the changes get uplifted?

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff77ff9b2033

Comment 14

[Alex Keybl [:akeybl]]

Comment on attachment 664664 [details] [diff] [review]
fix

Conditional on this check-in looking good on central, approving for Aurora/Beta as well to ensure that this is exercised in our tests asap.

Comment 15

[Phil Ringnalda (:philor)]

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

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

https://hg.mozilla.org/releases/mozilla-beta/rev/773303abaae9

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

This needs to land on Aurora before Monday Oct 8th in order to be in our first 17.0 beta.  Email sent as well.

Comment 18

[Justin Wood (:Callek)]

Comment on attachment 664664 [details] [diff] [review]
fix

Review of attachment 664664 [details] [diff] [review]:
-----------------------------------------------------------------

I attempted to land into aurora just now, but get:

$ hg qpush
applying bug793740-DisableARMv6.patch
patching file js/src/jsinfer.cpp
Hunk #1 FAILED at 2040
1 out of 1 hunks FAILED -- saving rejects to file js/src/jsinfer.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh bug793740-DisableARMv6.patch

And I'm not about to attempt unwinding .rej's right now.

Comment 19

[David Anderson [:dvander] - inactive, e-mail if emergency]

https://hg.mozilla.org/releases/mozilla-beta/rev/2d380f304bf0