449356 - Refactor gfxPangoFontGroup for user fonts

Status: RESOLVED FIXED

(Core :: Graphics, defect, P2)

Description

[Karl Tomlinson (:karlt)]

Currently gfxPangoFontGroup font selection uses pango_itemize and that uses a
PangoFcFontMap to find the faces most closely matching the CSS properties.

Making the user font faces available to the pango-provided PangoFcFontMap
would require adding the faces to the current global FcConfig.  Switching
between multiple FcConfig's to avoid fonts being used outside the document (or
element) would be clumsy and likely duplicate much data.  Although it would be
possible to filter out foreign user font faces between the PangoFcFontMap and
pango_itemize within gfxPangoFontGroup, but keeping the faces other users of
fontconfig would require hacks to replace symbols in system libraries.

A better approach would be to refactor the gfxPangoFontGroup font selection.
Implementing a new PangoFcFontMap would be sufficient to enable user font face
selection, but it may be better to replace the pango_itemize functionality
currently used.

Using Pango shapers requires being able to construct a PangoFont from a face.
The best way to do this is to implement a PangoFcFont using a
cairo_scaled_font_t constructed from the face.

Comment 1

[Karl Tomlinson (:karlt)]

We need this for downloadable fonts, but completion by August 19 is improbable.

Comment 2

[Karl Tomlinson (:karlt)]

I considered a hacky shortcut to add user fonts: add them to the global
FcConfig before each call to a Pango function that might use fontconfig, and
remove them again each time.

It would be necessary to ensure that the caches in the PangoFcFontMap didn't
provide the user fonts to other callers, possibly through vigorous use of
pango_fc_font_map_cache_clear or through using a bogus family in the
pango_font_description as a namespace.

The only way to defeat PangoFcFontMap's coverage_hash would be to use a unique
filename for each user font (or use a separate PangoFcFontMap).

The need to defeat the PangoFcFontMap's caches suggests that we would be using
it in a way for which it was not intended, and that we should be implementing
our own font matching.

Comment 3

[Karl Tomlinson (:karlt)]

(In reply to comment #0)
> Using Pango shapers requires being able to construct a PangoFont from a face.
> The best way to do this is to implement a PangoFcFont using a
> cairo_scaled_font_t constructed from the face.

That has been done in bug 385263.

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: font selection in Mozilla WIP 0.1.2

This is a basic implementation of a Mozilla-specific PangoFcFontMap, that
makes it possible for us to do our own font selection.

It doesn't make any attempt cache information to optimize speed.  With this
patch, memory use (Tp RSS) drops from 108 to 82 MB (1:0.76), but the cost is
an increase in page load time (Tp) from 440 to 638 ms (0.69:1).  This shows the
potential for memory savings by choosing what to cache carefully.

Roughly 7 MB of the 26 MB memory savings come from removal of the gfxPangoFontCache (bug 428425 comment 5).

Comment 5

[Christopher Blizzard (:blizzard)]

Nice, Karl!  Looking forward to see if we can find a happy medium here.

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: font selection in Mozilla's PangoFcFontMap 0.4

This is a proof of concept patch.  It still needs a bit of tidying up.
Results from Talos on the try server:

            ref w/patch change
Ts (ms)    1435  1463     +2%
Tp (ms)     446   441     -1%
Tp_RSS (MB) 110    80    -27%

Reference log:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1223866852.1223870369.23733.gz
w/patch:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1223860252.1223863695.10237.gz

The Ts change may be due to gfxFontconfigUtils being initialized earlier,
reverting some of the savings from the patch in bug 404857 comment 13.  If so,
doing some of the initialization lazily should recover that.  (Some of it may
never be used.)

There is more happening in the Tp test than is indicated in the overall result
here.  The Tp test runs through 400 pages then re-runs through the same set of
400 pages another 9 times.  The reported result is an average of some sort
over all 10 runs of 400 pages (possibly with some values discarded or other magic thrown in).

Without the patch, the mean page load time for the first run is 584 ms, the
mean for the second run is 444 ms, and the mean over the remaining runs
is 460 ms.  The increase after the second run is a bit concerning but it is
what would be expected from bug 453200.

With this patch, the mean for the first run is 446 ms (-24%), the mean
for the second run is 442 ms (~0%), and the mean over the remaining runs
is 459 ms (~0%).

The time taken for zooming is most related (but not equivalent) to the time
for the first run, so IMO having a fast first run is more important than the
weight attributed to it in Talos Tp.

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to comment #6)
> The Ts change may be due to gfxFontconfigUtils being initialized earlier,
> reverting some of the savings from the patch in bug 404857 comment 13.  If so,
> doing some of the initialization lazily should recover that.  (Some of it may
> never be used.)

Yes, removing gfxFontconfigUtils::mAliasTable, which is not used (and the initialization of which this patch would have activated) drops Ts w/patch by about 100 ms to 1350-1360 ms (-5 to -6% wrt the reference without the patch).

> Without the patch, ...
> The increase after the second run is a bit concerning but it is
> what would be expected from bug 453200.

No, it's not bug 453200.  Fixing that reduces page load times consistently across all runs by 15 - 20 ms (Tp is -5 to -6%).

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: font selection in Mozilla's PangoFcFontMap 1.0

This includes removal of gfxFontconfigUtils::mAliasTable for startup time improvement, as well as tidying up and making things more robust.

It feels like jumping through a couple of hoops to use a PangoFcFontMap, but it seems worth it to avoid risks associated with replacing pango_itemize at this stage.

Comment 9

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

+    nsTArray<FontSetByLangEntry> mFontSets;

This usually contains one element, right? You probably should make it nsAutoTArray<FontSetByLangEntry,1>.

nsStringArray looks a lot worse than nsTArray<nsString>. I filed bug 461047 on that.

+    // mLangStrings owns the memory for keys to mBestLangSupportTable and
+    // mFontsByLang.  The language support and the fonts for each lang are in
+    // separate tables because mFontsByLang is expected to be used only when
+    // no default fonts support the language.  There would be a large number
+    // of fonts in entries for languages using Latin script but these do not
+    // need to be created because default fonts already support these
+    // languages.

As we discussed, let's combine the tables into one and just allow the font list to be missing from some entries.

Seems like gfxRefPtrWrap should go into XPCOM and be split into "refcounted object" and "custom-destructor object". How about nsRefObj/nsAutoObj? (The template parameter doesn't need to be a pointer, after all --- nsAutoObj would be useful for HDCs, for example).

+    class familyComparator {
+    class langComparator : familyComparator {

FamilyComparator, LangComparator

+        if (existingFamilies.Contains(family, familyComparator())) {
+            continue;
+        }

Doesn't this get a bit slow? Maybe we should make existingFamilies a hash table?

+            for (PRUint32 l; l < findLangMatch.Length(); ++l) {
+    for (PRUint32 l; l < findLangMatch.Length(); ++l) {

Don't call variables 'l', it's hard to distinguish from '1'

+            if(FcFontSetAdd(fontSet, font)) {

"if (" (3 occurrences)

+        while (!mFcFontSet) {
+            if (mHaveFallbackFonts)
+                return nsnull;
+
+            SortFallbackFonts();
+            mHaveFallbackFonts = PR_TRUE;
+        }

Why is this a 'while' and not an 'if'?

+    gfxRefPtrWrap<FcPattern> mSortPattern;
+    gfxRefPtrWrap<FcFontSet> mFcFontSet;
+    gfxRefPtrWrap<FcCharSet> mCharSet;
+    nsTArray<FontEntry> mFonts;
+    int mFcFontsTrimmed;
+    PRPackedBool mHaveFallbackFonts;

Comment these please.

+        if (mFcFontsTrimmed == mFcFontSet->nfont) {
+            // finished with mFcFontSet
+            mFcFontSet.Reset();
+            mFcFontsTrimmed = 0;

Why reset to 0? That just seems confusing.

+    const cairo_font_options_t *options =
+        gdk_screen_get_font_options(gdk_screen_get_default());

If we're not compiling with system cairo, this might be a problem.

Comment 10

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

One thing that would help is a big comment somewhere explaining what the overall strategy is.

Comment 11

[Karl Tomlinson (:karlt)]

(In reply to comment #9)
> +    const cairo_font_options_t *options =
> +        gdk_screen_get_font_options(gdk_screen_get_default());
> 
> If we're not compiling with system cairo, this might be a problem.

Good catch.  Filed bug 462798 on that.  (It is not new in the patch here.)

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: font selection in Mozilla's PangoFcFontMap 1.2

Addresses the issues in comment 9 and comment 10 (except for bug 462798).

Comment 13

[Karl Tomlinson (:karlt)]

Attachment: font selection in Mozilla's PangoFcFontMap 1.2.2

Makes changes to template specializations due to changes in bug 461087,
and corrects some errors that showed up in testing.

Comment 14

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

What errors?

Comment 15

[Karl Tomlinson (:karlt)]

Uninitialized variables:

--- a/gfx/thebes/src/gfxPangoFonts.cpp
+++ b/gfx/thebes/src/gfxPangoFonts.cpp
@@ -807,1 +807,1 @@ gfxFcPangoFontSet::SortPreferredFonts()
-            for (PRUint32 r; r < requiredLangs.Length(); ++r) {
+            for (PRUint32 r = 0; r < requiredLangs.Length(); ++r) {
@@ -826,1 +826,1 @@ gfxFcPangoFontSet::SortPreferredFonts()
-    for (PRUint32 r; r < requiredLangs.Length(); ++r) {
+    for (PRUint32 r = 0; r < requiredLangs.Length(); ++r) {

Wrong test:

--- a/gfx/thebes/src/gfxFontconfigUtils.h
+++ b/gfx/thebes/src/gfxFontconfigUtils.h
@@ -213,1 +213,1 @@ public:
-        PRBool IsKeyInitialized() { return mKey.IsVoid(); }
+        PRBool IsKeyInitialized() { return !mKey.IsVoid(); }

The aForce parameter to UpdateFontListInternal forces a check for changes now, but that doesn't guarantee invalidation of caches (unless a change is detected).

--- a/gfx/thebes/src/gfxFontconfigUtils.cpp
+++ b/gfx/thebes/src/gfxFontconfigUtils.cpp
@@ -923,6 +923,7 @@ gfxFontconfigUtils::GetLangSupportEntry(
             // entry->mSupport needs to be recalculated, but this is an
             // indication that the set of installed fonts has changed, so
             // update all caches.
+            mLastConfig = NULL; // invalidates caches
             UpdateFontListInternal(PR_TRUE);
             return GetLangSupportEntry(aLang, aWithFonts);
         }

Comment 16

[Karl Tomlinson (:karlt)]

Attachment: avoid static object

Our leak test tools don't seem to like (non-POD?) static objects (even with function scope).

Comment 17

[Karl Tomlinson (:karlt)]

Attachment: avoid static object v2

Putting the smaller object after the larger hash tables.

Comment 18

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

Comment on attachment 346569 [details] [diff] [review]
avoid static object v2

Sorry, I should have caught that. Thanks. Get back in the landing queue.

Comment 19

[Karl Tomlinson (:karlt)]

Attachment: including all changes in one patch

Including all changes in one patch.  The only new change here is removal of the "static" keyword from the typedef (which some compilers didn't seem to mind/notice).

--- a/gfx/thebes/src/gfxPangoFonts.cpp
+++ b/gfx/thebes/src/gfxPangoFonts.cpp
@@ -601,8 +601,6 @@ private:
     PRPackedBool mHaveFallbackFonts;
 };
-
-static

 typedef FcBool (*FcPatternRemoveFunction)(FcPattern *p, const char *object,
                                           int id);

Comment 20

[Karl Tomlinson (:karlt)]

Landed this but had to back out due to:

REFTEST TEST-PASS | file:///builds/slave/trunk_linux-7/build/layout/reftests/bidi/bidi-003.html | 
REFTEST TEST-KNOWN-FAIL | file:///builds/slave/trunk_linux-7/build/layout/reftests/bidi/bidi-004.html | 
REFTEST TEST-KNOWN-FAIL | file:///builds/slave/trunk_linux-7/build/layout/reftests/bidi/bidi-004-j.html | 
REFTEST TEST-PASS | file:///builds/slave/trunk_linux-7/build/layout/reftests/bidi/bidi-005.html | 
../../objdir/dist/bin/run-mozilla.sh: line 131:  6372 Segmentation fault      "$prog" ${1+"$@"}
program finished with exit code 139
TinderboxPrint: reftest<br/>176/0/10

Haven't yet succeeded in reproducing.

Comment 21

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

Try it under Valgrind.

Comment 22

[Karl Tomlinson (:karlt)]

(In reply to comment #21)
Thanks.  Valgrind found the uninitialized variables in comment 15, but it hasn't reported a problem with the bidi test, with debug and optimized builds.

Comment 23

[Karl Tomlinson (:karlt)]

Attachment: what eventually landed

Thanks, Vlad for your help.

Two more uninitialized variables:

--- a/gfx/thebes/src/gfxPangoFonts.cpp
+++ b/gfx/thebes/src/gfxPangoFonts.cpp
@@ -872,7 +872,7 @@ gfxFcPangoFontSet::SortPreferredFonts()
     if (truncateMarker != NULL && fontSet) {
         nsAutoRef<FcFontSet> truncatedSet(FcFontSetCreate());

-        for (int f; f < fontSet->nfont; ++f) {
+        for (int f = 0; f < fontSet->nfont; ++f) {
             FcPattern *font = fontSet->fonts[f];
             if (font == truncateMarker)
                 break;
@@ -1127,7 +1127,7 @@ gfx_pango_fontset_get_font(PangoFontset
 {
     gfxPangoFontset *self = GFX_PANGO_FONTSET(fontset);

-    PangoFont *result;
+    PangoFont *result = NULL;

     FcPattern *baseFontPattern = NULL;
     if (self->mBaseFont) {

Comment 24

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/d062597e5b3d
http://hg.mozilla.org/mozilla-central/rev/b39e3a7974f2

Comment 25

[Karl Tomlinson (:karlt)]

Attachment: 1.9.1 memory use

(Wanted somewhere to host this image, as I haven't worked out how to prevent blogger from scaling images for me.)