1002904 - calendar cold launch regression 100ms hamachi Apr23-Apr25

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Calendar, defect, P1)

Description

[Hubert Figuiere [:hub]]

https://datazilla.mozilla.org/b2g/?branch=master&device=hamachi&range=30&test=cold_load_time&app_list=calendar&app=calendar&gaia_rev=56f79456db5dc3ca&gecko_rev=992ddc6bd0a9&plot=median


Apr 23
Gaia 0a4d2dea25a7162ee43db3a0db817798b70e7521
Gecko 35737ab92f9e
1172ms

Apr 25
Gaia 56f79456db5dc3ca010a56d09e1e8cc15a2408db
Gecko 992ddc6bd0a9
1269ms

Comment 1

[Hubert Figuiere [:hub]]

Sorry, The above was calendar.

Comment 2

[Miller Medeiros [:millermedeiros]]

yes, I was aware that this was going to happen. I did some performance tests to decide between custom fonts and SVG (for the vector icons) and decided to use the custom font during the visual refresh. (see: https://bugzilla.mozilla.org/show_bug.cgi?id=972871#c8)

My hope is that the systems front-end team will find a solution to improve the performance before we ship v2.0 (https://bugzilla.mozilla.org/show_bug.cgi?id=951593), otherwise we can replace the custom font with bitmaps if needed - not really the best option since we need to change the colors dynamically and have to support different screen resolutions.

PS: even with the performance regression we are still faster than v1.4.

Comment 3

[Jason Smith [:jsmith]]

(In reply to Miller Medeiros [:millermedeiros] from comment #2)
> PS: even with the performance regression we are still faster than v1.4.

How? Can you clarify this a bit more?

Comment 4

[Miller Medeiros [:millermedeiros]]

(In reply to Jason Smith [:jsmith] from comment #3)
> (In reply to Miller Medeiros [:millermedeiros] from comment #2)
> > PS: even with the performance regression we are still faster than v1.4.
> 
> How? Can you clarify this a bit more?

something changed between 2014-04-28 23:06:41 (Gaia Revision: cadddcac2b8ce162
Gecko Revision: b681a6daea3b) and 2014-04-29 01:06:55 (Gaia Revision: 725a23802708eb70
Gecko Revision: d7c07694f339) that affected the cold_load_time of all apps.

see: https://datazilla.mozilla.org/b2g/?branch=master&device=hamachi&range=30&test=cold_load_time&app_list=calendar&app=calendar&gaia_rev=725a23802708eb70&gecko_rev=d7c07694f339&plot=median

Comment 5

[Hubert Figuiere [:hub]]

But that fix is irrelevant. It is a different regression that was fixed - bring it back to earlier performance levels. We are not trying to get performance by side effect.

Comment 6

[Jason Smith [:jsmith]]

Right - agreed with Hub's comments. We've got performance criteria that we must hit for cold launch, so any regressions that cause us to miss that goal that are greater than 50 ms need to be fixed.

Comment 7

[Miller Medeiros [:millermedeiros]]

(In reply to Jason Smith [:jsmith] from comment #6)
> Right - agreed with Hub's comments. We've got performance criteria that we
> must hit for cold launch, so any regressions that cause us to miss that goal
> that are greater than 50 ms need to be fixed.

this will be fixed before the 2.0 release. The vector icons will speed up the development process and I'm hoping that the systems front-end team will find a solution for the regression (see Bug 951593). If we revert the vector icon patch we will lose a lot of work that was already done for the calendar visual refresh and that relies on those icons (we will need to rebase at least 5 branches). Doing the changes after we finish the visual refresh will be way simpler (in case we need to replace the custom font with bitmaps).

best case scenario would be for the Gecko team to find a way to avoid the delay introduced by @font-face so we don't need to touch the calendar code.

Comment 8

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

In that case, leaving it as (In reply to Hubert Figuiere [:hub] from comment #5)
> But that fix is irrelevant. It is a different regression that was fixed -
> bring it back to earlier performance levels. We are not trying to get
> performance by side effect.

I think that's probably a topic for wider discussion. If a global fix brings us under the acceptance mark, then that implies some degree of extra individual margin there. Otherwise, we can't buy time in order to do functional improvements.

I do think leaving the bug open as a blocker makes sense (re: the nom) so that we don't forget to address this again later.

Comment 9

[Mason Chang [Inactive] [:mchang]]

Hi Miller,

Sorry just getting up to speed on this bug. From comment 7, do we just need to fix bug 951593 to get this regression resolved or are there other bugs related to fixing this issue? Thanks!

Comment 10

[Miller Medeiros [:millermedeiros]]

Hi Mason,

if we fix Bug 951593 we will be able to move the icon font into the system (which will solve the regression). The regression is caused because @font-face loads the icon font dynamically and it blocks the page rendering until it finishes (which takes ~100ms on hamachi), system fonts doesn't affect the app launch time performance.

Another way to fix this bug is to replace the icon fonts with images, but I wouldn't recommend doing that since on the calendar app we would need to export some icons in 8 different colors and for multiple resolutions, which is not very easy to maintain.

Comment 11

[Mason Chang [Inactive] [:mchang]]

Hi Miller,

Great thanks for the explanation. Marking bug 951593 as a dependent.

Comment 12

[Kai-Chih Hu [:khu]]

This bug doesn't have any update for more than one month! What's the latest update? Thanks.

Comment 13

[Dave Huseby [:huseby]]

Jet,  I spoke with Vivien today and the solution for this regression is to land Bug 951593 to get the fonts into the system font set.  I left a ni? for you and :jfkthame in that bug asking if we can land those patches now.

This is a 2.0 blocker that we'd like to get resolved asap.  Thanks!

-dave

Comment 14

[Jet Villegas (inactive)]

Please see Jonathan's comments re: bug 1030829 and report back on the performance improvements from that patch. See also:
https://bugzilla.mozilla.org/show_bug.cgi?id=951593#c77

Comment 15

[Dave Huseby [:huseby]]

I've got an ni? out to :arnou in But 951593.  As soon as that work gets done this bug should be resolved as the fonts will be in the system hidden fonts directory and will no longer require being loaded at app launch time.

Comment 16

[Dave Huseby [:huseby]]

James,  I'm trying to get a 100ms regression in the calendar launch time resolved.  It turns out that the regression was caused by loading of the icon fonts during launch and we're trying to get the icon font into moztt so that it is loaded by the system instead of app.  In Bug 951593, :arnau is willing to update his patch to include the icons needed by the calendar app.  Can you find somebody working on calendar to work with :arnau to get this resolved ASAP.  This is a 2.0 blocker.

Thanks!

Comment 17

[Miller Medeiros [:millermedeiros]]

I added a comment explaining how to export the calendar icons (https://bugzilla.mozilla.org/show_bug.cgi?id=951593#c83), so I'm clearing the ni? since James is not actively working on calendar these days and I introduced the icon font to the calendar app.

Comment 18

[Miller Medeiros [:millermedeiros]]

Arnau is working on Bug 948856 (settings app) so as soon as he lands his patch we will be able to reuse CSS from the shared folder. So I'm marking it as a blocker. (his patch includes the calendar app icons, we just need to replace the icons.css file on the calendar app and link the shared/style/calendar.css as well)

Comment 19

[Dave Huseby [:huseby]]

Why is Bug 948856 no longer a blocker?  This regression is caused by loading icon fonts at app launch time.  We need to get the icon font into the system fonts.

Comment 20

[Miller Medeiros [:millermedeiros]]

Dave, Bug 948856 is about the settings app, calendar app only cares about the shared fonts, which is addressed by Bug 1035670.

I'm going to provide a patch for the calendar app later today, just fixing a few layout issues. It wasn't a simple replace because the base style for `.gaia-icon` and `.gaia-icon:before` is not the same as we had before and we also used a different grid size and base font-size, so we had a few conflicts (which I already fixed).

Comment 21

[Miller Medeiros [:millermedeiros]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/21508

Comment 22

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

comment 20 answers the question.
thanks Miller!

Comment 23

[Candice Serran (:cserran)]

Hey Dave and Garreth, has this been waiting on review and feedback since 7/8? Any chance we can get this looked asap? Thanks!

Comment 24

[Gareth Aye [:gaye] (back from PTO)]

Reviewing now!

Comment 25

[Gareth Aye [:gaye] (back from PTO)]

Comment on attachment 8452773 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/21508

LGTM. Thanks Miller!

Comment 26

[Dave Huseby [:huseby]]

Comment on attachment 8452773 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/21508

This appears to put the calendar icon fonts into the system fonts.  That fixes this regression.

Comment 27

[Miller Medeiros [:millermedeiros]]

landed into master: https://github.com/mozilla-b2g/gaia/commit/ffc14b04e3b984342ba5dc17cebe10d2501e3093

Comment 28

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/7bfb8969cc2c8d9dff1426848a499c13ac848cc7

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Reverted from v2.0 for build bustage.
v2.0: https://github.com/mozilla-b2g/gaia/commit/aa4f795b81c6147d67c4f06009e166debcf8856e

https://tbpl.mozilla.org/php/getParsedLog.php?id=43947031&tree=Mozilla-Aurora

Comment 30

[Miller Medeiros [:millermedeiros]]

we need to uplift the commit https://github.com/mozilla-b2g/gaia/commit/cb5d4664bd377c263c0e40d6410ae0a9e4207422 as well, that's why it failed. The shared icon font is not on 2.0.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/b67e4848a6808df16c8eb08c57386fe83de6fa67