Last Comment Bug 847344 - optimize our use of the harfbuzz and graphite apis: share hb_face_t / gr_face objects across multiple sizes of a font
: optimize our use of the harfbuzz and graphite apis: share hb_face_t / gr_face...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics: Text (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla24
Assigned To: Jonathan Kew (:jfkthame)
:
Mentors:
Depends on: 874053
Blocks: 876070
  Show dependency treegraph
 
Reported: 2013-03-04 03:59 PST by Jonathan Kew (:jfkthame)
Modified: 2014-04-25 15:16 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
pt 1 - gfxFont/gfxFontEntry refactoring for more efficient use of shaper APIs (56.25 KB, patch)
2013-05-15 13:45 PDT, Jonathan Kew (:jfkthame)
roc: review-
Details | Diff | Splinter Review
pt 2 - update gfxHarfBuzzShaper for the refactored font code (15.70 KB, patch)
2013-05-15 13:45 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Splinter Review
pt 3 - update gfxGraphiteShaper for the refactored font code (6.02 KB, patch)
2013-05-15 13:46 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Splinter Review
pt 4 - update GDI font backend (2.66 KB, patch)
2013-05-15 13:46 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Splinter Review
pt 5 - update DWrite font backend (22.09 KB, patch)
2013-05-15 13:46 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Splinter Review
pt 6 - update Mac font backend (12.89 KB, patch)
2013-05-15 13:46 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Splinter Review
pt 7 - update Pango font backend (7.26 KB, patch)
2013-05-15 13:47 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Splinter Review
pt 8 - update FT2 font backend (6.67 KB, patch)
2013-05-15 13:47 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Splinter Review
pt 1 - gfxFont/gfxFontEntry refactoring for more efficient use of shaper APIs (56.50 KB, patch)
2013-05-16 02:49 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Splinter Review
pt 2 - update gfxHarfBuzzShaper for the refactored font code (15.76 KB, patch)
2013-05-16 02:50 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Splinter Review
pt 5 - update DWrite font backend (22.17 KB, patch)
2013-05-16 02:51 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Splinter Review
pt 6 - update Mac font backend (12.83 KB, patch)
2013-05-16 02:51 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Splinter Review
refactor gfxFont/gfxFontEntry and associated classes for more efficient use of HarfBuzz and Graphite shaper APIs. (128.77 KB, patch)
2013-05-16 09:31 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
followup - explicitly cast the AutoSwap type to avoid ambiguity (1.42 KB, patch)
2013-05-19 20:34 PDT, Jonathan Kew (:jfkthame)
roc: review+
landry: feedback+
Details | Diff | Splinter Review

Description Jonathan Kew (:jfkthame) 2013-03-04 03:59:03 PST
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 2013-03-04 06:57:38 PST
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) 2013-05-15 13:40:23 PDT
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) 2013-05-15 13:45:38 PDT
Created attachment 750044 [details] [diff] [review]
pt 1 - gfxFont/gfxFontEntry refactoring for more efficient use of shaper APIs
Comment 4 Jonathan Kew (:jfkthame) 2013-05-15 13:45:57 PDT
Created attachment 750046 [details] [diff] [review]
pt 2 - update gfxHarfBuzzShaper for the refactored font code
Comment 5 Jonathan Kew (:jfkthame) 2013-05-15 13:46:11 PDT
Created attachment 750048 [details] [diff] [review]
pt 3 - update gfxGraphiteShaper for the refactored font code
Comment 6 Jonathan Kew (:jfkthame) 2013-05-15 13:46:24 PDT
Created attachment 750050 [details] [diff] [review]
pt 4 - update GDI font backend
Comment 7 Jonathan Kew (:jfkthame) 2013-05-15 13:46:41 PDT
Created attachment 750051 [details] [diff] [review]
pt 5 - update DWrite font backend
Comment 8 Jonathan Kew (:jfkthame) 2013-05-15 13:46:55 PDT
Created attachment 750052 [details] [diff] [review]
pt 6 - update Mac font backend
Comment 9 Jonathan Kew (:jfkthame) 2013-05-15 13:47:08 PDT
Created attachment 750053 [details] [diff] [review]
pt 7 - update Pango font backend
Comment 10 Jonathan Kew (:jfkthame) 2013-05-15 13:47:18 PDT
Created attachment 750054 [details] [diff] [review]
pt 8 - update FT2 font backend
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-05-15 16:07:31 PDT
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) 2013-05-15 16:21:28 PDT
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) 2013-05-15 16:23:40 PDT
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) 2013-05-16 02:49:37 PDT
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.
Comment 15 Jonathan Kew (:jfkthame) 2013-05-16 02:50:50 PDT
Created attachment 750365 [details] [diff] [review]
pt 2 - update gfxHarfBuzzShaper for the refactored font code

Updated to use gfxFontEntry::AutoTable.
Comment 16 Jonathan Kew (:jfkthame) 2013-05-16 02:51:13 PDT
Created attachment 750366 [details] [diff] [review]
pt 5 - update DWrite font backend

Updated to use gfxFontEntry::AutoTable.
Comment 17 Jonathan Kew (:jfkthame) 2013-05-16 02:51:37 PDT
Created attachment 750367 [details] [diff] [review]
pt 6 - update Mac font backend

Updated to use gfxFontEntry::AutoTable.
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-05-16 05:38:32 PDT
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) 2013-05-16 09:31:42 PDT
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.)
Comment 21 Ryan VanderMeulen [:RyanVM] 2013-05-16 17:41:18 PDT
https://hg.mozilla.org/mozilla-central/rev/0a6d36fc3749
Comment 22 Landry Breuil (:gaston) 2013-05-18 06:18:22 PDT
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 24 :Ms2ger (⌚ UTC+1/+2) 2013-05-19 11:28:36 PDT
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) 2013-05-19 20:30:00 PDT
Re-landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9aee54818715
Comment 26 Jonathan Kew (:jfkthame) 2013-05-19 20:34:24 PDT
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?
Comment 27 Ryan VanderMeulen [:RyanVM] 2013-05-20 13:27:53 PDT
https://hg.mozilla.org/mozilla-central/rev/9aee54818715
Comment 28 Landry Breuil (:gaston) 2013-05-20 13:54:49 PDT
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) 2013-05-24 19:20:42 PDT
(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) 2013-05-25 10:24:54 PDT
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) 2013-05-29 20:00:23 PDT
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 33 Ryan VanderMeulen [:RyanVM] 2013-05-30 09:07:17 PDT
https://hg.mozilla.org/mozilla-central/rev/6f082ca6465c

Note You need to log in before you can comment on or make changes to this bug.