Closed Bug 711156 Opened 8 years ago Closed 8 years ago

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

Categories

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

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 12
Tracking Status
firefox11 --- verified
firefox12 --- verified
fennec 11+ ---

People

(Reporter: nhirata, Assigned: blassey)

References

Details

(Keywords: perf)

Attachments

(1 file)

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.
can you get the regression range down to 1 day or 1 cset?
Assigning to Naoki to get a regression range
Assignee: nobody → nhirata.bugzilla
Priority: -- → P1
OS: Mac OS X → Android
Hardware: x86 → ARM
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
Forgot to mention the columns: W thru AA
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
It seems like the regression between 12/12 to 12/13 is due to this change set.  I'm unsure exactly why.

kgupta@mozilla.com – 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
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.
Summary: Perf: regression on performance on startup → Perf: regression on performance on startup from 12/11 to 12/12
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?
I will take the time out to do so if I have builds. Is there an easy way to push both builds to try?
Bug 708651 will re-arrange this code, actually adding more stats (via File.exists calls)
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?
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?
Attached patch untested patchSplinter Review
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?
Assignee: nhirata.bugzilla → blassey.bugs
Attachment #585463 - Flags: review?(gpascutto)
Attachment #585463 - Flags: review?(alexp)
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)
Attachment #585463 - Flags: review?(gpascutto) → review+
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.
Attachment #585463 - Flags: review?(alexp) → review+
https://hg.mozilla.org/mozilla-central/rev/d7a02042ab5b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 12
tracking-fennec: --- → 11+
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
Attachment #585463 - Flags: approval-mozilla-aurora?
Comment on attachment 585463 [details] [diff] [review]
untested patch

[Triage Comment]
Mobile only - approved for Aurora.
Attachment #585463 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed on Nightly 12.0a1 (2012-01-31)
                  Aurora  11.0a2 (2012-01-31)
Device:Samsung Galaxy S2 (Android 2.3.4)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.