1035244 - [Gecko] ~50ms Phone and Contacts Launch Regression

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect, P1)

Description

[Mason Chang [Inactive] [:mchang]]

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

[Mason Chang [Inactive] [:mchang]]

For contacts, datazilla is showing a gecko regression:

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

For phone:
Good: 945e47bb2af5
Bad: 2991fb290e14

Comment 2

[Jason Smith [:jsmith]]

Mason - Is this bad enough of a perf regression to block? If so, please nominate it for the first applicable release this should block.

Comment 3

[Jon Hylands [:jhylands]]

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.

Comment 4

[Jason Smith [:jsmith]]

Alright, I'll put this on the nom queue then.

Comment 5

[Jon Hylands [:jhylands]]

After bisecting, this appears to be a gecko regression caused by this commit:

commit b8114d206e5e2a3067c92550d73c55402d1909d8
Author: Jeff Muizelaar <[e-mail]>
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?

Comment 6

[Julian Seward [:jseward]]

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.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Jon can you try setting those members to 0 to see if that fixes the regression?

Comment 8

[Jon Hylands [:jhylands]]

Jeff, sorry, if you can attach a small patch that I can apply, I can re-run the performance test.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Attachment: Initialize to 0,0

Here's a patch

Comment 10

[Jon Hylands [:jhylands]]

I'm assigning this bug to Jeff, but I will test his patch to see if that fixes the regression.

Comment 11

[Jon Hylands [: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

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 13

[Bas Schouten (:bas.schouten)]

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

[Mike Lee [:mlee]]

Bas, is this a r-? If so please update its state. Please clarify if you need any other changes from Jeff.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

(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 16

[Bas Schouten (:bas.schouten)]

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.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

http://dxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLCanvasElement.cpp#151

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4ce3fc24d552