1674228 - Firefox takes a long time to load emoji when emoji font is set to "Noto Color Emoji"

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect, P3)

Description

[Sumagna Das]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:82.0) Gecko/20100101 Firefox/82.0

Steps to reproduce:

I was using Instagram where i didn't like the emoji style so i switched the emoji style from "Twemoji Mozilla" to "Noto Color emoji". After switching the emoji picker in DMs took about a minute to load. This is the perf profile ->

Prerequisite:-

• Noto Color Emoji font
• Ubuntu

Steps to Reproduce:

1. Change font.name-list.emoji from "Twemoji Mozilla" to "Noto Color Emoji"
2. visit getemoji.com

Actual results:

It takes an unusual amount of time to load the emojis in the site hence it doesn't open very fast. The same happens with the emoji picker in instagram DM.

Expected results:

It should have opened within a few seconds or milliseconds.

Comment 1

[Sumagna Das]

This is the perf profile -> https://share.firefox.dev/3mAp5jW

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I can confirm (on Fedora, if it matters). https://share.firefox.dev/31WVlFM is me loading getemoji.com.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

I can try to poke at this.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

We're trying to get the same table over and over on the same font instance (or a couple ones). If I log calls to CopyFontTable I get:

CopyFontTable(0x7feed09c6bb0, 43424454)

But there is supposed to be a font table cache that avoids this problem.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(The table is CBDT fwiw)

Comment 6

[Emilio Cobos Álvarez (:emilio)]

So, I'm very confused about what I'm seeing, halp Jonathan.

So, HasFontTable effectively gets the table and checks it's not empty. So far so good.

AutoTable will do mBlob = aFontEntry->GetFontTable(aTag); and then it'll call hb_blob_destroy on the blob we just got, so it expects an addrefed blob.

AutoTable calls GetFontTable, which looks in a cache and if it doesn't hit the cache we go and grab a blob and return it. The problem is that the cache doesn't keep a reference to the blob (intentionally it seems, per this comment).

So as soon as AutoTable gets destroyed we unref the blob, and since the cache doesn't keep a reference to it we remove it from the cache... So the cache is effectively only being effective for empty tables.

So in terms of fixing we could:

• Override gfxFontconfigFontEntry::HasFontTable to do something faster.
• Tweak the font table cache to actually work when you copy a table.

The second could look like this (plus removing ManageHashEntry and such):

diff --git a/gfx/thebes/gfxFontEntry.cpp b/gfx/thebes/gfxFontEntry.cpp
index f776614603f1..8088f19fd744 100644
--- a/gfx/thebes/gfxFontEntry.cpp
+++ b/gfx/thebes/gfxFontEntry.cpp
@@ -487,8 +487,7 @@ hb_blob_t* gfxFontEntry::FontTableHashEntry::ShareTableAndGetBlob(
 
-  // Tell the FontTableBlobData to remove this hash entry when destroyed.
-  // The hashtable does not keep a strong reference.
-  mSharedBlobData->ManageHashEntry(aHashtable, GetKey());
-  return mBlob;
+  return hb_blob_reference(mBlob);
 }
 
 void gfxFontEntry::FontTableHashEntry::Clear() {

But what am I missing?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1674228 - Make gfxFontEntry table cache keep a strong reference to its blob. r=jfkthame

So as repeated calls to HasFontTable don't load a table over and over.

If this is too hard on memory we may want keep a cache specific to
HasFontTable or something instead, maybe? But I think this should be
fine, we probably end up using all tables we check for.

Comment 8

[Jonathan Kew [:jfkthame]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

Created attachment 9184638 [details]
Bug 1674228 - Make gfxFontEntry table cache keep a strong reference to its blob. r=jfkthame

So as repeated calls to HasFontTable don't load a table over and over.

If this is too hard on memory we may want keep a cache specific to
HasFontTable or something instead, maybe? But I think this should be
fine, we probably end up using all tables we check for.

IIRC, this table cache was set up so that multiple gfxFont instances of the same gfxFontEntry will share table data rather than each loading it separately; but the data will be released when all font instances using it are destroyed. We don't want the gfxFontEntry itself to keep ownership of the cached table data, though, because the font entry stays around for the entire life of the process.

So in this case, giving the gfxFontEntry table cache a strong reference means that as soon as we've checked for the (huge) color-bitmap table in Noto Color Emoji once, it'll stay in memory. That's unfortunate, as it might never be needed again.

I'd prefer, therefore, to avoid holding on to the table data here; instead, I think we should do something (probably based on FT_Sfnt_Table_Info) to check for the existence of a table without actually loading it. (Do you want to write this version, or should I take it? Either works for me.)

A much simpler fix for this specific case would be to just add a boolean flag in gfxFontEntry to cache the result in HasColorBitmapTable() instead of letting it call HasFontTable() every time. It's probably worth doing that, too, but I think we should also improve HasFontTable for the Freetype-based backends so that it never copies table data.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1674228 - Cache the result of the HasFontTable checks in gfxFontEntry::HasColorBitmapTable. r=emilio

Comment 10

[Jonathan Kew [:jfkthame]]

So here's the simple fix that I think we should take straight away; I think it's also worth optimizing HasFontTable for Freetype-based backends (i.e. both Linux and Android), but we can do that separately as a followup. The patch above should fix the really bad behavior here already.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1674228 - Speed up HasFontTable checks for freetype fonts. r=jfkthame

Comment 12

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jonathan Kew (:jfkthame) from comment #10)

So here's the simple fix that I think we should take straight away; I think it's also worth optimizing HasFontTable for Freetype-based backends (i.e. both Linux and Android), but we can do that separately as a followup. The patch above should fix the really bad behavior here already.

Hah, I was writing this :-)

Comment 13

[Jonathan Kew [:jfkthame]]

Cool, you've done the followup already too :) ... LGTM.

Comment 14

[Jonathan Kew [:jfkthame]]

By the way, I suspect the performance here regressed recently as a result of bug 1371386, which meant that we're much more careful about color- vs monochrome-font selection. Could you try the example on Firefox 81 (i.e. before that change) and see how the behavior compared? If it's a clear regression, that would strengthen the case for uplifting at least the simple patch here.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Yeah, I bisected this and indeed bug 1371386 is in the range (I stopped after a while).

Comment 16

[Jonathan Kew [:jfkthame]]

Comment on attachment 9184744 [details]
Bug 1674228 - Cache the result of the HasFontTable checks in gfxFontEntry::HasColorBitmapTable. r=emilio

Beta/Release Uplift Approval Request

• User impact if declined: Poor performance on Linux/Android when Noto Color Emoji is used (or other large color-bitmap fonts)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Trivial patch to cache the result a potentially-expensive font probe
• String changes made/needed:

Comment 17

[Jonathan Kew [:jfkthame]]

Just uplifting the smaller/simpler patch here (D95279) should be enough to address the bad perf regression, I believe.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea800bc80f16
Speed up HasFontTable checks for freetype fonts. r=jfkthame

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b6bb7278ef6
Cache the result of the HasFontTable checks in gfxFontEntry::HasColorBitmapTable. r=emilio

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c99eba6641e
Rename some functions to avoid a gcc false-positive warnings.

Comment 21

[ALWIN]

Attachment: gambar.png

emoji.json

Comment 22

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/ea800bc80f16
https://hg.mozilla.org/mozilla-central/rev/0b6bb7278ef6
https://hg.mozilla.org/mozilla-central/rev/4c99eba6641e

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1371386

Comment 24

[Pascal Chevrel:pascalc (PTO until Sept 2)]

Comment on attachment 9184744 [details]
Bug 1674228 - Cache the result of the HasFontTable checks in gfxFontEntry::HasColorBitmapTable. r=emilio

Low risk patch for a significant performance regression on Linux, uplift approved for 83 beta 8, thanks.

Comment 25

[Julien Cristau [:jcristau] (back August 18)]

https://hg.mozilla.org/releases/mozilla-beta/rev/45e3ecd73706

Comment 26

[Florin Strugariu [:Bebe]]

== Change summary for alert #27536 (as of Fri, 06 Nov 2020 06:06:13 GMT) ==

Improvements:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
7%	ebay-kleinanzeigen-search		android-hw-p2-8-0-android-aarch64-shippable	warm	448.84 -> 418.28
3%	cnn-ampstories	FirstVisualChange	android-hw-g5-7-0-arm7-api-16-shippable	live warm	1,245.12 -> 1,203.42
3%	cnn-ampstories	ContentfulSpeedIndex	android-hw-g5-7-0-arm7-api-16-shippable	live warm	1,245.12 -> 1,203.92
3%	cnn-ampstories	LastVisualChange	android-hw-g5-7-0-arm7-api-16-shippable	live warm	1,249.29 -> 1,208.08
3%	cnn-ampstories	SpeedIndex	android-hw-g5-7-0-arm7-api-16-shippable	live warm	1,245.12 -> 1,204.25
3%	cnn-ampstories	PerceptualSpeedIndex	android-hw-g5-7-0-arm7-api-16-shippable	live warm	1,245.92 -> 1,205.42

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=27536

Comment 27

[Florin Strugariu [:Bebe]]

== Change summary for alert #27541 (as of Fri, 06 Nov 2020 10:11:00 GMT) ==

Improvements:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
4%	Resident Memory		linux1804-64-shippable		656,606,079.01 -> 630,163,543.56
3%	Resident Memory		linux1804-64-shippable-qr		1,192,650,563.23 -> 1,151,781,434.68

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=27541