[Gecko] ~50ms Phone and Contacts Launch Regression

RESOLVED FIXED in mozilla34



5 years ago
4 years ago


(Reporter: mchang, Assigned: jrmuizel)


({perf, regression})

Gonk (Firefox OS)
perf, regression

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [c=regression p= s= u=])


(1 attachment)

Phone cold launch time regressed ~50ms, Contacts regressed ~50ms. 

Contacts link: https://datazilla.mozilla.org/b2g/?branch=master&device=hamachi&range=7&test=cold_load_time&app_list=contacts&app=contacts&gaia_rev=7a32f4ce7f922ed1&gecko_rev=06e9a27a6fcc&plot=median

Phone Link: https://datazilla.mozilla.org/b2g/?branch=master&device=flame&range=7&test=cold_load_time&app_list=phone&app=phone&gaia_rev=b597b86274ab109d&gecko_rev=2991fb290e14&plot=median

Backout Bot:
Gaia: d813860f012e653df96f7b1a02dd28f962f46630
Gecko: 192089:ac6960197eb6bdd5501a7ddd23dcda6976a8a9e0
Test: Contacts  Median: 700 Mean: 709 Std Dev: 36
Test: Phone Median: 428 Mean: 434 Std Dev: 21

Gaia: 36306be1b1b41a9bc973a24b86271a8c8ae38162 
Gecko: 192236:06e9a27a6fcc5e665588fa4f670d141fd9bac694
Test: Contacts  Median: 753 Mean: 758 Std Dev: 28
Test: Phone Median: 504 Mean: 507 Std Dev: 37

Probably a Gecko regression.

Comment 1

5 years ago
For contacts, datazilla is showing a gecko regression:

Bad: 9c43461f16ba
Good: 06e9a27a6fcc
Using gaia:  7a32f4ce7f922ed1

For phone:
Good: 945e47bb2af5
Bad: 2991fb290e14
Keywords: regression


5 years ago
Assignee: nobody → jhylands
Mason - Is this bad enough of a perf regression to block? If so, please nominate it for the first applicable release this should block.
Flags: needinfo?(mchang)
This would block 2.1 I believe (current master), since it happened on July 3.

Blocker criteria is listed here: https://wiki.mozilla.org/FirefoxOS/Performance/Triage

50ms is on the bottom end of optional blockers. I should probably investigate this a little more before we make that decision.
Flags: needinfo?(mchang)
Alright, I'll put this on the nom queue then.
blocking-b2g: --- → 2.1?
After bisecting, this appears to be a gecko regression caused by this commit:

commit b8114d206e5e2a3067c92550d73c55402d1909d8
Author: Jeff Muizelaar <jmuizelaar@mozilla.com>
Date:   Tue Jul 1 17:20:07 2014 +0200

    Bug 1026998. Initialize size CanvasRenderingContext2D. r=bas

    This fixes an unitiailized read found by valgrind.

Jeff, any ideas on why this commit would cause a 50 ms launch regression in only the contacts and phone apps?
Flags: needinfo?(jmuizelaar)
Blocks: 1026998
Component: Gaia → Canvas: 2D
Product: Firefox OS → Core
Version: unspecified → Trunk
Random guess: Jeff's patch for bug 1026998 initialises the uninitialised
fields thusly

-  : mForceSoftware(false), mZero(false), mOpaque(false), mResetLayer(true)
+  : mForceSoftware(false)
+  // these are the default values from the Canvas spec
+  , mWidth(300), mHeight(150)

and I wonder if doing so somehow causes a bitmap-y style object to be
allocated and initialised at the 300x150 size?

My initial proposed fix set them both to zero and that seemed to
also stop Valgrind complaining, FWIW.
Summary: [Gaia] ~50ms Phone and Contacts Launch Regression → [Gecko] ~50ms Phone and Contacts Launch Regression

Comment 7

5 years ago
Jon can you try setting those members to 0 to see if that fixes the regression?
Flags: needinfo?(jmuizelaar) → needinfo?(jhylands)
Jeff, sorry, if you can attach a small patch that I can apply, I can re-run the performance test.
Flags: needinfo?(jhylands) → needinfo?(jmuizelaar)

Comment 9

5 years ago
Created attachment 8453306 [details] [diff] [review]
Initialize to 0,0

Here's a patch
Flags: needinfo?(jhylands)
I'm assigning this bug to Jeff, but I will test his patch to see if that fixes the regression.
Assignee: jhylands → jmuizelaar
Flags: needinfo?(jhylands)
Jeff, that patch appears to fix the regression. Please submit the patch however you normally do, and close the bug once the patch lands.

Comment 12

5 years ago
Comment on attachment 8453306 [details] [diff] [review]
Initialize to 0,0

Review of attachment 8453306 [details] [diff] [review]:

It looks like initializing to 0, 0 gives us better behaviour. So let's do that instead.
Attachment #8453306 - Flags: review?(bas)
Comment on attachment 8453306 [details] [diff] [review]
Initialize to 0,0

Review of attachment 8453306 [details] [diff] [review]:

What about the canvas spec? It mentions that these sizes are specified in the Canvas spec.

Comment 14

5 years ago
Bas, is this a r-? If so please update its state. Please clarify if you need any other changes from Jeff.
Flags: needinfo?(bas)

Comment 15

5 years ago
(In reply to Bas Schouten (:bas.schouten) from comment #13)
> Comment on attachment 8453306 [details] [diff] [review]
> Initialize to 0,0
> Review of attachment 8453306 [details] [diff] [review]:
> -----------------------------------------------------------------
> What about the canvas spec? It mentions that these sizes are specified in
> the Canvas spec.

I assume we'll get those from a canvas element...
Comment on attachment 8453306 [details] [diff] [review]
Initialize to 0,0

Review of attachment 8453306 [details] [diff] [review]:

Do we? I mean.. there's a comment there? I assume someone put it there for a reason? I'll r+ this for now, but let's make sure we double-check.
Attachment #8453306 - Flags: review?(bas) → review+
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34


5 years ago
Flags: needinfo?(bas)


5 years ago
Flags: needinfo?(jmuizelaar)
blocking-b2g: 2.1? → ---
You need to log in before you can comment on or make changes to this bug.