1036394 - [Settings] - 60ms launch time regression

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Performance, defect, P2)

Description

[Jon Hylands [:jhylands]]

Early this morning (July 9), a 60ms launch time regression on the Settings app showed up in Datazilla: https://datazilla.mozilla.org/b2g/?branch=master&device=flame&range=7&test=cold_load_time&app_list=settings&app=settings&gaia_rev=ba9da60d7b2ae227&gecko_rev=dfcc4c3b6f50&plot=median

I will bisect and find the offending commit.

Comment 1

[Jon Hylands [:jhylands]]

So it looks like this commit (in gaia) caused the regression:

commit 5db347ac8bf38ea5399d4f99e83b8bce1227d24f
Merge: b19616b 818d8fa
Author: Arnau <[e-mail]>
Date:   Wed Jul 9 08:35:26 2014 +0200

    Merge pull request #19083 from rnowm/948856

    Bug 948856 - [Settings] Update setting icons to use Icon Fonts r=arthurcc

Arnau, can you comment here on this commit?

Comment 2

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

Ni: jfkthame and roc, as they worked in bug 1030829,
Is there a way to test if both fonts:
/system/fonts/hidden/Gaia-icons.ttf
/shared/style/icons.css
are matching?

I'm not sure if we are getting the performance improvement from the font in system.
Thanks!

Comment 3

[Jonathan Kew [:jfkthame]]

Looking in /shared/style/icons.css, it's clear you won't be getting the system font, as @font-face declaration there doesn't exactly match the instructions from bug 951593 comment 77.

See https://github.com/mozilla-b2g/gaia/pull/21637 for a PR that should fix this.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: [PR] correct the gaia-icons font URI

Comment 5

[Wilson Page [:wilsonpage]]

(In reply to Jonathan Kew (:jfkthame) from comment #4)
> Created attachment 8454607 [details] [review]
> [PR] correct the gaia-icons font URI

Can you explain why when we use .woff fonts we should use 'data:font/woff;base64,' prefix, but when we use .ttf fonts we use 'font/opentype;base64,' prefix?

Comment 6

[Wilson Page [:wilsonpage]]

I suggest we alter this patch to use the new gaia-icons [1] repo before landing so that it doesn't need doing again later. This is so that building-block web-components and apps themselves can all depend on a single icon resource.

[1] http://github.com/gaia-components/gaia-icons

Comment 7

[Wilson Page [:wilsonpage]]

arnau: Do you agree with comment 6?

Comment 8

[Wilson Page [:wilsonpage]]

I've submitted a pull-request to moztt repo which updates gaia-icons to v0.2.0. This version includes 167 glyphs ready to be used across Gaia. As soon as this lands we can update the settings to use the official gaia-icons repo (at v0.2.0) and all *should* be well :)

I'm using .woff format as it is a lot smaller in size than the .ttf font, but can change the patch back to use .ttf if there are issues with this. I just thought it would encourage usage if it looked like a smaller asset. Plus AFAIK .woff is supposed to be the standard for web-fonts.

Comment 9

[Wilson Page [:wilsonpage]]

The pull-request: https://github.com/mozilla-b2g/moztt/pull/51

Comment 10

[Jonathan Kew [:jfkthame]]

(In reply to Wilson Page [:wilsonpage] from comment #8)
> I'm using .woff format as it is a lot smaller in size than the .ttf font,
> but can change the patch back to use .ttf if there are issues with this.

I'd recommend doing some performance tests to compare the results with .ttf vs .woff. Note that although the .woff file is a smaller asset, FreeType has to decompress it into RAM in order to use it, while the .ttf file can be accessed directly with mmap(). So the .woff version will have an added RAM footprint, and perhaps an initialization cost for the decompression. I don't know which will be better overall for the devices involved...

Comment 11

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

I'm running B2GPerf with several options. I'll share my results in a bit.

Comment 12

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

All right, I have run several times 'b2gperf --delay=10 Settings', please let me know if I'm doing something wrong or this is not the best way to run performance test locally.

So the median values I've got for cold_load_time are around 820, and does not care if the font in system is not matching with the one in shared.
I have even tried removing the font in system and I get similar results.

Now the cool part:
If I remove the font in system/fonts/hidden and I move it to system/fonts, I don't need the matching thing and I can remove the font-face in shared/style/icons.css
The value I'm getting now is 755.
I know this goes against the idea of having the font in a hidden directory.
But I guess that's the performance value we want to reach.
Although we have recently discovered when generating the fonts using grunt webfont (https://github.com/sapegin/grunt-webfont) as you can see here: https://github.com/gaia-components/gaia-icons/blob/master/style.css (line 34 and below) we are not using PUA characters anymore but a simple name we use inside 'content'.
No Idea if that fact could make us use just the font in system.

Please let me know what do you think, as I believe we should try to get that 755, and if that's not possible let's go back to use bitmaps.

Comment 13

[Wilson Page [:wilsonpage]]

(In reply to Arnau March  [:arnau] from comment #12)

> Now the cool part:
> If I remove the font in system/fonts/hidden and I move it to system/fonts, I
> don't need the matching thing and I can remove the font-face in
> shared/style/icons.css
> The value I'm getting now is 755.
> I know this goes against the idea of having the font in a hidden directory.
> But I guess that's the performance value we want to reach.
> Although we have recently discovered when generating the fonts using grunt
> webfont (https://github.com/sapegin/grunt-webfont) as you can see here:
> https://github.com/gaia-components/gaia-icons/blob/master/style.css (line 34
> and below) we are not using PUA characters anymore but a simple name we use
> inside 'content'.
> No Idea if that fact could make us use just the font in system.

I have a couple of concerns here:

1. Components need to be able to work both inside and outside of Gaia; for style-guides, live examples pages and third-party implementations (eg. Brick). If we are depending on something as critical as icons being available in the current environment, like everything webby, we should have a fall-back it they aren't there.

2. Apps/components are unable to depends on a specific version of the icon-font, and are instead handed whatever is in system at the time. This may / may not map to glyphs used the app/components's CSS. Although I do understand that using versions other than the one in system will void the perf boost, so this is not advised anyway.

Perhaps jonathan can shed some light on why we don't seem to be getting the perf boost with the icon-font in the hidden directory...?

Comment 14

[Jonathan Kew [:jfkthame]]

(In reply to Arnau March  [:arnau] from comment #12)
> All right, I have run several times 'b2gperf --delay=10 Settings', please
> let me know if I'm doing something wrong or this is not the best way to run
> performance test locally.
> 
> So the median values I've got for cold_load_time are around 820, and does
> not care if the font in system is not matching with the one in shared.

Are you *sure* you've got the correct data URI in the CSS to match the font installed in /system/fonts/hidden? The font version needs to be identical (so that the base64 data matches), and the prefix ("data:font/opentype;base64," or "data:font/woff;base64,") must be *exactly* right, otherwise it won't be considered a match and we'll fall back to decoding the embedded font from the CSS. That would account for the result you're describing.

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: add some logging to see what fonts are actually being used

:arnau, if you add this patch to gecko, you should be able to check (in the logcat output) whether the gaia-icons font is being found in cache or not - it should be, if the data: URI is correct. If it's being found, and yet performance is not improved, then we have a problem; if it's not being found, it's time to re-check that URI in the CSS. (Compare it with the one that'll be logged during startup.)

Comment 16

[Wilson Page [:wilsonpage]]

Testing instructions (via arnau):

easy_install pip
pip install b2gperf
pip install marionette_client
pip install setuptools
adb forward tcp:2828 tcp:2828
b2gperf --delay=10 Settings
and you will get something like that:
2014-07-14 13:17:08,886 B2GPerfRunner INFO    | Results for Settings, cold_load_time: median:755, mean:757, std: 21, max:853, min:725, all:771,750,757,746,749,750,725,766,769,760,736,759,757,737,770,764,754,759,753,742,745,752,774,757,773,747,764,753,731,853

Comment 17

[Wilson Page [:wilsonpage]]

BEFORE ICON-FONT ADDED

cold_load_time: median:372, mean:375, std: 23, max:431, min:334, all:370,334,354,413,380,348,385,382,412,366,394,394,373,431,377,381,366,371,347,349,366,350,365,383,345,397,361,346,396,421

ICON-FONT-ADDED

cold_load_time: median:427, mean:447, std: 68, max:699, min:397, all:442,443,438,408,412,434,658,512,534,699,433,413,429,417,413,402,408,435,424,434,397,398,435,448,425,419,411,417,421,469

MIME-TYPE CHANGED

cold_load_time: median:419, mean:419, std: 10, max:441, min:395, all:429,425,415,441,422,420,395,415,407,417,423,418,415,438,431,436,429,417,415,422,441,423,419,408,411,406,425,405,409,421

-----

This indicates that something isn't working quite right yet. I will try to apply jfkthame's Gecko patch and see if that indicates stuff is working as expected.

Comment 18

[Wilson Page [:wilsonpage]]

I managed to get jfkthame's Gecko logging patch applied. It outputs the following:

ICON-FONT ADDED

> I/Gecko   (17150): Using cached font for family gaia-icons

MIME-TYPE CHANGED

> I/Gecko   (18291): Sync-loading data-URI font for family gaia-icons

Comment 19

[Wilson Page [:wilsonpage]]

CORRECTION (comment 18)

ICON-FONT ADDED

  I/Gecko   (18291): Sync-loading data-URI font for family gaia-icons

MIME-TYPE CHANGED

  I/Gecko   (17150): Using cached font for family gaia-icons

Comment 20

[Wilson Page [:wilsonpage]]

(In reply to Arnau March  [:arnau] from comment #12)
> If I remove the font in system/fonts/hidden and I move it to system/fonts, I
> don't need the matching thing and I can remove the font-face in
> shared/style/icons.css
> The value I'm getting now is 755.

I can't get the icons to render with this approach. How is the font-family inferred? Did you check that the icons are rendered in Settings app before running `b2gperf`?

Comment 21

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

Wilson,
to do that, I manually copied the font in system/fonts, and removed font-face declaration:
1. download the ttf from moztt repo.
2. adb push (you local path)/Gaia-icons.ttf /system/fonts/
3. remove in shared/style/icons.css
    @font-face {
      font-family: gaia-icons;
      src: url(data:application/x-font-ttf;charset=utf-8;base64,AAEAAAALA...
    }

And yes, I saw the icons rendered in settings app, even after doing a reset-gaia.

Comment 22

[Jonathan Kew [:jfkthame]]

(In reply to Wilson Page [:wilsonpage] from comment #17)
> BEFORE ICON-FONT ADDED
> 
> cold_load_time: median:372, mean:375, std: 23, max:431, min:334,
> all:370,334,354,413,380,348,385,382,412,366,394,394,373,431,377,381,366,371,
> 347,349,366,350,365,383,345,397,361,346,396,421
> 
> ICON-FONT-ADDED
> 
> cold_load_time: median:427, mean:447, std: 68, max:699, min:397,
> all:442,443,438,408,412,434,658,512,534,699,433,413,429,417,413,402,408,435,
> 424,434,397,398,435,448,425,419,411,417,421,469
> 
> MIME-TYPE CHANGED
> 
> cold_load_time: median:419, mean:419, std: 10, max:441, min:395,
> all:429,425,415,441,422,420,395,415,407,417,423,418,415,438,431,436,429,417,
> 415,422,441,423,419,408,411,406,425,405,409,421
> 
> -----
> 
> This indicates that something isn't working quite right yet. I will try to
> apply jfkthame's Gecko patch and see if that indicates stuff is working as
> expected.

Actually, I think it indicates things are working - but not reaching the numbers we'd really like to see. Once the data: URI for the icon font is fixed, you're seeing 28ms taken off the mean load time (and a great reduction in variability, too, fwiw), which I think (though I'm no statistician) looks like a significant improvement.

I haven't been able to run b2gperf locally, but I have done some further instrumentation of a build (on Peak) and seen a similar result: with the correct data: URI, so that the pre-cached hidden font is used instead of loading from the base64 data in icons.css, the time spent in gfxUserFontSet::FindFontEntry when gaia-icons is loaded drops from about 20-30ms to around 2-3ms. This is part of the app's initial reflow, so it will translate directly into a cold-start load time improvement of nearly 20-30ms. Which is very comparable to the b2gperf figures you're getting (on what device?).

The trouble is, we really wanted to gain back 70ms, not just 30.

My suspicion at this point is that a large part of the remaining 40ms is the overhead of linking to an additional CSS file that has to be loaded and parsed during loading of the app. I'm not sure offhand where/how to measure this, though; I don't know anything about that whole process.

Out of curiosity, I did instrument the method Loader::ParseSheet in layout/style/Loader.cpp, and determined that on my Peak, it's taking about 10-14ms to parse the stylesheet from /shared/style/icons.css. That's *after* the sheet has been located and loaded, which must also be taking some time. So it's clear that the addition of the

  <link rel="stylesheet" type="text/css" href="shared/style/icons.css">

line in index.html is costing a significant amount of time (somewhere well over 10ms; I don't know how much additional time the actual file-load takes) even before we actually use the icon font at all.

Would it be worth trying to defer the loading of this CSS until after the app is loaded, so that it doesn't contribute directly to launch time?

Comment 23

[Wilson Page [:wilsonpage]]

I have new figures,

PNGS

cold_load_time: median:425, mean:424, std: 11, max:441, min:407, all:438,435,407,424,408,426,441,434,422,413

ICON-FONT

cold_load_time: median:473, mean:472, std: 18, max:508, min:442, all:470,464,489,478,508,447,476,478,442,468

ICON-FONT LAZY-LOADED

cold_load_time: median:423, mean:430, std: 30, max:498, min:393, all:429,407,441,393,418,438,498,413,465,401

Comment 24

[Jonathan Kew [:jfkthame]]

OK, so the lazy-loaded icon font does avoid the launch-time hit. How's it look, UX-wise; does it make for a disturbing visual effect, or is it quick enough to go unnoticed? (Could you attach your patch for us to experiment with, please?)

Overall, all these figures look higher than you had before (in comment 17); do we know why that is? Unrelated changes to the app?

Comment 25

[Wilson Page [:wilsonpage]]

ICON-FONT LAZY-LOADED TTF FROM FILE

cold_load_time: median:442, mean:439, std: 15, max:460, min:407, all:460,441,443,422,457,407,444,436,450,436

Comment 26

[Jonathan Kew [:jfkthame]]

Note that when I tried the lazy-load approach on my Peak device, it doesn't display correctly: the icons end up too far from the text of the Settings items, and when they appear (after a momentary delay), the symbols at the right margin also shift rightwards. Something isn't coping well with the dynamic change to the layout. :(

Comment 27

[Wilson Page [:wilsonpage]]

I've rebased my test branches against 'master' so perf figures shouldn't be compared with earlier tests.

PNGS

median:550, mean:549, std: 18, max:583, min:512
all:566,548,553,512,553,566,534,583,535,546

ICON-FONT

median:872, mean:812, std: 109, max:907, min:638
all:885,654,897,907,891,894,859,638,650,849

ICON-FONT (SYSTEM)

median:540, mean:547, std: 24, max:602, min:520
all:572,530,563,530,550,602,552,523,520,529

Comment 28

[Wilson Page [:wilsonpage]]

jfkthame: From my testing so far, it seems like the only performant solution is to place the icon-font in /system/fonts/ and remove the usage of @font-face. What was the main argument against this approach? Are we able to adapt things to make this viable? Do you have any alternative ideas?

Comment 29

[Jonathan Kew [:jfkthame]]

(In reply to Wilson Page [:wilsonpage] from comment #27)
> I've rebased my test branches against 'master' so perf figures shouldn't be
> compared with earlier tests.
> 
> PNGS
> 
> median:550, mean:549, std: 18, max:583, min:512
> all:566,548,553,512,553,566,534,583,535,546
> 
> ICON-FONT
> 
> median:872, mean:812, std: 109, max:907, min:638
> all:885,654,897,907,891,894,859,638,650,849

So what used to be a 60-70ms regression, reduced to 40ms once we fixed the CSS properly, has now turned into 260ms?! I think we need to know why this has become 6x more expensive since your earlier tests. What changed?

(In reply to Wilson Page [:wilsonpage] from comment #28)
> jfkthame: From my testing so far, it seems like the only performant solution
> is to place the icon-font in /system/fonts/ and remove the usage of
> @font-face. What was the main argument against this approach?

The problem with this is that it makes these icons appear directly as part of the "web platform" that FirefoxOS exposes to app and web page authors. Which amounts to an invitation to use them as a trivial way to prettify their apps/pages. And that's bad because it makes their pages dependent on the FxOS environment instead of being built on top of the standardized, interoperable web platform.

> Are we able to
> adapt things to make this viable? Do you have any alternative ideas?

We need to figure out why loading the font has suddenly become so much more expensive than previously.

Comment 30

[Wilson Page [:wilsonpage]]

[flame] Using `make test-perf` observing the `moz-app-visually-complete`

PNG SPRITE: 2525ms
EMPTY DATA-URI (about:blank): 2570ms
UNUSED DATA-URI: 2619ms
ICON-FONT (base64 encoded inc. system/fonts/hidden): 2600ms
ICON-FONT (base64 encoded excl. system/fonts/hidden): 2569ms, 2558ms, 2534ms
ICON-FONT (system): 2546ms
ICON-FONT (from file): 2737ms
NO ICONS (png or font): 2497ms

The numbers are not really making much sense to me at the moment. We're still getting lots of variation, it's hard to draw any conclusion. I'm beginning to think a side by side visual perf measurement might be the only sure way to know if we're actually regressing.

Comment 31

[Jonathan Kew [:jfkthame]]

I'd say there's something fishy about this pair (unless you possibly got them switched around? or maybe the first one was just a particularly unlucky run?):

  ICON-FONT (base64 encoded inc. system/fonts/hidden): 2600ms
  ICON-FONT (base64 encoded excl. system/fonts/hidden): 2569ms, 2558ms, 2534ms

as there's no reasonable way that adding the hidden font should be slower - and indeed the b2gperf measurements earlier were showing a small but pretty consistent improvement from it.

Aside from that, the only thing this really confirms is that loading the font from file does have a significant impact. That's expected, but given that loading from file would be beneficial from a memory PoV, I'd like to explore whether we can do a Gecko hack that improves its performance. I'll try to come up with an experimental patch and post here for you to test, so we know whether the idea is worth pursuing.

Comment 32

[Jonathan Kew [:jfkthame]]

FWIW, I'd also say that looking at

https://datazilla.mozilla.org/b2g/?branch=master&device=flame&range=30&test=cold_load_time&app_list=settings&app=settings&gaia_rev=139d3af6b2341f52&gecko_rev=6e36501f2ddd&plot=avg

the 60ms regression originally reported here is pretty minor compared to others that have happened since!

And the figures there, in the range of 300-700ms, bear no resemblance at all to the 2500 range that you're getting. It's pretty clear this is measuring something different. Which raises the question of which measurement we should actually be caring about. If we compare

https://datazilla.mozilla.org/b2g/?branch=master&device=flame&range=30&test=startup_%3E_moz-app-visually-complete&app_list=settings&app=settings&gaia_rev=139d3af6b2341f52&gecko_rev=6e36501f2ddd&plot=avg

then it's not at all clear that cold_load_time "regressions" are the most important thing here.

Comment 33

[Jonathan Kew [:jfkthame]]

Attachment: experimental patch for eager font loading

:wilsonpage - here is a Gecko patch (just an experimental hack, really) to see if we can get the separate .ttf font to load more promptly. Build Gecko with this patch, then change the @font-face rule to use src:url(Gaia-icons.ttf); and add the prefixed descriptor -moz-loading-behavior:eager; to the rule, and see how this compares to the existing 'icon-font from file' performance. I don't really know what result to expect: the same work should end up getting done, but doing the font-load more eagerly may help the overall timing.

Comment 34

[Jon Hylands [:jhylands]]

(In reply to Jonathan Kew (:jfkthame) from comment #32)
> FWIW, I'd also say that looking at
> 
> https://datazilla.mozilla.org/b2g/
> ?branch=master&device=flame&range=30&test=cold_load_time&app_list=settings&ap
> p=settings&gaia_rev=139d3af6b2341f52&gecko_rev=6e36501f2ddd&plot=avg
> 
> the 60ms regression originally reported here is pretty minor compared to
> others that have happened since!

Yes, but we try and track all regressions, and open individual bugs for each one.

> And the figures there, in the range of 300-700ms, bear no resemblance at all
> to the 2500 range that you're getting. It's pretty clear this is measuring
> something different. Which raises the question of which measurement we
> should actually be caring about. If we compare
> 
> https://datazilla.mozilla.org/b2g/
> ?branch=master&device=flame&range=30&test=startup_%3E_moz-app-visually-
> complete&app_list=settings&app=settings&gaia_rev=139d3af6b2341f52&gecko_rev=6
> e36501f2ddd&plot=avg
> 
> then it's not at all clear that cold_load_time "regressions" are the most
> important thing here.

Note that cold load time is just a subset of the actual application load time, so you can't compare it to anything useful except itself. We have a new set of metrics coming on line very shortly (like next week hopefully) that correspond much more closely to the user's perceived experience, and we'll be basing regression decisions on these new metrics. The second link above is an example of one of those new metrics (startup_>_moz-app-visually-complete).

Comment 35

[Wilson Page [:wilsonpage]]

I'm getting 'No data found' message for all of the above Datazilla links.

Comment 36

[Jon Hylands [:jhylands]]

Sometimes Datazilla does that - if you change to the 30 day view, and then back to 7, it usually starts working again.

Comment 37

[Wilson Page [:wilsonpage]]

I've run this test again, with 20 runs:

ICON-FONT (base64 encoded inc. system/fonts/hidden): 2576ms

Seems to suggest, like past tests, that the system/font/hidden caching and matching makes minimal difference.

Comment 38

[Wilson Page [:wilsonpage]]

ICON-FONT (-moz-font-loading: eager): 2810ms (20 runs)

Comment 39

[Wilson Page [:wilsonpage]]

I realised I didn't reboot before the last test run, so here is the result after `adb reboot`:

ICON-FONT (-moz-font-loading: eager): 2751ms (20 runs)

Comment 40

[Jonathan Kew [:jfkthame]]

(In reply to Wilson Page [:wilsonpage] from comment #37)
> I've run this test again, with 20 runs:
> 
> ICON-FONT (base64 encoded inc. system/fonts/hidden): 2576ms
> 
> Seems to suggest, like past tests, that the system/font/hidden caching and
> matching makes minimal difference.

Direct instrumentation of the actual code suggests that it saves around 20-30ms, but that's liable to be lost in the noise of these measurements.

(In reply to Wilson Page [:wilsonpage] from comment #39)
> I realised I didn't reboot before the last test run, so here is the result
> after `adb reboot`:
> 
> ICON-FONT (-moz-font-loading: eager): 2751ms (20 runs)

This was after patching and rebuilding gecko?

OK, so that's not useful. I'm going to try a bit more local instrumentation to see whether there's anything else we can do here.

Comment 41

[Wilson Page [:wilsonpage]]

For more information I've recorded B2G profiles:

NO ICONS (png or font): https://people.mozilla.org/~bgirard/cleopatra/#report=e2b4d01cb1d0378f1570a56a680ef3b0b3255705
ICON-FONT (embedded w/ system/fonts/hidden): https://people.mozilla.org/~bgirard/cleopatra/#report=e91c478deacd6f0eed3e1c49db4a00a94a0db880

Comment 42

[Mason Chang [Inactive] [:mchang]]

Hi Wilson, for the second profile, the ICON-FONT, I only see the Homescreen app, not the Settings app. Can you please get another profile with the settings app?

From your results in comment 30, were you rebooting between tests? Any consistency in the other app events? Thanks!

Comment 43

[Jonathan Kew [:jfkthame]]

(In reply to Jon Hylands [:jhylands] from comment #34)
> Note that cold load time is just a subset of the actual application load
> time, so you can't compare it to anything useful except itself. 

Understood. My concern is that we could "fix" a cold-load-time regression by moving work to a later stage in the launch process, so that it no longer shows up as part of that particular measurement; yet it might be worse for the overall user experience.

Based on the tests and experiments so far, I don't think the "regression" as originally reported here should block us from moving forward with the use of the icon font, if this provides the functionality needed by the design/product teams. We'll continue to look for ways to optimize performance, obviously, but I don't see evidence that moving from PNGs to the icon font (data: URI version) is substantially hurting user experience.

Comment 44

[Wilson Page [:wilsonpage]]

ICON-FONT (embedded w/ system/fonts/hidden): https://people.mozilla.org/~bgirard/cleopatra/#report=458a8a7b515bbccff050892a7dcc78802592e44d

Comment 45

[Jonathan Kew [:jfkthame]]

One thing we do if you load the icon font from a file or from a data: URI, but *not* if it's installed in /system/fonts/hidden, is to validate the font data (because bad or malicious font data delivered via @font-face can be a security issue).

For the Gaia-icons 1.00 TTF font (at about 55K), this process takes somewhere around 5ms on my Peak. So it's not too bad, but it's a few ms we can definitely trim by using the hidden font.

Comment 46

[:Eli Perelman]

(In reply to Jonathan Kew (:jfkthame) from comment #43)
> (In reply to Jon Hylands [:jhylands] from comment #34)
> > Note that cold load time is just a subset of the actual application load
> > time, so you can't compare it to anything useful except itself. 
> 
> Understood. My concern is that we could "fix" a cold-load-time regression by
> moving work to a later stage in the launch process, so that it no longer
> shows up as part of that particular measurement; yet it might be worse for
> the overall user experience.

While this was possible in the cold load measurements done by b2gperf, in the new events that Jon is speaking about, this will no longer be possible or acceptable.

Comment 47

[Mason Chang [Inactive] [:mchang]]

Nexus 4 w/ icon fonts - https://people.mozilla.org/~bgirard/cleopatra/#report=2ed121e3a8f6bff17374bd0615e65ecda045fdb7

Nexus 4 w/o icon fonts - https://people.mozilla.org/~bgirard/cleopatra/#report=d7f2b289db16e2f397aec52ac83d637ac186879a

Comment 48

[Mason Chang [Inactive] [:mchang]]

Attachment: Display List Dump w/ Icon Fonts

Comment 49

[Mason Chang [Inactive] [:mchang]]

Attachment: Display List Dump w/o Icon Fonts

Comment 50

[Mason Chang [Inactive] [:mchang]]

Attachment: Layer Tree w/ Icon

Comment 51

[Mason Chang [Inactive] [:mchang]]

Attachment: Display Lists w/ Opacity Layer Alone

Hi Roc, I'm trying to Wilson Page looking at what's going on with this start up regression. I guess we have a couple of questions:

1) From this attached display list dump, any idea why we have the ScrollBarFrame(0xb34d4598) alone in it's own layer? 
2) Any idea on how expensive painting a unicode character is? Something like this from a display list dump: Text p=0xb2bf8310 f=0xb3340dc8(Text(0)"\ue6a2") bounds(870,85110,1860,1860) visible(870,85110,1860,1860) componentAlpha(870,85110,1860,1860) clip(0,76920,23040,14580) layer=0xb34d8400

Thanks!

Comment 52

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Mason Chang [:mchang] from comment #51)
> 1) From this attached display list dump, any idea why we have the
> ScrollBarFrame(0xb34d4598) alone in it's own layer? 

You mean why are there blank lines around it? I don't know.

Scrollbars are layerized on B2G to reduce the amount of repainting we have to do when they change. So I'm not surprised it has its own layer(s).

> 2) Any idea on how expensive painting a unicode character is? Something like
> this from a display list dump: Text p=0xb2bf8310
> f=0xb3340dc8(Text(0)"\ue6a2") bounds(870,85110,1860,1860)
> visible(870,85110,1860,1860) componentAlpha(870,85110,1860,1860)
> clip(0,76920,23040,14580) layer=0xb34d8400

That should be relatively cheap, no more expensive than drawing other text. Certainly, paint time should not depend on how the font was loaded. I thought we expected a small startup regression moving from PNGs to icon fonts, since font glyph rasterization is an inherently more complex process than decoding a PNG. I also thought this bug was about getting the performance of using a data: URL font as close as possible to the performance of using a system font. That performance difference should definitely not affect painting.

My preferred solution to variable-resolution application icons has always been to include icons as SVG assets and have the app installer pre-rasterize those SVGs to PNGs at install time, guaranteeing that performance of those icons is exactly as good as PNGs. Instead we seem to be burning up a huge amount of energy on an inferior solution.

Comment 53

[Jonathan Kew [:jfkthame]]

Pre-rasterized PNGs is certainly an option, although it doesn't allow icons to be dynamically styled with a "theme color" in the way font glyphs can. That may be one of the attractions of the icon-font option?

Comment 54

[Wilson Page [:wilsonpage]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #52)
> My preferred solution to variable-resolution application icons has always
> been to include icons as SVG assets and have the app installer pre-rasterize
> those SVGs to PNGs at install time, guaranteeing that performance of those
> icons is exactly as good as PNGs. Instead we seem to be burning up a huge
> amount of energy on an inferior solution.

Yeah theme-ability, reuse and maintainability are the main reasons we are pushing for icon-fonts here.

It seems as though everything is normal/expected at the layout/painting level and our bottleneck remains at the loading/css-parse time.

jfkthame: Any more tricks up your sleeve?

Comment 55

[Wilson Page [:wilsonpage]]

Also NI Vivien here to help make a decision about where we should go, we have the following options:

1. Accept ~50ms regression is a fair cost to benefits of theme-able, scalable icons right now. And have some eyes on the platform layer to optimizer this if possible.

2. Put gaia-icons.ttf in system/fonts/ and get good performance with the risk that third parties may use them. If there is a way we could restrict availability to installed/privileged apps only to prevent them leaking to the web, this could be fair compromise.

3. Fallback to using PNGs and forfeit 'theme-ability'.

Comment 56

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

Any chance we could use the ttf in system/fonts only for certified apps and give a fall back to the base64 in shared folder in case is not?

Comment 57

[Jonathan Kew [:jfkthame]]

(In reply to Wilson Page [:wilsonpage] from comment #54)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #52)
> > My preferred solution to variable-resolution application icons has always
> > been to include icons as SVG assets and have the app installer pre-rasterize
> > those SVGs to PNGs at install time, guaranteeing that performance of those
> > icons is exactly as good as PNGs. Instead we seem to be burning up a huge
> > amount of energy on an inferior solution.
> 
> Yeah theme-ability, reuse and maintainability are the main reasons we are
> pushing for icon-fonts here.
> 
> It seems as though everything is normal/expected at the layout/painting
> level and our bottleneck remains at the loading/css-parse time.
> 
> jfkthame: Any more tricks up your sleeve?

Not really. I suppose we could reduce the cost of loading the extra CSS somewhat if we replaced the full data URL with something more compact, such as a hash, but then we'd lose the built-in fallback on non-FxOS devices/environments. So I don't see that as a good way forward - nor is it clear exactly how much we'd gain.

(In reply to Arnau March  [:arnau] from comment #56)
> Any chance we could use the ttf in system/fonts only for certified apps and
> give a fall back to the base64 in shared folder in case is not?

This would require Gecko plumbing to support certified-app-only system fonts, which :roc opposed pretty firmly in bug 1008458.

Comment 58

[jachen]

Adding Gordon to the conversation in addition to the NI for Vivien.

Comment 59

[Gordon Brander :gordonb]

Sounds good. As Eli and others point out, we don't want to "push our peas around on the plate" by moving work that should be done during app startup to later (which would juice numbers without improving experience). Everyone involved has a deep read on the tradeoffs and potential solutions, so I don't have much to add.

Comment 60

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

Comment on attachment 8454607 [details] [review]
[PR] correct the gaia-icons font URI

Even making fonts match doesn't look like we are fixing the regression.
Clearing review 'till we find a new solution.

Comment 61

[Jon Hylands [:jhylands]]

Jonathan, since you're the one actually working on this bug, I'm going to assign it to you.

Comment 62

[Jonathan Kew [:jfkthame]]

Actually, I don't have any current work here, beyond the experimental patches above - but I think Michael has some work going on that may help; hence re-assigning. :)

Comment 63

[Michael Wu [:mwu]]

Attachment: Use CRC32 to look up hidden fonts

The integration with gfxUserFontSet feels like a hack to me, but I'm not sure what the best thing to do here is. This does seem to work though, and the numbers seem comparable to having the font in /system/fonts. I've only been using the startup time overlay to check though.

Comment 64

[Michael Wu [:mwu]]

Comment on attachment 8465802 [details] [diff] [review]
Use CRC32 to look up hidden fonts

Review of attachment 8465802 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxUserFontSet.cpp
@@ +1086,5 @@
> +    if (!sUserCRCFonts) {
> +        return nullptr;
> +    }
> +
> +    nsCOMPtr<nsIIOService> io = mozilla::services::GetIOService();

IO service not actually directly used here - ignore this.

Comment 65

[Jonathan Kew [:jfkthame]]

Comment on attachment 8465802 [details] [diff] [review]
Use CRC32 to look up hidden fonts

Review of attachment 8465802 [details] [diff] [review]:
-----------------------------------------------------------------

This looks really promising. As I understand it, the idea here is to use CRC32 (and file length) as a "key" to determine whether the preloaded font will satisfy a given @font-face request. So the CSS only needs to include the resource URI pointing to the zip-file entry, rather than the entire font data. I guess that's OK, given that this applies only to fonts being loaded from within the app's own resources; we won't be potentially matching CRCs for any random @font-face resource on the web.

Regarding the actual code of the patch, I don't much like the fact that we end up with two separate hashtables in the UserFontCache here; I think I'd prefer to add crc32 and length fields to the existing hashtable's Key, with a couple of different lookup behaviors according to whether crc/length values are provided or not. But I admit I haven't fully thought this through...perhaps it would end up getting messy. I'm imagining that we'd pass the crc/length to UserFontCache::GetFont as additional parameters. If these are set (e.g. if length is non-zero?), it would construct a key that includes these (and ignores uri and principal) and try looking up that; if that fails, it would then do a normal lookup based on uri and principal, and ignoring crc.

One thing to note is that I do think we need to check the family name when looking up fonts by crc; parts of the gfx font code expect the familyname field of a gfxFontEntry to match the name of the family through which it's being used. So family name should be incorporated into the key being used. We should also be checking the various other attributes (feature settings, style properties, etc) that the existing cache code uses, as it's not valid to use a pre-cached font entry if the app's @font-face rule wants to override those descriptors.

Comment 66

[Michael Wu [:mwu]]

Attachment: Add api to access nsIZipEntry through nsIJARChannel

This seems about right to me, but I need to take this to try to check the new test.

Comment 67

[Michael Wu [:mwu]]

Attachment: Use CRC32 to look up hidden fonts, v2

The nsIJARChannel changes were taken out of this patch. The main change here is that we use the existing hashtable instead of having a separate dedicated one for CRC matched fonts. Feels a bit awkward, but it seems a little bit better overall.

I had to change the template test app I got from Wilson Page to make this work - the CSS specified a weight of 400 (normal), but the font is actually configured as 500.

There's some debugging logging I left in this patch so people can test it more easily. I'll remove them when things are ready to land.

Comment 68

[Michael Wu [:mwu]]

Attachment: Add api to access nsIZipEntry through nsIJARChannel, v2

This seems to work and test ok.

Comment 69

[Jonathan Kew [:jfkthame]]

Comment on attachment 8467293 [details] [diff] [review]
Use CRC32 to look up hidden fonts, v2

Review of attachment 8467293 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Yes, the use of the user font cache hashtable, with (in effect) two different kinds of key, is a little weird, but it's an implementation detail that's pretty well encapsulated, and the rest of the code doesn't need to worry about it.

A few small nits and suggestions below; r=me with these tidied up.

::: gfx/thebes/gfxUserFontSet.cpp
@@ +955,1 @@
>              return false;

braces around the if-statement's body, even if it's only one line

@@ +971,5 @@
>      }
>  
>      if (mPrivate != aKey->mPrivate) {
>          return false;
>      }

I think the mPrivate check should probably go inside the else-block above, so that it doesn't apply when matching JAR'd fonts by CRC. Otherwise, private-browsing windows will be unable to take advantage of preloaded fonts, and I don't see any need for that limitation.

@@ +1008,5 @@
>  
>      gfxUserFontData *data = aFontEntry->mUserFontData;
> +    if (data->mLength) {
> +        sUserFonts->PutEntry(Key(data->mCRC32, data->mLength, aFontEntry,
> +                                 data->mPrivate, aPersistence));

In principle, we could drop the aPersistence parameter from UserFontCache::CacheFont; it'll always be true here, when we're caching by CRC32, and always false otherwise, won't it?

I was going to suggest we go ahead and remove it, and just hard-code the kPersistent value here; however, I kinda like having it explicitly present at the call site in gfxFT2FontList. So how about adding an assertion here that aPersistence == kPersistent, to confirm we're using this code in the expected way?

And we shouldn't ever be caching private-browsing fonts by CRC32, that's only for preloaded system fonts. Might be worth an additional assertion here that data->mPrivate is false.

@@ +1020,5 @@
> +        } else {
> +            principal = data->mPrincipal;
> +        }
> +        sUserFonts->PutEntry(Key(data->mURI, principal, aFontEntry,
> +                                 data->mPrivate, aPersistence));

...and here, assert that aPersistence is kDiscardable.

@@ +1076,5 @@
>      }
>  
> +    nsCOMPtr<nsIChannel> chan;
> +    if (NS_FAILED(NS_NewChannel(getter_AddRefs(chan), aSrcURI)))
> +        return nullptr;

Gecko style calls for braces on each of these ifs.

@@ +1092,5 @@
> +        return nullptr;
> +
> +    uint32_t length;
> +    if (NS_FAILED(zipentry->GetRealSize(&length)))
> +        return nullptr;

Neither GetCRC32 nor GetRealSize can ever fail, AFAIK, so you could omit the NS_FAILED checks here to reduce the verbosity slightly.

Comment 70

[Michael Wu [:mwu]]

Attachment: Use CRC32 to look up hidden fonts, v3

Review comments addressed, debug logging disabled/removed.

Comment 71

[Jason Duell]

Comment on attachment 8467472 [details] [diff] [review]
Add api to access nsIZipEntry through nsIJARChannel, v2

Review of attachment 8467472 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libjar/nsJARChannel.cpp
@@ +903,5 @@
> +    if (NS_FAILED(rv))
> +        return rv;
> +
> +    if (!mJarFile)
> +        return NS_ERROR_NOT_IMPLEMENTED;

NOT_AVAILABLE might be more accurate?  I.e. it's not possible for some jarChannels.

Comment 72

[Michael Wu [:mwu]]

(In reply to Jason Duell (:jduell) from comment #71)
> NOT_AVAILABLE might be more accurate?  I.e. it's not possible for some
> jarChannels.

Done.

https://hg.mozilla.org/integration/b2g-inbound/rev/365edfca5712
https://hg.mozilla.org/integration/b2g-inbound/rev/5b36c2cc1d79

Comment 73

[Michael Wu [:mwu]]

Followup bustage fix - https://hg.mozilla.org/integration/b2g-inbound/rev/86702af059f4

Comment 74

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/mozilla-central/rev/365edfca5712
https://hg.mozilla.org/mozilla-central/rev/5b36c2cc1d79

Comment 75

[Wilson Page [:wilsonpage]]

The original bug isn't technically 'FIXED' yet as we haven't landed the patch to use the new gaia-icons.ttf, that will land as part of bug 1015297.

Comment 76

[Wilson Page [:wilsonpage]]

I'm going to re-open until bug 1015297 has landed and we have proven that the regression has gone.

Comment 77

[Tim Guan-tin Chien [:timdream] (please needinfo)]

leave-open per comment 76, remove when this bug is ready to be closed.

Comment 78

[Milan Sreckovic [:milan] (needinfo for best results)]

Bug 1015297 has landed!  Wilson, are you the one to test (and if appropriate, close) this bug, or is it Jon who's guilty of creating the bug? :)

Comment 79

[Wilson Page [:wilsonpage]]

(In reply to Milan Sreckovic [:milan] from comment #78)
> Bug 1015297 has landed!  Wilson, are you the one to test (and if
> appropriate, close) this bug, or is it Jon who's guilty of creating the bug?
> :)

To the best of our knowledge I think we can say that the gaia-icons font should now not have any startup penalty. Happy for this bug to be re-opened if this is proven otherwise.

Comment 80

[Dietrich Ayala (:dietrich)]

Triage reviewed, and marked blocking+ because perf regression.