Refactor gfxPangoFontGroup for user fonts

RESOLVED FIXED in mozilla1.9.1b2

Status

()

Core
Graphics
P2
normal
RESOLVED FIXED
9 years ago
3 years ago

People

(Reporter: karlt, Assigned: karlt)

Tracking

(Depends on: 1 bug, {footprint, perf})

Trunk
mozilla1.9.1b2
x86
Linux
footprint, perf
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 8 obsolete attachments)

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.
(Assignee)

Comment 1

9 years ago
We need this for downloadable fonts, but completion by August 19 is improbable.
Flags: wanted1.9.1?
Priority: -- → P1
Target Milestone: --- → mozilla1.9.1
(Assignee)

Updated

9 years ago
Blocks: 428425
(Assignee)

Updated

9 years ago
Blocks: 385263
Flags: wanted1.9.1? → wanted1.9.1+
Priority: P1 → P2
(Assignee)

Comment 2

9 years ago
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.
(Assignee)

Updated

9 years ago
Blocks: 397860
(Assignee)

Updated

9 years ago
No longer blocks: 385263
Depends on: 385263
(Assignee)

Updated

9 years ago
No longer blocks: 397860, 403618
(Assignee)

Updated

9 years ago
Depends on: 404857
(Assignee)

Updated

9 years ago
Depends on: 456545
(Assignee)

Comment 3

9 years ago
(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.
(Assignee)

Comment 4

9 years ago
Created attachment 341049 [details] [diff] [review]
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).
Nice, Karl!  Looking forward to see if we can find a happy medium here.

Updated

9 years ago
Blocks: 458169

Updated

9 years ago
No longer blocks: 441473
(Assignee)

Comment 6

9 years ago
Created attachment 342841 [details] [diff] [review]
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.
Attachment #341049 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Keywords: footprint, perf
(Assignee)

Comment 7

9 years ago
(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%).
(Assignee)

Comment 8

9 years ago
Created attachment 344063 [details] [diff] [review]
 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.
Attachment #342841 - Attachment is obsolete: true
Attachment #344063 - Flags: review?(roc)
+    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.
One thing that would help is a big comment somewhere explaining what the overall strategy is.
(Assignee)

Updated

9 years ago
Depends on: 461087
(Assignee)

Comment 11

9 years ago
(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.)
(Assignee)

Comment 12

9 years ago
Created attachment 346004 [details] [diff] [review]
font selection in Mozilla's PangoFcFontMap 1.2

Addresses the issues in comment 9 and comment 10 (except for bug 462798).
Attachment #344063 - Attachment is obsolete: true
Attachment #346004 - Flags: review?(roc)
Attachment #344063 - Flags: review?(roc)
Attachment #346004 - Flags: review?(roc) → review+
(Assignee)

Comment 13

9 years ago
Created attachment 346344 [details] [diff] [review]
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.
Attachment #346004 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #346344 - Attachment description: font selection in Mozilla's PangoFcFontMap 1.2 → font selection in Mozilla's PangoFcFontMap 1.2.2
What errors?
(Assignee)

Comment 15

9 years ago
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);
         }
(Assignee)

Comment 16

9 years ago
Created attachment 346567 [details] [diff] [review]
avoid static object

Our leak test tools don't seem to like (non-POD?) static objects (even with function scope).
Attachment #346567 - Flags: review?(roc)
(Assignee)

Comment 17

9 years ago
Created attachment 346569 [details] [diff] [review]
avoid static object v2

Putting the smaller object after the larger hash tables.
Attachment #346567 - Attachment is obsolete: true
Attachment #346569 - Flags: review?(roc)
Attachment #346567 - Flags: review?(roc)
Comment on attachment 346569 [details] [diff] [review]
avoid static object v2

Sorry, I should have caught that. Thanks. Get back in the landing queue.
Attachment #346569 - Flags: review?(roc) → review+
(Assignee)

Comment 19

9 years ago
Created attachment 346583 [details] [diff] [review]
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);
Attachment #346344 - Attachment is obsolete: true
Attachment #346569 - Attachment is obsolete: true
(Assignee)

Comment 20

9 years ago
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.
Try it under Valgrind.
(Assignee)

Comment 22

9 years ago
(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.
(Assignee)

Comment 23

9 years ago
Created attachment 346830 [details] [diff] [review]
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) {
Attachment #346583 - Attachment is obsolete: true
(Assignee)

Comment 24

9 years ago
http://hg.mozilla.org/mozilla-central/rev/d062597e5b3d
http://hg.mozilla.org/mozilla-central/rev/b39e3a7974f2
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9.1 → mozilla1.9.1b2
(Assignee)

Updated

9 years ago
Depends on: 463592

Updated

9 years ago
Depends on: 466956
(Assignee)

Comment 25

9 years ago
Created attachment 352261 [details]
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.)
(Assignee)

Updated

8 years ago
Blocks: 410678
Blocks: 991407
You need to log in before you can comment on or make changes to this bug.