385263 - [pango] we call FT_Open_Face twice per font

Status: RESOLVED FIXED

(Core :: Graphics, defect, P2)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We call FT_Open_Face twice per font that we load.  (We might even rasterize it twice -- I remember debugging a rounding problem once where the Xft debugging code was showing me correct rasterization but we were drawing incorrectly.)

The first call is here:

#0  FT_Open_Face (library=0xa194708, args=0xbf87af34, face_index=0, aface=0xa96b79c) at /usr/src/debug/freetype-2.3.4/src/base/ftobjs.c:1653
#1  0x00c8f9d8 in FT_New_Face (library=0xa194708, pathname=0xbf87af34 "\004", face_index=0, aface=0xa96b79c) at /usr/src/debug/freetype-2.3.4/src/base/ftobjs.c:1077
#2  0x060557ac in _XftLockFile (f=0xa96b770) at xftfreetype.c:157
#3  0x06055d21 in XftFontOpenInfo (dpy=0x9cd6c20, pattern=0xa912588, fi=0xbf87b028) at xftfreetype.c:800
#4  0x0605629a in XftFontOpenPattern (dpy=0x9cd6c20, pattern=0xa912588) at xftfreetype.c:1015
#5  0x009e10da in xft_font_get_font (font=<value optimized out>) at pangoxft-font.c:397
#6  0x024e749e in gfxPangoFont::RealizeXftFont (this=0xa876aa0, force=0) at /builds/trunk/mozilla/gfx/thebes/src/gfxPangoFonts.cpp:438
#7  0x024eca6a in gfxPangoFont::GetXftFont (this=0xa876aa0) at ../../../dist/include/thebes/gfxPangoFonts.h:79
#8  0x024e784c in gfxPangoFont::GetMetrics (this=0xa876aa0) at /builds/trunk/mozilla/gfx/thebes/src/gfxPangoFonts.cpp:533

The second call is here:

#0  FT_Open_Face (library=0xa641450, args=0xbf87a344, face_index=0, aface=0xbf87a398) at /usr/src/debug/freetype-2.3.4/src/base/ftobjs.c:1653
#1  0x00c8f9d8 in FT_New_Face (library=0xa641450, pathname=0xbf87a344 "\004", face_index=0, aface=0xbf87a398) at /usr/src/debug/freetype-2.3.4/src/base/ftobjs.c:1077
#2  0x02526e8e in _cairo_ft_unscaled_font_lock_face (unscaled=0xa99c060) at /builds/trunk/mozilla/gfx/cairo/cairo/src/cairo-ft-font.c:553
#3  0x02527638 in _cairo_ft_scaled_font_create (unscaled=0xa641450, font_face=0xbf87a344, font_matrix=0xbf87a398, ctm=0xbf87a6e8, options=0xa99c2c8, ft_options=
      {base = {antialias = CAIRO_ANTIALIAS_DEFAULT, subpixel_order = CAIRO_SUBPIXEL_ORDER_DEFAULT, hint_style = CAIRO_HINT_STYLE_FULL, hint_metrics = CAIRO_HINT_METRICS_DEFAULT}, load_flags = 0, extra_flags = 0}) at /builds/trunk/mozilla/gfx/cairo/cairo/src/cairo-ft-font.c:1449
#4  0x0252792e in _cairo_ft_font_face_scaled_font_create (abstract_face=0xa890cb8, font_matrix=0xbf87a650, ctm=0xbf87a6e8, options=0xa99c2c8, scaled_font=0xbf87a608)
    at /builds/trunk/mozilla/gfx/cairo/cairo/src/cairo-ft-font.c:2185
#5  0x0250732e in *INT__moz_cairo_scaled_font_create (font_face=0xa890cb8, font_matrix=0xbf87a650, ctm=0xbf87a6e8, options=0xa99c2c8)
    at /builds/trunk/mozilla/gfx/cairo/cairo/src/cairo-scaled-font.c:514
#6  0x024e63b9 in CreateScaledFont (aCR=0xa90d388, aCTM=0xbf87a6e8, aPangoFont=<value optimized out>) at /builds/trunk/mozilla/gfx/thebes/src/gfxPangoFonts.cpp:897
#7  0x024e70ca in gfxPangoFont::SetupCairoFont (this=0xa876aa0, aCR=0xa90d388) at /builds/trunk/mozilla/gfx/thebes/src/gfxPangoFonts.cpp:923

I'm not sure if this means we're allocating structures over again, or if we're getting the same ones back -- but I am seeing about 32K of allocation for one font within FT_Open_Face (not sure if it's all within one call, or separated between two).

Comment 1

[Karl Tomlinson (:karlt)]

This is a consequence of using Xft for measuring/glyph-selection and cairo for rendering, each of which have their own font (face) caches.

This can be resolved for distributions that --enable-system-cairo by switching from pangoxft to pangocairo.  (But with moz-cairo, moz-cairo and pango's cairo will still have separate caches.)

Comment 2

[Karl Tomlinson (:karlt)]

We are now not usually using Xft, but instead both system cairo (through Pango) and tree cairo.  Distributions using --enable-system-cairo and Pango-1.18 will be able to make a big gain (half the time spent opening fonts and half the cairo font cache size) by using pango_cairo_font_get_scaled_font() in CreateScaledFont.

We could make a similarly large gain in binary builds with tree cairo by implementing our own PangoFontMap.  With our own PangoFontMap providing an implementation PangoFcFont using tree cairo, we could still use pango_itemize and pango_shape.

Such an implementation might resemble the PangoCairoFcFontMap in Pango but would be linked against tree cairo (or system cairo if selected by distributions).  The implementation of PangoFcFont might be a wrapper around a gfxFont.
The gfxFontStyle could be passed through pango_itemize to the PangoFontMap by attaching it to the PangoContext.

As well as gains in speed and memory from using half as many fonts, our own PangoFontMap would give us greater control over fontconfig pattern caches (we could have more than 64 entries for the itemizing path) and choice of fonts for characters in a language different from that specified in the document (e.g. Latin characters in a Japanese document), would enable us to implement font-size-adjust and font-weight:bolder appropriately for more than just the first-specified font, and could enable us to disable font fallback for
PUA code points.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This sounds like a pretty complex fix; marking blocking 1.9- given the other remaining work, but please renom if you disagree.

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: a PangoFcFont using tree cairo

This seems to work.  Need to check performance.
This doesn't change font selection at all, but includes the font metrics changes from bug 403618.

Comment 5

[Karl Tomlinson (:karlt)]

          reference w/patch change
Ts (ms)      1567    1579    <+1% (less than margin of error)
Tp (ms)       462     463     ~0%
Tp_RSS (MB)   127     115    -10%

Comment 6

[Stuart Parmenter]

10% is nice.

Comment 7

[Karl Tomlinson (:karlt)]

We should end up with a net gain in performance but we lose a little of the
gain (~1% of cpu cycles in the Firefox process) because we rely on cairo to
cache glyph extents whereas the PangoCairoFont added it's own cache on top of
the cairo cache.

The cairo_scaled_font glyph cache and the PangoCairoFont glyph_extents_cache
are both the same size.  The effect of having two caches would be roughly
equivalent to doubling the size of the cairo cache (but differing eviction
policies are involved).

Most of the 1% difference is cache lookup inefficiencies (bug 453200) that we
can easily recover (and gain more).  The remaining fraction of the 1% is due
to having to load glyphs ~10% more often.

I think the effect of the second cache is small enough that there are likely
more significant performance issues that we could focus on.  (e.g. bug 453200
will gain us much more than we lose.)  And if this is worth working on then,
rather than adding another level of caching, a better solution would be to
tune cairo's caches.

Comment 8

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

We do our own glyph extents caching in gfxGlyphExtents --- I'm surprised these other caches still matter.

Comment 9

[Karl Tomlinson (:karlt)]

I was using "extents" in the Pango and cairo sense where extents includes the x-advance width (logical extents in Pango).  Advance width is required at shaping, and cairo_scaled_font_glyph_extents is used for this.  (The Pango APIs allow shapers to ask for inked (tight) extents also if they want them, but I think we can expect this to be rare.)

The glyph needs to be loaded to get the advance width, so inked extents become available at the same time with Freetype and cairo APIs.  Though caching only the advance width would mean a much smaller cache.

I guess gfxGlyphExtents can be modified to store advance widths for all (or at least more) glyphs.  AppUnits are a bit of a complication.  Do we do subpixel positioning on Mac?  If not we can just store integer pixels (one set for all AppUnitsPerGfxUnit factors).  (This reminds me of gfxFont's user/device unit confusion/ignorance.)  As the tight glyph extents are doubles there is no need to recalculate those for each factor either.

Comment 10

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

I see. I wouldn't bother changing anything here, then, just pretend I never said anything.

We do do subpixel positioning on Mac. I thought we did it with GTK too.

Comment 11

[Karl Tomlinson (:karlt)]

Freetype/cairo does subpixel antialiasing (sometimes/often), but not positioning.

For most Western language texts, cairo's 256 glyph per font cache should be enough.  I assume the issue becomes measurable in talos because the same scaled font is used for multiple different scripts.  Maybe it is significant in scripts with many characters too, but then i'd expect aquiring the glyph surfaces for painting to be the bottleneck.

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: a PangoFcFont using tree cairo v0.5

I'm happy with the performance of this one.

The reason we weren't getting the performance I expected was that there were still different cairo_scaled_fonts for measuring and drawing (even thought they were the same FT_Face and cairo_font_face), which meant that there were two independent glyph caches.  The changes in this version mean that we usually use same cairo_scaled_font for each phase (non-identity ctms being an exception), and so spend 36% less time in libfreetype (but on its own that represents only a saving of ~1% total time).

Comment 13

[Karl Tomlinson (:karlt)]

Comparing builds with and without attachment 336982 [details] [diff] [review] on the tryserver,
both builds having attachment 336403 [details] [diff] [review]:

          reference w/336982 change
Ts (ms)      1331    1303     -2%
Tp (ms)       447     424     -5%
Tp_RSS (MB)   121     108    -11%

The Tp savings are roughly 2% from metric calculation changes (bug 403618),
1.5% from having half as many FT_Faces and cairo_scaled_fonts, and 1.5% from
moving work from system cairo to a moz cairo with a fix for bug 453200.  If
builds were compared without attachment 336403 [details] [diff] [review], attachment 336982 [details] [diff] [review] would
increase the effect of bug 453200, and the Tp savings would be roughly halved.

I need to tidy up this patch and add a bit more documentation.

Comment 14

[Karl Tomlinson (:karlt)]

Attachment: a PangoFcFont using tree cairo and dependencies

This is a complete patch including everything that is not yet in other bugs.
I'll break some pieces off to make review easier.

Comment 15

[Karl Tomlinson (:karlt)]

Attachment: a PangoFcFont using tree cairo v1.2

The basic overview of the change here is that

  gfxFontGroup has 
    gfxFont has
      cairo_scaled_font
      PangoFont has 
        different cairo_scaled_font

becomes

  gfxFontGroup has
    PangoFont has
      gfxFont has
        cairo_scaled_font

Metric changes for bug 403618 got a bit tangled up here too.

Many of the patches in dependencies will be needed for this to apply/build/run.

Comment 16

[Karl Tomlinson (:karlt)]

Attachment: a PangoFcFont using tree cairo v1.3

Update to correct the diff of gfxPangoFonts.h which was against the wrong version, and to remove some "#if 0"d code.

Comment 17

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

Who wants to review this? I'm happy to do it, but maybe Stuart or Vlad should do it.

Comment 18

[Karl Tomlinson (:karlt)]

Attachment: a PangoFcFont using tree cairo v1.4

One more small change:  Always unref the fontmap on shutdown (not just in
debug versions).  We should be free from external influences that might cause
problems here (bug 454730 comment 3).

Comment 19

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

Comment on attachment 339005 [details] [diff] [review]
a PangoFcFont using tree cairo v1.4

+    gfxFloat GetAdjustedSize() { if (!mBasePangoFont) GetBasePangoFont(); return mAdjustedSize; }

This would be clearer written over multiple lines.

+    PRBool   mHasMetrics;

PRPackedBool and put it at the end. Doesn't make a real difference but it's safer if someone copies your code or adds fields.

+    operator FT_Face() { return mFace; }

We don't usually have overloaded conversion operators. Just make it GetFT_Face.

+    PRUint32 GetCharExtents(char aChar, cairo_text_extents_t& aExtents);
+
+    void GetMetrics(gfxFont::Metrics& aMetrics, PRUint32& aSpaceGlyph);

Prefer to use pointers for out-params so it's obvious in the caller that they're out-params.

+            // cairo won't know which font to open without a file.
+            // (We don't create fonts from an FT_Face.)
+            NS_NOTREACHED("Fonts without a file are not supported");
+            static const char *noFile = "NO FILE";
+            file = noFile;
...
+            // cairo won't know what to do here either.
+            NS_NOTREACHED("No index in pattern");
+            index = 0;

Are there guarantees that say we won't get here? Or are we just doomed if we do?

+        // Note that the unique face in the name and the gfxFontStyle are not
+        // necessarily enough to provide a key that will describe a unique
+        // font.  Here, aPattern is a fully resolved pattern from
+        // FcFontRenderPrepare, which takes the requested pattern and the font
+        // pattern as input and can modify elements of the resulting pattern
+        // that affect rendering but are not included in the gfxFontStyle.
+        nsRefPtr<gfxPangoFontEntry> fe = new gfxPangoFontEntry(name);

So you're saying that we need a new fontentry for every face in order to provide that unique key?

+    // In some case, _cairo_ft_options_merge uses some options from the

"In some cases"

+        xScale = FLOAT_FROM_26_6(FLOAT_FROM_16_16(ftMetrics.x_scale));
+        yScale = FLOAT_FROM_26_6(FLOAT_FROM_16_16(ftMetrics.y_scale));

Can you comment on what the unit conversions are here?

+            aMetrics.xHeight = 0.56 * aMetrics.emHeight; // guess

Comment on where 0.56 came from

Comment 20

[Karl Tomlinson (:karlt)]

(In reply to comment #19)
> +    operator FT_Face() { return mFace; }
> 
> We don't usually have overloaded conversion operators. Just make it GetFT_Face.

That ended up not being used, I think, so I'll just remove it for now.

> 
> +            // cairo won't know which font to open without a file.
> +            // (We don't create fonts from an FT_Face.)
> +            NS_NOTREACHED("Fonts without a file are not supported");
> +            static const char *noFile = "NO FILE";
> +            file = noFile;
> ...
> +            // cairo won't know what to do here either.
> +            NS_NOTREACHED("No index in pattern");
> +            index = 0;
> 
> Are there guarantees that say we won't get here? Or are we just doomed if we
> do?

gfx_pango_font_map_create_font guarantees this.

> 
> +        // Note that the unique face in the name and the gfxFontStyle are not
> +        // necessarily enough to provide a key that will describe a unique
> +        // font.  Here, aPattern is a fully resolved pattern from
> +        // FcFontRenderPrepare, which takes the requested pattern and the font
> +        // pattern as input and can modify elements of the resulting pattern
> +        // that affect rendering but are not included in the gfxFontStyle.
> +        nsRefPtr<gfxPangoFontEntry> fe = new gfxPangoFontEntry(name);
> 
> So you're saying that we need a new fontentry for every face in order to
> provide that unique key?

That wasn't what I was trying to say.  It's that the cairoFont contains
information (from the pattern) that is not in the name/fe and fontStyle
combination.

I'll modify the comment a bit and move it to the "new gfxFcFont" statement.

> +            aMetrics.xHeight = 0.56 * aMetrics.emHeight; // guess
> 
> Comment on where 0.56 came from

It kind of came from Windows/Atsui fonts but the math is not right.
I now see that CSS 2.1 says to use 0.5em, so I'll do that and comment why.

I'll make the other changes as suggested, thanks.

Comment 21

[Karl Tomlinson (:karlt)]

Attachment: a PangoFcFont using tree cairo v1.5

Addressed roc's review comments as discussed in comment 19 and 20.

Comment 22

[Karl Tomlinson (:karlt)]

I landed this, but backed out due to svg reftest failures

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1222410677.1222414459.5180.gz#err0

until I can work out why this patch caused the following <rect> not to fill the
content area of the enclosing <svg> element:

    <svg xmlns="http://www.w3.org/2000/svg" width="51" height="51"
         style="float:left; padding:0; border:1px solid blue; margin:0;">
      <rect width="100%" height="100%" fill="blue"/>
    </svg>

Comment 23

[Karl Tomlinson (:karlt)]

Attachment: make line heights integer

In the svg reftests, it looks like the border on <svg/> is being snapped to
pixels but the <rect/> that is filled is not.  (This is evident in the fact
that the test won't pass except at zoom = 1.)

The position of the rect depends on the line-height of the surrounding text,
so the test is implicitly testing for integer line-heights (which might be why
many of the tests don't pass on Mac).

While I assume integer line-heights are not necessary for CSS compliance, they
actually seem a good idea, because they ensure that a paragraph of lines will
be equally spaced (rather than just being snapped to pixels, some up and some
down.)

Comment 24

[Karl Tomlinson (:karlt)]

Comment on attachment 340550 [details] [diff] [review]
make line heights integer

This logic is not quite right.

Comment 25

[Karl Tomlinson (:karlt)]

Attachment: make line heights integer v2

Comment 26

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

Comment on attachment 340554 [details] [diff] [review]
make line heights integer v2

I guess this approach is OK.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

During the brief time that this landed, it helped memory use a good bit:
http://graphs.mozilla.org/graph.html#show=395139,395141,395170,911698,1431037&sel=1222343298,1222467457

Comment 28

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/25ca781361ed
http://hg.mozilla.org/mozilla-central/rev/406819ca370c