Closed
Bug 847344
Opened 12 years ago
Closed 12 years ago
optimize our use of the harfbuzz and graphite apis: share hb_face_t / gr_face objects across multiple sizes of a font
Categories
(Core :: Graphics: Text, defect)
Core
Graphics: Text
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: jfkthame, Assigned: jfkthame)
References
Details
Attachments
(10 files, 4 obsolete files)
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 |
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•12 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•12 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•12 years ago
|
||
Attachment #750044 -
Flags: review?(roc)
Assignee | ||
Comment 4•12 years ago
|
||
Attachment #750046 -
Flags: review?(roc)
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #750048 -
Flags: review?(roc)
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #750050 -
Flags: review?(roc)
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #750051 -
Flags: review?(roc)
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #750052 -
Flags: review?(roc)
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #750053 -
Flags: review?(roc)
Assignee | ||
Comment 10•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
Attachment #750044 -
Attachment is obsolete: true
Assignee | ||
Comment 15•12 years ago
|
||
Updated to use gfxFontEntry::AutoTable.
Attachment #750365 -
Flags: review?(roc)
Assignee | ||
Updated•12 years ago
|
Attachment #750046 -
Attachment is obsolete: true
Assignee | ||
Comment 16•12 years ago
|
||
Updated to use gfxFontEntry::AutoTable.
Attachment #750366 -
Flags: review?(roc)
Assignee | ||
Updated•12 years ago
|
Attachment #750051 -
Attachment is obsolete: true
Assignee | ||
Comment 17•12 years ago
|
||
Updated to use gfxFontEntry::AutoTable.
Attachment #750367 -
Flags: review?(roc)
Assignee | ||
Updated•12 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•12 years ago
|
||
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•12 years ago
|
||
Target Milestone: --- → mozilla24
Comment 21•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 22•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Assignee | ||
Comment 26•12 years ago
|
||
I believe this should fix the OpenBSD build problem. :gaston, could you please confirm this?
Assignee | ||
Updated•12 years ago
|
Flags: needinfo?(landry)
Comment 27•12 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Comment 28•12 years ago
|
||
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 | ||
Comment 29•12 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
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Comment 30•12 years ago
|
||
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 31•12 years ago
|
||
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+
Comment 32•12 years ago
|
||
Comment 33•12 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•