Closed
Bug 711156
Opened 14 years ago
Closed 13 years ago
Perf: regression on performance on startup from 12/11 to 12/12
Categories
(Firefox for Android Graveyard :: General, defect, P1)
Tracking
(firefox11 verified, firefox12 verified, fennec11+)
VERIFIED
FIXED
Firefox 12
People
(Reporter: nhirata, Assigned: blassey)
References
Details
(Keywords: perf)
Attachments
(1 file)
745 bytes,
patch
|
alexp
:
review+
gcp
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
can you get the regression range down to 1 day or 1 cset?
Assignee | ||
Comment 2•13 years ago
|
||
Assigning to Naoki to get a regression range
Assignee: nobody → nhirata.bugzilla
Priority: -- → P1
Assignee | ||
Updated•13 years ago
|
OS: Mac OS X → Android
Hardware: x86 → ARM
![]() |
Reporter | |
Comment 3•13 years ago
|
||
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
![]() |
Reporter | |
Comment 4•13 years ago
|
||
Forgot to mention the columns: W thru AA
Assignee | ||
Comment 5•13 years ago
|
||
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
![]() |
Reporter | |
Comment 6•13 years ago
|
||
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
![]() |
Reporter | |
Comment 7•13 years ago
|
||
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
Comment 8•13 years ago
|
||
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?
![]() |
Reporter | |
Comment 9•13 years ago
|
||
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•13 years ago
|
||
Bug 708651 will re-arrange this code, actually adding more stats (via File.exists calls)
Comment 11•13 years ago
|
||
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?
Assignee | ||
Comment 12•13 years ago
|
||
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?
Assignee | ||
Comment 13•13 years ago
|
||
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 14•13 years ago
|
||
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 15•13 years ago
|
||
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+
Assignee | ||
Comment 16•13 years ago
|
||
Whiteboard: [inbound]
Comment 17•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 12
Assignee | ||
Updated•13 years ago
|
tracking-fennec: --- → 11+
Assignee | ||
Updated•13 years ago
|
status-firefox11:
--- → affected
status-firefox12:
--- → fixed
Assignee | ||
Comment 18•13 years ago
|
||
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 19•13 years ago
|
||
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+
Updated•13 years ago
|
Comment 20•13 years ago
|
||
Verified fixed on Nightly 12.0a1 (2012-01-31)
Aurora 11.0a2 (2012-01-31)
Device:Samsung Galaxy S2 (Android 2.3.4)
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•