Closed Bug 449356 Opened 16 years ago Closed 16 years ago

Refactor gfxPangoFontGroup for user fonts

Categories

(Core :: Graphics, defect, P2)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: karlt, Assigned: karlt)

References

Details

(Keywords: memory-footprint, perf)

Attachments

(2 files, 8 obsolete files)

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.
We need this for downloadable fonts, but completion by August 19 is improbable.
Flags: wanted1.9.1?
Priority: -- → P1
Target Milestone: --- → mozilla1.9.1
Blocks: 428425
Blocks: 385263
Flags: wanted1.9.1? → wanted1.9.1+
Priority: P1 → P2
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.
Blocks: 397860
No longer blocks: 385263
Depends on: 385263
No longer blocks: 397860, 403618
Depends on: 404857
Depends on: 456545
(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.
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.
Blocks: 458169
No longer blocks: 441473
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
Keywords: footprint, perf
(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%).
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.
Depends on: 461087
(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.)
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)
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
Attachment #346344 - Attachment description: font selection in Mozilla's PangoFcFontMap 1.2 → font selection in Mozilla's PangoFcFontMap 1.2.2
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);
         }
Attached patch avoid static object (obsolete) — Splinter Review
Our leak test tools don't seem to like (non-POD?) static objects (even with function scope).
Attachment #346567 - Flags: review?(roc)
Attached patch avoid static object v2 (obsolete) — Splinter Review
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+
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
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.
(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.
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
http://hg.mozilla.org/mozilla-central/rev/d062597e5b3d
http://hg.mozilla.org/mozilla-central/rev/b39e3a7974f2
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9.1 → mozilla1.9.1b2
Depends on: 463592
Depends on: 466956
Attached image 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.)
Blocks: 410678
Blocks: 991407
You need to log in before you can comment on or make changes to this bug.