816483 - cache platform instantiations of webfonts and share them across pages

Status: RESOLVED FIXED

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

Description

[Jonathan Kew [:jfkthame]]

STR: load the NZHerald site (see URL field). Switch among pages by clicking the various sections (National / World / Business / Sport / etc). Note how there's often a momentary delay before the text of these section titles, and the headlines within the page, "pop" into view on the new page.

The NZHerald site is an example of a common pattern where a site uses the same webfonts on multiple pages. Currently, we rebuild the user font set on each page load, and this means we reload the webfonts used for the section-title bar and the article headlines. Although we load the files from cache, this still means that we're repeating the decompression and sanitization process and creating new platform font objects (asynchronously), even though the same fonts are being used on each page.

We can fix this by giving gfxUserFontSet a simple shared cache of the instantiated platform font entries; then, when a new page/font-set calls for the same fonts, we'll just re-use the existing platform objects instead of starting over with raw data from the wire. This means we'll be able to render the webfont text immediately on subsequent page loads, rather than waiting for an asynchronous font load to complete.

Result should be a *much* snappier user experience when navigating between pages that use the same webfonts.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: (WIP) cache user fonts and share them across pages

Experimental patch that caches the platform font entries associated with user fonts so that they can be re-used on subsequent pages instead of recreating fonts from the raw downloaded data. Currently works great on OS X and Windows, AFAIK. Behaves erratically on Linux/fontconfig because of how we use the CSS family name in the FcPattern that the font entry creates; this means FC's font matching fails if we share the font entry between CSS families with different names. And crashes on Android - haven't debugged this yet.

Comment 2

[Jonathan Kew [:jfkthame]]

Comment on attachment 686677 [details] [diff] [review]
(WIP) cache user fonts and share them across pages

Roc, is this the sort of thing you had in mind when we talked in Vancouver? It does make a big difference to the user experience on NZ Herald, for example.

Comment 3

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

Comment on attachment 686677 [details] [diff] [review]
(WIP) cache user fonts and share them across pages

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

::: gfx/thebes/gfxUserFontSet.cpp
@@ +838,5 @@
> +    // We do a simple linear search because we can't readily sort or hash
> +    // the entries, as we don't require an *exact* URI or principal match,
> +    // only that their Equals() methods consider them "the same".
> +    // There are unlikely to be a large number of entries at any one time,
> +    // so the linear search will not be too painful.

I'm not sure about that assumption.

nsURIHashKey can be used to hash on a URI, and principals have a hashValue that you should be able to use (see PrinicpalKey in nsScriptSecurityManager). So think using a hashtable here is quite doable.

@@ +899,5 @@
> +    for (uint32_t i = fonts.Length(); i > 0; ) {
> +        --i;
> +        if (fonts[i].mFontEntry == aFontEntry) {
> +            fonts.RemoveElementAt(i);
> +        }

If you use the hashtable then I guess we'll need to store a URI and a principal in the font entry so we can remove it efficiently.

Comment 4

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

Comment on attachment 686677 [details] [diff] [review]
(WIP) cache user fonts and share them across pages

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

Forgot to add --- the basic approach looks great!

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> Comment on attachment 686677 [details] [diff] [review]
> (WIP) cache user fonts and share them across pages
> 
> Review of attachment 686677 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxUserFontSet.cpp
> @@ +838,5 @@
> > +    // We do a simple linear search because we can't readily sort or hash
> > +    // the entries, as we don't require an *exact* URI or principal match,
> > +    // only that their Equals() methods consider them "the same".
> > +    // There are unlikely to be a large number of entries at any one time,
> > +    // so the linear search will not be too painful.
> 
> I'm not sure about that assumption.

I tried instrumenting the code to track and report the number of user-font entries present, and during normal browsing didn't see more than a handful at any one time, except when I deliberately set out to push the number up by loading webfont test/example pages such as http://fonts.philip.html5.org/ and http://opentype.info/demo/webfontdemo.html, which enabled me to push the number up closer to 100. But that was quite atypical - and still performed well.

> nsURIHashKey can be used to hash on a URI, 

I was concerned that URIs we want to treat as the same (i.e. Equals() will return true) might not necessarily have identical spec strings, and so wouldn't return the same hash key. According to http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIURI.idl#159, Equals() for URIs is not a strict string-equality test, yet nsURIHashKey just uses HashString to generate its value (see http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsURIHashKey.h#42). Or is this not an issue?

> and principals have a hashValue
> that you should be able to use (see PrinicpalKey in
> nsScriptSecurityManager). So think using a hashtable here is quite doable.

It's certainly doable (modulo the concern that equivalent entries might be stored more than once because of returning different hash values, as above - though this wouldn't break anything, it'd just reduce the effectiveness slightly). I'm not entirely sure it's worth it for the number of entries involved, though.

> 
> @@ +899,5 @@
> > +    for (uint32_t i = fonts.Length(); i > 0; ) {
> > +        --i;
> > +        if (fonts[i].mFontEntry == aFontEntry) {
> > +            fonts.RemoveElementAt(i);
> > +        }
> 
> If you use the hashtable then I guess we'll need to store a URI and a
> principal in the font entry so we can remove it efficiently.

We already have the URI in the gfxUserFontData record attached to the platform font entry; I guess we could add the principal as well.

Comment 6

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

(In reply to Jonathan Kew (:jfkthame) from comment #5)
> I tried instrumenting the code to track and report the number of user-font
> entries present, and during normal browsing didn't see more than a handful
> at any one time, except when I deliberately set out to push the number up by
> loading webfont test/example pages such as http://fonts.philip.html5.org/
> and http://opentype.info/demo/webfontdemo.html, which enabled me to push the
> number up closer to 100. But that was quite atypical - and still performed
> well.

I guess I'm paranoid about people deliberately creating large numbers of small fonts for some reason. If there's a straightforward way to avoid pathological cases which can be triggered by authors, I'm for it :-).

> > nsURIHashKey can be used to hash on a URI, 
> 
> I was concerned that URIs we want to treat as the same (i.e. Equals() will
> return true) might not necessarily have identical spec strings, and so
> wouldn't return the same hash key. According to
> http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIURI.
> idl#159, Equals() for URIs is not a strict string-equality test, yet
> nsURIHashKey just uses HashString to generate its value (see
> http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/
> nsURIHashKey.h#42). Or is this not an issue?

Cases where equal URIs map to different keys mean that the cache would be less efficient than necessary, but I doubt this is going to be a problem in practice --- I'd expect authors to use the same URI strings pretty much all of the time. They're just going to be copying and pasting URIs around, or generating them programmatically in some standard way.

Over to you. If you decide not to have a hashtable here, I'm OK with that.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: cache instantiated user fonts and share them across pages that use the same resources

Updated WIP patch; this now works on OS X, Windows and Android, just need to figure out what to do with the fontconfig side on Linux. Added telemetry that we can use to monitor how large the collection of cached user fonts becomes, to see whether it's worth the added complexity of a hashtable; my current guess is that the count will be typically be low enough (single-digit or low double-digit) that the simple approach here is sufficient.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: cache instantiated user fonts and share them across pages that use the same resources

In the end, I've switched this over to use a hashtable after all, primarily because instrumenting the Linux backend indicated that font entry lifetimes are not as easy to manage and predict there, and there's a tendency for the user-font entries to stay alive for a longer period. This in turn means that the number of cached user-font instances may get rather larger. Using the hashtable means this won't significantly degrade lookup performance.

Comment 9

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

Comment on attachment 690188 [details] [diff] [review]
cache instantiated user fonts and share them across pages that use the same resources

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

The rest looks good.

::: gfx/thebes/gfxUserFontSet.h
@@ +16,5 @@
>  #include "nsIFile.h"
>  #include "nsISupportsImpl.h"
>  #include "nsIScriptError.h"
> +#include "nsURIHashKey.h"
> +#include "nsScriptSecurityManager.h"

Why do you need this #include?

@@ +307,5 @@
> +                                            PrincipalKey::HashKey(aKey->mPrincipal),
> +                                            ( aKey->mFontEntry->mItalic |
> +                                             (aKey->mFontEntry->mWeight << 1) |
> +                                             (aKey->mFontEntry->mStretch << 10) ) ^
> +                                             aKey->mFontEntry->mLanguageOverride);

Shouldn't you hash mFeatureSettings too?

Comment 10

[Jonathan Kew [:jfkthame]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> > +#include "nsScriptSecurityManager.h"
> 
> Why do you need this #include?

That's where PrincipalKey::HashKey lives.

> Shouldn't you hash mFeatureSettings too?

I skipped it because it didn't have a handy hash function to call, and I figured that just means we may get an occasional extra hash collision, but it'll be pretty rare for it to matter. But ok, it's easy enough to include it in the hash; might as well do it right. I'll post an update.

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: cache instantiated user fonts and share them across pages that use the same resources

Updated to include feature settings in the hash, by just applying HashBytes to the array contents.

Comment 12

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

(In reply to Jonathan Kew (:jfkthame) from comment #10)
> That's where PrincipalKey::HashKey lives.

Why don't you just call nsIPrincipal::GetHashValue directly?

Comment 13

[Jonathan Kew [:jfkthame]]

Won't that require me to declare a local variable and call nsIPrincipal::GetHashValue separately, outside the call to HashGeneric? It just seemed neater to use PrincipalKey::HashKey as it already encapsulates that for me, but in the end it's doing the exact same thing. So I can avoid the extra #include and do it the other way if you prefer.

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: cache instantiated user fonts and share them across pages that use the same resources

Like this?

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: cache instantiated user fonts and share them across pages that use the same resources

One more thing while we're here - by fixing up the declaration of mOriginPrincipal in gfxFontFaceSrc, we can then dispense with a redundant QI in nsFontFaceLoader.

Comment 16

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

Comment on attachment 690229 [details] [diff] [review]
cache instantiated user fonts and share them across pages that use the same resources

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

Excellent!

Comment 17

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31b16bc75897

Hope that improves your newspaper-reading experience. :)

Comment 18

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

I can't wait for this to land on central :-)

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/31b16bc75897

Comment 20

[Sebastian Zartner [:sebo]]

Could it be that this feature doesn't consider the browser cache setting or reloading a page via Ctrl+F5?
This may be problematic for web developers, which are now not able anymore to see the requests to these webfonts in subsequent page loads.

Sebastian