711156 - Perf: regression on performance on startup from 12/11 to 12/12

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

1. see https://docs.google.com/spreadsheet/ccc?key=0Arku3jleCA0UdFNQdkpuWVZIbjBPQ1gxa3RldzF2b1E&hl=en_US#gid=0
Regression range 201111206 ~ 20111215

1) Download java app : http://dump.lassey.us/time onto the desktop machine
2) using adb : 
adb push time /data/local
3) change the permission of the file so you can execute:
ie adb shell chmod 777 /data/local/time
4) download http://people.mozilla.com/~nhirata/Perf/startup5.html onto the desktop machine
push the html files to the android device using:
adb push <local file> /sdcard/download/
5) reboot the android device
6) adb shell
7) run am start -a android.intent.action.VIEW -n org.mozilla.fennec/.App -d file://mnt/sdcard/download/startup5.html#"`millistring=\`/data/local/time\`; echo \`date \"+%s\"\`${millistring##*.}`"

Instructions/Device setup can be seen here:
https://etherpad.mozilla.org/fennec-perf-ts-take2

Note: I suspect that it may be due to the about:home showing prior to the startup5.html page loading.

Comment 1

[Doug Turner (:dougt)]

can you get the regression range down to 1 day or 1 cset?

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Assigning to Naoki to get a regression range

Comment 3

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

The regression range seems to be a gradual increase from 12/11/2011 to 12/15/2011 with the largest increase between 12/12/2011 and 12/13/2011.

See line 3 of https://docs.google.com/spreadsheet/ccc?key=0Arku3jleCA0UdFNQdkpuWVZIbjBPQ1gxa3RldzF2b1E&hl=en_US#gid=0

Comment 4

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Forgot to mention the columns: W thru AA

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Naoki, each regression needs a separate bug. From there it would be good to bisect on-change builds to the changeset that caused the regression. I'd start with the regression from 12/12 to 12/13

Comment 6

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

It seems like the regression between 12/12 to 12/13 is due to this change set.  I'm unsure exactly why.

[e-mail] – Mon Dec 12 06:17:41 2011 PST (compare: )List changeset URLsSelf-serve Build API

    73979783bac9
    Kartikaya Gupta – Bug 708683 - Improve JSON generation by using JSONStringer. r=pcwalton a=java-only
    4850eb9ce32a
    Kartikaya Gupta – Bug 708683 - Fix NaN viewport values. r=pcwalton


See the Chart at R/50 located here:
https://docs.google.com/spreadsheet/ccc?key=0Arku3jleCA0UdFNQdkpuWVZIbjBPQ1gxa3RldzF2b1E&hl=en_US#gid=29

Comment 7

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Regression from 12/11 to 12/12 seems to be the following change set:

1ce022be38d4
Mark Finkle – Bug 709048 - Over usage of haptic buzz [r=mbrubeck a=khuey]
71dfb2adaf0f
Chris Peterson – Bug 706984 - Check whether profile directory exists to avoid NullPointerException. r=dougt a=khuey

See chart at R 51 : https://docs.google.com/spreadsheet/ccc?key=0Arku3jleCA0UdFNQdkpuWVZIbjBPQ1gxa3RldzF2b1E&hl=en_US#gid=31

Changing the title of this bug to reflect this regression range.

Comment 8

[Doug Turner (:dougt)]

my bet is that it is chris's fix.  It adds a stat.

Nhirata, are you able to test two different builds - one with his fix and one with out it?

Comment 9

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I will take the time out to do so if I have builds. Is there an easy way to push both builds to try?

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Bug 708651 will re-arrange this code, actually adding more stats (via File.exists calls)

Comment 11

[Chris Peterson [:cpeterson]]

My fix for bug 706984 added a file.exists() check to avoid a NullPointerException. I can submit a new patch that (optimistically) skips the file.exists() and instead checks for profiles==null. This approach would avoid the costs of an extra stat() or exception throw/catch.

Should I investigate this? Or just wait for bug 708651 to rewrite this code?

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

we really just shouldn't be calling getProfileDir() on the main thread, which we do here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1583

Let's just move this to the background thread, that should eliminate any start up perf regression, right?

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: untested patch

I'm assuming this was put on the main thread because of the SharedPreferences access, which we're not doing anymore. Alex, can you confirm the update check doesn't need to be on main thread, and GCP can you do the same for the migration check?

Comment 14

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 585463 [details] [diff] [review]
untested patch

Migrator only needs a looper so this will be fine. (It will pop up a GUI at some point, but we can deal with that then)

Comment 15

[Alex Pakhotin (:alexp)]

Comment on attachment 585463 [details] [diff] [review]
untested patch

I don't think it matters for the update check what thread it's running on. There is no UI - just calls to some system functions.

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 17

[Marco Bonardo [:mak]]

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

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 585463 [details] [diff] [review]
untested patch

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
we're touching the disk on the main thread in the start up path, which slows down start up
Testing completed (on m-c, etc.): 
explicit testing on updates prior to landing. Continuing testing on m-c
Risk to taking this patch (and alternatives if risky):
risks (updater and profile migrator) have been identified and patch reviewed by feature owners

Comment 19

[Alex Keybl [:akeybl]]

Comment on attachment 585463 [details] [diff] [review]
untested patch

[Triage Comment]
Mobile only - approved for Aurora.

Comment 20

[Catalin Suciu [:csuciu]]

Verified fixed on Nightly 12.0a1 (2012-01-31)
                  Aurora  11.0a2 (2012-01-31)
Device:Samsung Galaxy S2 (Android 2.3.4)