Closed Bug 1002904 Opened 6 years ago Closed 5 years ago

calendar cold launch regression 100ms hamachi Apr23-Apr25

Categories

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

x86_64
Linux
defect

Tracking

(blocking-b2g:2.0+, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.0 S6 (18july)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: hub, Assigned: mmedeiros)

References

Details

(Keywords: perf, regression, Whiteboard: [c=regression p=1 s= u=2.0])

Attachments

(1 file)

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
Keywords: perf, regression
Sorry, The above was calendar.
Summary: cold launch regression 100ms hamachi Apr23-Apr25 → calendar cold launch regression 100ms hamachi Apr23-Apr25
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.
Whiteboard: [c= p= s= u=]
(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?
Flags: needinfo?(mmedeiros)
(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
Flags: needinfo?(mmedeiros)
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.
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.
blocking-b2g: --- → 2.0?
(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.
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.
Severity: normal → blocker
blocking-b2g: 2.0? → 2.0+
Priority: -- → P1
Whiteboard: [c= p= s= u=] → [c=progress p= s= u=2.0]
Assignee: nobody → hub
Status: NEW → ASSIGNED
Target Milestone: --- → 2.0 S2 (23may)
Whiteboard: [c=progress p= s= u=2.0] → [c=progress p=2 s= u=2.0]
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!
Flags: needinfo?(mmedeiros)
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.
Flags: needinfo?(mmedeiros)
Hi Miller,

Great thanks for the explanation. Marking bug 951593 as a dependent.
Depends on: 951593
Whiteboard: [c=progress p=2 s= u=2.0] → [c=progress p=1 s= u=2.0]
Whiteboard: [c=progress p=1 s= u=2.0] → [c=progress p=0 s= u=2.0]
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)
Whiteboard: [c=progress p=0 s= u=2.0] → [c=regression p= s= u=2.0]
Whiteboard: [c=regression p= s= u=2.0] → [c=regression p=1 s= u=2.0]
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)
Assignee: hub → dhuseby
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
This bug doesn't have any update for more than one month! What's the latest update? Thanks.
Flags: needinfo?(dhuseby)
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
Flags: needinfo?(dhuseby) → needinfo?(bugs)
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
Flags: needinfo?(bugs)
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.
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!
Flags: needinfo?(jlal)
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.
Flags: needinfo?(jlal)
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)
Depends on: 948856
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.
Flags: needinfo?(rnowmrch)
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).
Target Milestone: 2.0 S5 (4july) → 2.0 S6 (18july)
comment 20 answers the question.
thanks Miller!
Flags: needinfo?(rnowmrch)
Hey Dave and Garreth, has this been waiting on review and feedback since 7/8? Any chance we can get this looked asap? Thanks!
Flags: needinfo?(gaye)
Flags: needinfo?(dhuseby)
Reviewing now!
Flags: needinfo?(gaye)
Comment on attachment 8452773 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/21508

LGTM. Thanks Miller!
Attachment #8452773 - Flags: review?(gaye) → review+
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.
Attachment #8452773 - Flags: feedback?(dhuseby) → feedback+
Flags: needinfo?(dhuseby)
Assignee: dhuseby → mmedeiros
landed into master: https://github.com/mozilla-b2g/gaia/commit/ffc14b04e3b984342ba5dc17cebe10d2501e3093
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Flags: needinfo?(mmedeiros)
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.
Flags: needinfo?(mmedeiros)
You need to log in before you can comment on or make changes to this bug.