The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla24

Status

()

Core
Graphics: Text
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

unspecified
mozilla24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments, 4 obsolete attachments)

6.02 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.66 KB, patch
roc
: review+
Details | Diff | Splinter Review
7.26 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.67 KB, patch
roc
: review+
Details | Diff | Splinter Review
56.50 KB, patch
roc
: review+
Details | Diff | Splinter Review
15.76 KB, patch
roc
: review+
Details | Diff | Splinter Review
22.17 KB, patch
roc
: review+
Details | Diff | Splinter Review
12.83 KB, patch
roc
: review+
Details | Diff | Splinter Review
128.77 KB, patch
Details | Diff | Splinter Review
1.42 KB, patch
roc
: review+
gaston
: feedback+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
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

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

Comment 2

4 years ago
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.
Assignee: nobody → jfkthame
Summary: optimize our use of the graphite api - share gr_face across multiple sizes of a font → optimize our use of the harfbuzz and graphite apis: share hb_face_t / gr_face objects across multiple sizes of a font
(Assignee)

Comment 3

4 years ago
Created attachment 750044 [details] [diff] [review]
pt 1 - gfxFont/gfxFontEntry refactoring for more efficient use of shaper APIs
Attachment #750044 - Flags: review?(roc)
(Assignee)

Comment 4

4 years ago
Created attachment 750046 [details] [diff] [review]
pt 2 - update gfxHarfBuzzShaper for the refactored font code
Attachment #750046 - Flags: review?(roc)
(Assignee)

Comment 5

4 years ago
Created attachment 750048 [details] [diff] [review]
pt 3 - update gfxGraphiteShaper for the refactored font code
Attachment #750048 - Flags: review?(roc)
(Assignee)

Comment 6

4 years ago
Created attachment 750050 [details] [diff] [review]
pt 4 - update GDI font backend
Attachment #750050 - Flags: review?(roc)
(Assignee)

Comment 7

4 years ago
Created attachment 750051 [details] [diff] [review]
pt 5 - update DWrite font backend
Attachment #750051 - Flags: review?(roc)
(Assignee)

Comment 8

4 years ago
Created attachment 750052 [details] [diff] [review]
pt 6 - update Mac font backend
Attachment #750052 - Flags: review?(roc)
(Assignee)

Comment 9

4 years ago
Created attachment 750053 [details] [diff] [review]
pt 7 - update Pango font backend
Attachment #750053 - Flags: review?(roc)
(Assignee)

Comment 10

4 years ago
Created attachment 750054 [details] [diff] [review]
pt 8 - update FT2 font backend
Attachment #750054 - Flags: review?(roc)
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.
Attachment #750044 - Flags: review?(roc) → review+
Attachment #750046 - Flags: review?(roc) → review+
Attachment #750048 - Flags: review?(roc) → review+
Attachment #750050 - Flags: review?(roc) → review+
Attachment #750051 - Flags: review?(roc) → review+
Attachment #750052 - Flags: review?(roc) → review+
Actually I think we should avoid manual hb_blob_destroys by exposing AutoBlob and encouraging callers of GetFontTable to use AutoBlob instead.
Attachment #750053 - Flags: review?(roc) → review+
Attachment #750054 - Flags: review?(roc) → review+
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.
Attachment #750044 - Flags: review+ → review-
(Assignee)

Comment 14

4 years ago
Created attachment 750363 [details] [diff] [review]
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.
Attachment #750363 - Flags: review?(roc)
(Assignee)

Updated

4 years ago
Attachment #750044 - Attachment is obsolete: true
(Assignee)

Comment 15

4 years ago
Created attachment 750365 [details] [diff] [review]
pt 2 - update gfxHarfBuzzShaper for the refactored font code

Updated to use gfxFontEntry::AutoTable.
Attachment #750365 - Flags: review?(roc)
(Assignee)

Updated

4 years ago
Attachment #750046 - Attachment is obsolete: true
(Assignee)

Comment 16

4 years ago
Created attachment 750366 [details] [diff] [review]
pt 5 - update DWrite font backend

Updated to use gfxFontEntry::AutoTable.
Attachment #750366 - Flags: review?(roc)
(Assignee)

Updated

4 years ago
Attachment #750051 - Attachment is obsolete: true
(Assignee)

Comment 17

4 years ago
Created attachment 750367 [details] [diff] [review]
pt 6 - update Mac font backend

Updated to use gfxFontEntry::AutoTable.
Attachment #750367 - Flags: review?(roc)
(Assignee)

Updated

4 years ago
Attachment #750052 - Attachment is obsolete: true
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.
Attachment #750363 - Flags: review?(roc) → review+
Attachment #750365 - Flags: review?(roc) → review+
Attachment #750366 - Flags: review?(roc) → review+
Attachment #750367 - Flags: review?(roc) → review+
(Assignee)

Comment 19

4 years ago
Created attachment 750499 [details] [diff] [review]
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.)
(Assignee)

Comment 20

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a6d36fc3749
Target Milestone: --- → mozilla24
https://hg.mozilla.org/mozilla-central/rev/0a6d36fc3749
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
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 ?
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
Had to back this out due to conflicts with another backout:

https://hg.mozilla.org/integration/mozilla-inbound/rev/370a2c56b793
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 25

4 years ago
Re-landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9aee54818715
(Assignee)

Comment 26

4 years ago
Created attachment 751542 [details] [diff] [review]
followup - explicitly cast the AutoSwap type to avoid ambiguity

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

Updated

4 years ago
Flags: needinfo?(landry)
(Assignee)

Updated

4 years ago
Depends on: 874053
https://hg.mozilla.org/mozilla-central/rev/9aee54818715
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
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>
Status: RESOLVED → REOPENED
Flags: needinfo?(landry)
Resolution: FIXED → ---
(Assignee)

Updated

4 years ago
Blocks: 876070
(Assignee)

Comment 29

4 years ago
(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.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
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..
Attachment #751542 - Flags: feedback+
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.
Attachment #751542 - Flags: review?(roc)
Attachment #751542 - Flags: review?(roc) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f082ca6465c
https://hg.mozilla.org/mozilla-central/rev/6f082ca6465c
You need to log in before you can comment on or make changes to this bug.