Last Comment Bug 793740 - ARMv6 builds should specify JM usage, even when running on ARMv7 boards
: ARMv6 builds should specify JM usage, even when running on ARMv7 boards
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 18
Assigned To: David Anderson [:dvander]
:
Mentors:
Depends on:
Blocks: 787893 793994
  Show dependency treegraph
 
Reported: 2012-09-24 09:40 PDT by Alex Keybl [:akeybl]
Modified: 2012-10-20 09:13 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
fixed


Attachments
fix (1.77 KB, patch)
2012-09-25 15:05 PDT, David Anderson [:dvander]
marty.rosenberg: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Alex Keybl [:akeybl] 2012-09-24 09:40:05 PDT
ARMv6 builds should specify JM usage, even when running on ARMv7 boards.
Comment 1 Alex Keybl [:akeybl] 2012-09-25 12:48:37 PDT
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] 2012-09-25 14:10:12 PDT
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] 2012-09-25 14:38:25 PDT
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] 2012-09-25 15:05:41 PDT
Created attachment 664664 [details] [diff] [review]
fix

Thanks, Marty, this should do it then.
Comment 5 Marty Rosenberg [:mjrosenb] 2012-09-25 16:03:20 PDT
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) 2012-09-26 17:26:02 PDT
(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) 2012-09-26 17:31:11 PDT
(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] 2012-09-26 17:47:45 PDT
Try builds with this patch are here: ftp://ftp.mozilla.org/pub/mobile/try-builds/danderson@mozilla.com-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) 2012-09-26 18:27:03 PDT
(In reply to David Anderson [:dvander] from comment #8)
> Try builds with this patch are here:
> ftp://ftp.mozilla.org/pub/mobile/try-builds/danderson@mozilla.com-
> 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/danderson@mozilla.com-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/danderson@mozilla.com-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] 2012-09-26 21:06:32 PDT
Thank you for testing, Callek! It looks like this works so I'll check it in shortly.
Comment 11 Alex Keybl [:akeybl] 2012-09-27 12:59:16 PDT
(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 Zambrano [:armenzg] (EDT/UTC-4) 2012-09-28 11:31:42 PDT
What is the status of this bug? Did the changes get uplifted?
Comment 13 David Anderson [:dvander] 2012-09-28 12:13:19 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff77ff9b2033
Comment 14 Alex Keybl [:akeybl] 2012-09-28 14:37:13 PDT
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) 2012-09-28 22:25:30 PDT
https://hg.mozilla.org/mozilla-central/rev/ff77ff9b2033
Comment 16 David Anderson [:dvander] 2012-10-01 15:37:09 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/773303abaae9
Comment 17 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-05 17:18:50 PDT
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) 2012-10-07 21:17:01 PDT
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] 2012-10-19 15:37:34 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/2d380f304bf0

Note You need to log in before you can comment on or make changes to this bug.