[Gecko] ~50ms Phone and Contacts Launch Regression

RESOLVED FIXED in mozilla34

Status

()

Core
Canvas: 2D
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mchang, Assigned: jrmuizel)

Tracking

({perf, regression})

Trunk
mozilla34
ARM
Gonk (Firefox OS)
perf, regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

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

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

For phone:
Good: 945e47bb2af5
Bad: 2991fb290e14

Updated

3 years ago
Keywords: regression
(Reporter)

Updated

3 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)

Updated

3 years ago
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

 CanvasRenderingContext2D::CanvasRenderingContext2D()
-  : 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
(Assignee)

Comment 7

3 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)
(Assignee)

Comment 9

3 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.
Status: NEW → ASSIGNED
(Assignee)

Comment 12

3 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

3 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)
(Assignee)

Comment 15

3 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+
(Assignee)

Comment 17

3 years ago
http://dxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLCanvasElement.cpp#151
https://hg.mozilla.org/mozilla-central/rev/4ce3fc24d552
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34

Updated

3 years ago
Flags: needinfo?(bas)
(Assignee)

Updated

3 years ago
Flags: needinfo?(jmuizelaar)

Updated

3 years ago
blocking-b2g: 2.1? → ---
You need to log in before you can comment on or make changes to this bug.