600452 - share font entries between gfxFcFonts with the same cairo_font_face_t

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Karl Tomlinson (:karlt)]

Currently (scaled) gfxFcFonts usually each have their own gfxFontEntry.

gfxHarfBuzzShaper typically lives as long as its associated scaled font.  It
keeps references hb_blob_ts of font table data.  Memory use to kept down by
sharing the font table data between shapers using the same font face by
keeping a record of the tables on the gfxFontEntry.

For this to work effectively with gfxFcFonts, the fonts need to share font
entries.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: share font entries between fonts with the same cairo_font_face_t

In a similar way to the management of font entries for downloaded fonts,
cairo_font_face_ts are used to keep track of existing font entries for system
fonts.

In some situations, such as different antialias options at different sizes,
there may be two co-existing cairo_font_face_ts for the same system font.
In those situations, the font entries could be shared but that would require
maintaining a hash table of existing font entries.  It seems better just to
piggy-back on the cairo_font_face_t hash table, which gives us most of the gain but for next to no cost.

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: move font FcPattern reference from Font to FontEntry

Now, the font entry is a sensible place to keep the font FcPattern reference
instead of passing the pattern to every scaled font.

This patch also moves the remainder of FontEntry/cairo_font_face
management to the new gfxFcFontEntry structure.

Comment 3

[Karl Tomlinson (:karlt)]

Bug 569770 needs this to keep footprint reasonable.

Comment 4

[Jonathan Kew [:jfkthame]]

Comment on attachment 479288 [details] [diff] [review]
move font FcPattern reference from Font to FontEntry

Seems fine, modulo a couple of small issues:

>+    // FontEntrys for src:local() fonts in gfxUserFontSet may return more than
>+    // one pattern.  (See comment in gfxUserFcFontEntry.)
>+    const nsTArray< nsCountedRef<FcPattern> >& GetPatterns()
>+    {
>+        return mPatterns;
>+    }

I don't think we're altogether consistent about it, but in general I'd expect the opening brace to be on the same line as the method name here....

>+    static gfxFcFontEntry *LookupFontEntry(cairo_font_face_t *aFace) {
>+        return static_cast<gfxFcFontEntry*>
>+            (cairo_font_face_get_user_data(aFace, &sFontEntryKey));
>+    }

....though if you'd prefer to put this one on a new line as well, I won't argue with that style. (As I don't recall whether the style guide includes explicit rules on this issue.)

>     {
>         cairo_font_face_reference(mFontFace);
>         cairo_font_face_set_user_data(mFontFace, &sFontEntryKey, this, NULL);
>+        mPatterns.AppendElement();
>+        mPatterns[0] = aFontPattern;
>     }

This worried me briefly, as nsTArray::AppendElement() is not generally infallible; but as mPatterns is actually an nsAutoTArray<Type,1> it should never fail here. Could we have a comment noting that fact, please, so that I don't worry next time I run across this code fragment? :)

> gfxFcFont::gfxFcFont(cairo_scaled_font_t *aCairoFont,
>                      FcPattern *aFontPattern,
>-                     gfxFontEntry *aFontEntry,
>+                     gfxFcFontEntry *aFontEntry,
>                      const gfxFontStyle *aFontStyle)
>     : gfxFT2FontBase(aCairoFont, aFontEntry, aFontStyle),
>-      mFontPattern(aFontPattern),
>       mPangoFont()
> {
>     cairo_scaled_font_set_user_data(mScaledFont, &sGfxFontKey, this, NULL);
> }

As this no longer uses aFontPattern for anything, it looks like you could remove this parameter from the constructor altogether.

r=me, with the above nits addressed appropriately.

Comment 5

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/df3db4d3a0c7
http://hg.mozilla.org/mozilla-central/rev/6b3f7a3a6a7f

(In reply to comment #4)
> ....though if you'd prefer to put this one on a new line as well, I won't argue
> with that style.

I went with the new line, as it is in most of the other cases in this file, and I think I like the consistency between inline / non-inline functions, when the inline functions need extra lines for the body.
Some like the way methods have different style to classes (though I don't really have an opinion on that).

> This worried me briefly, as nsTArray::AppendElement() is not generally
> infallible; but as mPatterns is actually an nsAutoTArray<Type,1> it should
> never fail here. Could we have a comment noting that fact, please, so that I
> don't worry next time I run across this code fragment? :)

Added some comments.

> As this no longer uses aFontPattern for anything, it looks like you could
> remove this parameter from the constructor altogether.

Yes.  Done, thanks.