847344 - optimize our use of the harfbuzz and graphite apis: share hb_face_t / gr_face objects across multiple sizes of a font

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Jonathan Kew [:jfkthame]]

From discussion with Martin, it appears we could save significant RAM by sharing the gr_face across all instantiations of a particular graphite font (regardless of size). So we should move this to the font entry, instead of creating it within the gfxGraphiteShaper (which belongs to the size-specific gfxFont). Only the gr_font should be created per-size.

This should significantly reduce the RAM impact of enabling graphite in fennec, as reported in bug 846832.

Comment 1

[martin_hosken]

By adding some telemetry to graphite we find that on a 64-bit system, Charis gr_face ram footprint is about 820K. This would probably reduce 200K on 32-bit. We will investigate to see if we might drop them both to about 500K but need to compare speed tradeoffs. A gr_font footprint is about 4 bytes per glyph (5K glyphs in Charis).

Comment 2

[Jonathan Kew [:jfkthame]]

Looking at the refactoring involved here, it's actually applicable to both the harfbuzz and graphite shaping back-ends. They both have a "face" object that is relatively expensive to create, and which provides the size-independent interface to the font tables, shaping behavior, etc., and then a "font" object that carries size information.

Currently, we create both the "face" and "font" objects within the gfxHarfBuzzShaper or gfxGraphiteShaper interface object that's attached to a specific gfxFont instance. This means that for each separate instance (gfxFont) that we create for a given font file (gfxFontEntry), we make harfbuzz do its font table sanitization afresh; and we make graphite do its parsing of the graphite tables and creating its internal structures (which add up to a significant RAM footprint) separately for each size.

So what I plan to do here is to refactor the gfxFont and gfxFontEntry classes, and the font table access methods they provide, such that the (size-independent) font entry will be responsible for creating a harfbuzz and/or graphite face object that can be shared by all (size-specific) gfxFont instances that use the same font entry.

Harfbuzz and graphite take different approaches to object lifetime management, and therefore the gfxFontEntry's management of hb_face_t and gr_face will not be the same, but they're each fairly straightforward in their own right. For hb_face_t, we can use its internal refcounting, while for gr_face, we'll explicitly keep count in the gfxFontEntry as the face is requested/released by gfxFont instances.

One aspect of the refactoring here is that currently, we have two kinds of GetFontTable() methods at different levels: gfxFontEntry::GetFontTable copies font table into a caller-supplied nsTArray, and gfxFont::GetFontTable returns an hb_blob_t that wraps table data but may not involve copying. To support creation of the hb_face_t by the font entry, we need to move the hb_blob_t-returning GetFontTable to gfxFontEntry, so that's one of the refactorings here.

I'm proposing to rework things so that gfxFontEntry::GetFontTable, returning an hb_blob_t, will be the -only- public API we use for getting font tables. The array-copying version will be renamed CopyFontTable, and will be used only internally by the default impl of GetFontTable (where we cache the tables within the gfxFontEntry). Font entry subclasses that provide a non-copying GetFontTable of their own (DWrite, MacOSX) will not need to use CopyFontTable at all.

To make review more manageable, I've split the patch here into multiple pieces, but they'll need to be folded together for landing. As various interfaces between gfxFont, gfxFontEntry, and related classes are being modified, the complete set of patches need to land together.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: pt 1 - gfxFont/gfxFontEntry refactoring for more efficient use of shaper APIs

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - update gfxHarfBuzzShaper for the refactored font code

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: pt 3 - update gfxGraphiteShaper for the refactored font code

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: pt 4 - update GDI font backend

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: pt 5 - update DWrite font backend

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: pt 6 - update Mac font backend

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: pt 7 - update Pango font backend

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: pt 8 - update FT2 font backend

Comment 11

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

Comment on attachment 750044 [details] [diff] [review]
pt 1 - gfxFont/gfxFontEntry refactoring for more efficient use of shaper APIs

Review of attachment 750044 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with those fixed.

::: gfx/thebes/gfxFont.cpp
@@ +82,5 @@
> +// when the AutoBlob goes out of scope; takes over ownership of the hb_blob
> +// reference passed to its constructor, does not add a reference itself.
> +class AutoBlob {
> +public:
> +    AutoBlob(hb_blob_t* aBlob): mBlob(aBlob) { }

Why not move the GetFontTable call that you always use with AutoBlob into AutoBlob itself, so the constructor takes the gfxFontEntry and the table name? That would save a bit of code and prevent mistakes (wouldn't need the "does not add a reference itself" note).

@@ +89,5 @@
> +private:
> +    hb_blob_t* mBlob;
> +    // not implemented:
> +    AutoBlob(const AutoBlob&);
> +    AutoBlob& operator=(const AutoBlob&);

use MOZ_DELETE

::: gfx/thebes/gfxFont.h
@@ +330,5 @@
>      }
>  
> +    // Access to raw font table data (needed for Harfbuzz):
> +    // returns a pointer to data owned by the fontEntry or the OS,
> +    // which will remain valid until the blob is destroyed.

Can you say something more about the lifetime here? As-is, it's unclear how to safely use the result (i.e. what kinds of operations could result in the blob being destroyed). I assume the blob is guaranteed to exist as long as the gfxFontEntry, but you should say that.

::: gfx/thebes/gfxSVGGlyphs.h
@@ +111,5 @@
>      /**
>       * @param aSVGTable The SVG table from the OpenType font
>       * @param aCmapTable The CMAP table from the OpenType font
>       */
> +    gfxSVGGlyphs(hb_blob_t *aSVGTable, hb_blob_t *aCmapTable);

Add a comment explaining that ownership of these blobs passes to the gfxSVGGlyphs.

Comment 12

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

Actually I think we should avoid manual hb_blob_destroys by exposing AutoBlob and encouraging callers of GetFontTable to use AutoBlob instead.

Comment 13

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

Comment on attachment 750044 [details] [diff] [review]
pt 1 - gfxFont/gfxFontEntry refactoring for more efficient use of shaper APIs

Review of attachment 750044 [details] [diff] [review]:
-----------------------------------------------------------------

Changing this to r- to get AutoBlob improved.

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: pt 1 - gfxFont/gfxFontEntry refactoring for more efficient use of shaper APIs

OK, I've replaced AutoBlob with a public helper class gfxFontEntry::AutoTable, which can be used whenever we need a short-lived reference to a particular font table. This works nicely for the transient use of tables while setting up font metrics, reading cmaps, etc. Then only cases where we want to hold on to a table reference longer-term will use manual hb_blob_destroy() to release it.

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - update gfxHarfBuzzShaper for the refactored font code

Updated to use gfxFontEntry::AutoTable.

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: pt 5 - update DWrite font backend

Updated to use gfxFontEntry::AutoTable.

Comment 17

[Jonathan Kew [:jfkthame]]

Attachment: pt 6 - update Mac font backend

Updated to use gfxFontEntry::AutoTable.

Comment 18

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

Comment on attachment 750363 [details] [diff] [review]
pt 1 - gfxFont/gfxFontEntry refactoring for more efficient use of shaper APIs

Review of attachment 750363 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.h
@@ +339,5 @@
> +    // access than copying table data into our own buffers.
> +    //
> +    // Get blob that encapsulates a specific font table, or NULL if
> +    // the table doesn't exist in the font
> +    virtual hb_blob_t *GetFontTable(uint32_t aTag);

The comment should be more clear that the caller is responsible for calling hb_blob_destroy on the result.

@@ +359,5 @@
> +    private:
> +        hb_blob_t* mBlob;
> +        // not implemented:
> +        AutoTable(const AutoTable&);
> +        AutoTable& operator=(const AutoTable&);

MOZ_DELETE

::: gfx/thebes/gfxSVGGlyphs.h
@@ +111,5 @@
>      /**
>       * @param aSVGTable The SVG table from the OpenType font
>       * @param aCmapTable The CMAP table from the OpenType font
>       */
> +    gfxSVGGlyphs(hb_blob_t *aSVGTable, hb_blob_t *aCmapTable);

Please make this comment clear that ownership of the blobs passes to gfxSVGGlyphs.

Comment 19

[Jonathan Kew [:jfkthame]]

Attachment: refactor gfxFont/gfxFontEntry and associated classes for more efficient use of HarfBuzz and Graphite shaper APIs.

Fixed-up comments as requested, and folded all parts into a single patch ready for landing. (Carrying forward r=roc from all the pieces.)

Comment 20

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a6d36fc3749

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0a6d36fc3749

Comment 22

[Landry Breuil (:gaston)]

This broke the build on OpenBSD :

gfx/thebes/gfxPangoFonts.cpp:832:35: error: conversion from 'const mozilla::AutoSwap_PRUint16' to 'size_t'  (aka 'unsigned long') is ambiguous

        (bsearch(&aTableTag, dir, header->numTables, sizeof(TableDirEntry),
                                  ^~~~~~~~~~~~~~~~~
/var/buildslave-mozilla/mozilla-central-amd64/build/gfx/thebes/gfxFontUtils.h:354:5: note: candidate function
    operator uint16_t() const
    ^
/var/buildslave-mozilla/mozilla-central-amd64/build/gfx/thebes/gfxFontUtils.h:359:5: note: candidate function
    operator uint32_t() const
    ^
/var/buildslave-mozilla/mozilla-central-amd64/build/gfx/thebes/gfxFontUtils.h:364:5: note: candidate function
    operator uint64_t() const
    ^
/usr/include/stdlib.h:116:49: note: passing argument to parameter here
void    *bsearch(const void *, const void *, size_t, size_t,
                                                   ^
1 error generated.
gmake[6]: *** [gfxPangoFonts.o] Error 1

the return of the pruint nightmare..... maybe an explicit reinterpret_cast would help here ?

Comment 23

[Landry Breuil (:gaston)]

oh, forgot to mention it broke on OpenBSD i386 _and_ amd64 :
http://buildbot.rhaalovely.net/builders/mozilla-central-amd64/builds/761/steps/build/logs/stdio
http://buildbot.rhaalovely.net/builders/mozilla-central-i386/builds/312/steps/build/logs/stdio

Comment 24

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Had to back this out due to conflicts with another backout:

https://hg.mozilla.org/integration/mozilla-inbound/rev/370a2c56b793

Comment 25

[Jonathan Kew [:jfkthame]]

Re-landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9aee54818715

Comment 26

[Jonathan Kew [:jfkthame]]

Attachment: followup - explicitly cast the AutoSwap type to avoid ambiguity

I believe this should fix the OpenBSD build problem. :gaston, could you please confirm this?

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9aee54818715

Comment 28

[Landry Breuil (:gaston)]

I'd love to test it properly, but something which landed in the meantime broke me even more :

In file included from ../../dist/include/mozilla/Atomics.h:166:
../../dist/stl_wrappers/atomic:54:15: fatal error: 'atomic' file not found
#include_next <atomic>

Comment 29

[Jonathan Kew [:jfkthame]]

(In reply to Landry Breuil (:gaston) from comment #28)
> I'd love to test it properly, but something which landed in the meantime
> broke me even more :
> 
> In file included from ../../dist/include/mozilla/Atomics.h:166:
> ../../dist/stl_wrappers/atomic:54:15: fatal error: 'atomic' file not found
> #include_next <atomic>

It sounds like you should file a new bug to track the OpenBSD build failure(s); you probably want the followup patch from comment 26, but it's not clear to me (at least without more info) whether this new problem has anything to do with the changes here or if it's an unrelated issue.

Comment 30

[Landry Breuil (:gaston)]

Comment on attachment 751542 [details] [diff] [review]
followup - explicitly cast the AutoSwap type to avoid ambiguity

The other failure related to the use of atomic stuff is definitely unrelated, the build goes past gfxPangoFonts.cpp if i apply this patch on top of 0a6d36fc3749. So definitely f+ for me :)

i'll file a separate bug for the other failure once i've found its source..

Comment 31

[Landry Breuil (:gaston)]

Comment on attachment 751542 [details] [diff] [review]
followup - explicitly cast the AutoSwap type to avoid ambiguity

just making sure this gets landed... it's also needed on OpenBSD/i386.

Comment 32

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f082ca6465c

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6f082ca6465c