Closed Bug 1514869 Opened 6 years ago Closed 6 years ago

Share the platform font list and associated metadata/character maps between processes

Categories

(Core :: Layout: Text and Fonts, enhancement, P2)

Product:
Core
Component:
Layout: Text and Fonts
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68
Project Flags:
Fission Milestone M2
Tracking Flags:
Tracking Status
firefox68 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Blocks 3 open bugs)

Details

Attachments

(8 files, 13 obsolete files)

Bug 1514869 - patch 1 - Basic implementation of a cross-process sharable font list, using shared memory to store the list of families & faces, and per-font character maps. r?jwatt
(deleted), text/x-phabricator-request
Details
Bug 1514869 - patch 2 - Adapt platform-font-list code to work with either the existing in-process font list or cross-process shared font list. r?jwatt
(deleted), text/x-phabricator-request
Details
Bug 1514869 - patch 3 - Implement macOS backend for the shared font list. r?jwatt
(deleted), text/x-phabricator-request
Details
Bug 1514869 - patch 4 - Implement Linux/fontconfig backend for the shared font list. r?jwatt
(deleted), text/x-phabricator-request
Details
Bug 1514869 - patch 5 - Implement DirectWrite backend for the shared font list. r?jwatt
(deleted), text/x-phabricator-request
Details
Bug 1514869 - patch 6 - Hook up SetupFamilyCharMap for shared font-list Family records, to accelerate last-ditch fallback searches. r?jwatt
(deleted), text/x-phabricator-request
Details
Bug 1514869 - patch 7 - Check font families for "simple" set of faces, and mark the Family record appropriately so we can use simplified style-matching. r?jwatt
(deleted), text/x-phabricator-request
Details
Bug 1514869 - patch 8 - Make the SetCharacterMap message async, and use the unshared gfxCharacterMap in the content process until the shared one is in place. r?jwatt
(deleted), text/x-phabricator-request
Details
Rather than having each process maintain its own list of all the installed fonts, with the metadata needed to support the font-matching algorithm, fallback, etc., let's have a single copy of this data in shared memory for all processes to use.
The implementation here requires shuffling some existing headers & code around because of build issues; in particular, I ran into problems on Windows because the generated DOM bindings code (and some other places, but this was the main culprit) refuses to build if windows.h has been #included (however indirectly) by any headers that it uses. The shared-memory code on Windows requires windows.h (unsurprisingly); when shared-memory-related declarations start to find their way into gfxPlatformFontList.h, which in turn is used by gfxFont.h and gfxTextRun.h, sadness follows. So there are a number of patches here that make no functional change, but just move declarations around so as to reduce the #include entanglement. A further complication is that I think we should initially implement this behind a pref, as it's a radical enough change that some cautious testing/roll-out may be in order; but this means that a bunch of gfxPlatformFontList methods will in effect have two implementations for now, one that works with the existing in-process font list, and a separate code path to work with the shared font list.
Trivial patch to avoid a build issue I ran into later due to the use of 'using' in a header, which is fragile in the presence of unified compilation.
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
This is to free users of the nsFontMetrics API from (indirectly) having to include gfxFont.h. (No functional change.)
And this reduces include pain for the InspectorFontFace code. (No functional change.)
Similarly, eliminate a troublesome include from CanvasRenderingContext2D.h. (No functional change.)
More use of IPC shared-memory code will mean more directories need to see the associated headers. (I wouldn't be surprised if this needs further refinement in due course, but for now it's enough for everything to build.)
This makes it easier to change the type of the list in a subsequent patch. (no functional change here)
We can't use pointer-heavy types like gfxSharedBitSet in shared memory that may be mapped at different addresses, so here's a version that doesn't depend on internal pointers. Not yet connected to anything, that's coming in a couple patches time.
As we can't put regular C++ objects like gfxFontFamily, gfxFontEntry and their subclasses into a shared-memory list, we'll have separate types there; so then code that looks up font families, etc., needs to be able to work with either type. So we wrap a pointer to either gfxFontFamily or fontlist::Family in a tagged union for convenience.
This implements a list of Family records, each with a set of Face records, and the associated data (names, properties, etc) in a shared-memory area. Because we can't know ahead of time how much space we'll need (not even approximately), it allocates blocks of shared memory and then doles out pieces of these as needed. Because we only ever add data to the list, never modify/delete what's already there, we don't need a full memory manager in the shared area; we just allocate chunks sequentially until each block is full, then start a new one. Only the parent process creates new blocks; child processes have to ask the parent for a handle to the block. This is (unfortunately) a sync message, but should be rare enough to not be a problem.
This adapts the key font-list methods to work with a shared list, if available; note that no back-end yet creates one, though, so this will not yet change any actual behavior.
Finally, we connect up a platform backend, so that the macOS implementation of gfxPlatformFontList can create and use a shared list (subject to the gfx.e10s.font-list.shared pref). Not too much orange on try, and what's there looks like it may be unrelated (will test further): https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a99ef5aece366af141c4e44732343bef0c6c695
Attachment #9031980 - Attachment is obsolete: true
Priority: -- → P2
Attachment #9031962 - Flags: review?(jwatt)
Attachment #9031963 - Flags: review?(jwatt)
Attachment #9031968 - Flags: review?(jwatt)
Attachment #9031971 - Flags: review?(jwatt)
Attachment #9031974 - Flags: review?(jwatt)
Attachment #9031975 - Flags: review?(jwatt)
Attachment #9031976 - Flags: review?(jwatt)
Attachment #9031977 - Flags: review?(jwatt)
Attachment #9031978 - Flags: review?(jwatt)
I have DWrite and Linux versions of patch 12 in progress, but not yet complete; also not marking r? on patches 10 and 11 yet as I suspect they may get some minor adjustments in the course of working through the other platform implementations.
Comment on attachment 9031962 [details] [diff] [review] patch 1 - Use fully-qualified name for mozilla::ipc::FileDescriptor in AutoMemMap.h, rather than depending on a 'using' declaration Review of attachment 9031962 [details] [diff] [review]: ----------------------------------------------------------------- To avoid unnecessary verbosity in headers we generally handle this by adding a typedef to the class declaration. I.e. in this case we'd add the following after the "public:" line: typedef mozilla::ipc::FileDescriptor FileDescriptor; Unless you have a reason not to do that?
Attachment #9031962 - Flags: review?(jwatt) → review+
Comment on attachment 9031963 [details] [diff] [review] patch 2 - Move the Orientation enum from gfxFont to nsFontMetrics to enable some #include-elimination, in particular to avoid including gfxTextRun.h in nsFontMetrics.h Review of attachment 9031963 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/src/nsFontMetrics.h @@ +27,5 @@ > > +namespace mozilla { > +namespace gfx { > +class DrawTarget; > +} Curious; does this not require a trailing comment too?
Attachment #9031963 - Flags: review?(jwatt) → review+
Comment on attachment 9031968 [details] [diff] [review] patch 3 - Move the gfxTextRange struct from gfxFont.h to InspectorFontFace.h, so that we can avoid including gfxFont.h here Review of attachment 9031968 [details] [diff] [review]: ----------------------------------------------------------------- Maybe giving it its own header would be less weird that locating it in InspectorFontFace.h?
Attachment #9031968 - Flags: review?(jwatt) → review+
Comment on attachment 9031971 [details] [diff] [review] patch 4 - Avoid including gfxTextRun.h in the CanvasRenderingContext2d.h header Review of attachment 9031971 [details] [diff] [review]: ----------------------------------------------------------------- (It would be nice to extend the comment to mention which members require gfxTextRun.h)
Attachment #9031971 - Flags: review?(jwatt) → review+
Attachment #9031974 - Flags: review?(jwatt) → review+
Attachment #9031975 - Flags: review?(jwatt) → review+
Attachment #9031976 - Flags: review?(jwatt) → review+
Attachment #9031977 - Flags: review?(jwatt) → review+

Jonathan, I've been assuming since it looks like you've not had time to finish off the remaining patches that there's no rush on the outstanding review request. Let me know if that's wrong.

Also, it might be worth landing the r+'ed patches and tagging this bug as leave-open to avoid bitrot causing you unnecessary work.

Flags: needinfo?(jfkthame)

(In reply to Jonathan Kew (:jfkthame) from comment #26)

https://treeherder.mozilla.org/#/
jobs?repo=try&revision=c5618931d0bc7715996c75456686c29afcfcc609

Just a heads up, with webrender enabled the font memory seems to be going crazy on that push:

GPU (pid 7904)
Explicit Allocations

1,355.53 MB (100.0%) -- explicit
├──1,329.33 MB (98.07%) -- gfx/webrender
│  ├──1,325.30 MB (97.77%) -- resource-cache
│  │  ├──1,325.30 MB (97.77%) ── fonts

(In reply to Eric Rahm [:erahm] (ni? for phab reviews) from comment #27)

(In reply to Jonathan Kew (:jfkthame) from comment #26)

https://treeherder.mozilla.org/#/
jobs?repo=try&revision=c5618931d0bc7715996c75456686c29afcfcc609

Just a heads up, with webrender enabled the font memory seems to be going crazy on that push:

I suspect that was because it was based on a slightly old tree, and the issue there has since been fixed.

Here's a newer try run that shouldn't have such alarming behavior:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b0b642bf1a518be6ade444cf232ee52ca2c8ccb

Flags: needinfo?(jfkthame)

The awsy numbers are looking really good -- I triggered a few more awsy runs to get better confidence. OSX hasn't come in yet but the linux64 result is super impressive (~2MB). Win64 is in line with what we expected from measurements in bug 1470015 (~200KB).

Win32 looks like a giant win, I think this might explain our mysterious bug 1483414 where the measured memory in automation didn't sync up with what we observed running manually (and on loaners). I'm not sure why poking fonts would cause a 30MB regression but here we are.

The qr regression on windows only is still pretty bad on the latest push. Seems like some bad behavior related to the gpu process.

(In reply to Eric Rahm [:erahm] (ni? for phab reviews) from comment #29)

Win32 looks like a giant win,

Yeah, although note that on Win32 there are currently a lot of test failures that appear to be related to failure to load expected fonts, so it's possible we'll lose some of that "giant win" once it actually works properly there.

Depends on: 1533395
Depends on: 1533428
Fission Milestone: --- → ?
Depends on: 1533448
Depends on: 1533462
Blocks: 1533462
No longer depends on: 1533462

It was suggested that this might be related to https://bugzilla.mozilla.org/show_bug.cgi?id=1468889 If so I'm hoping this fix can reach the next Firefox version and close that as well.

I experience a roughly 1 second delay whenever opening a new tab, if my number of existing tabs is lower than the maximum number of processes Firefox may spawn. The analysis I submitted at the time suggested this might be related to libfontconfig.

Comment on attachment 9031968 [details] [diff] [review] patch 3 - Move the gfxTextRange struct from gfxFont.h to InspectorFontFace.h, so that we can avoid including gfxFont.h here Moved this to bug 1533395.
Attachment #9031968 - Attachment is obsolete: true
Comment on attachment 9031962 [details] [diff] [review] patch 1 - Use fully-qualified name for mozilla::ipc::FileDescriptor in AutoMemMap.h, rather than depending on a 'using' declaration Moved a bunch of the preparatory patches here to bug 1533428.
Attachment #9031962 - Attachment is obsolete: true
Attachment #9031963 - Attachment is obsolete: true
Attachment #9031971 - Attachment is obsolete: true
Attachment #9031974 - Attachment is obsolete: true
Attachment #9031975 - Attachment is obsolete: true
Attachment #9031976 - Attachment is obsolete: true
Attachment #9031978 - Attachment is obsolete: true
Attachment #9031978 - Flags: review?(jwatt)
Comment on attachment 9031977 [details] [diff] [review] patch 8 - Provide a version of gfxSparseBitSet that is better suited to shared-memory use This new class is now moved to bug 1533448.
Attachment #9031977 - Attachment is obsolete: true
Attachment #9031981 - Attachment is obsolete: true
Attachment #9031982 - Attachment is obsolete: true
Attachment #9032120 - Attachment is obsolete: true
Fission Milestone: ? → M2
Whiteboard: [4/11] patches under review
Pushed by jkew@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/21ef00977ab6 patch 1 - Basic implementation of a cross-process sharable font list, using shared memory to store the list of families & faces, and per-font character maps. r=jwatt,jld https://hg.mozilla.org/integration/autoland/rev/095b3edec3c8 patch 2 - Adapt platform-font-list code to work with either the existing in-process font list or cross-process shared font list. r=jwatt https://hg.mozilla.org/integration/autoland/rev/c154853c599a patch 3 - Implement macOS backend for the shared font list. r=jwatt https://hg.mozilla.org/integration/autoland/rev/d51c979e9930 patch 4 - Implement Linux/fontconfig backend for the shared font list. r=jwatt https://hg.mozilla.org/integration/autoland/rev/ba0672dcd82d patch 5 - Implement DirectWrite backend for the shared font list. r=jwatt https://hg.mozilla.org/integration/autoland/rev/0bad995bae22 patch 6 - Hook up SetupFamilyCharMap for shared font-list Family records, to accelerate last-ditch fallback searches. r=jwatt https://hg.mozilla.org/integration/autoland/rev/b3fbab6d325a patch 7 - Check font families for "simple" set of faces, and mark the Family record appropriately so we can use simplified style-matching. r=jwatt https://hg.mozilla.org/integration/autoland/rev/7de7d6a0be86 patch 8 - Make the SetCharacterMap message async, and use the unshared gfxCharacterMap in the content process until the shared one is in place. r=jwatt,jld
Whiteboard: [4/11] patches under review
Regressions: 1547914
Blocks: 1550037
Blocks: 1552941
Blocks: 1554208
Depends on: 1554819
Regressions: 1553228

@Jonathan, this fix landed two uses of the the MOZ_CONTENT_SANDBOX macro in gfx/thebes/gfxFcPlatformFontList.cpp, but that macro was removed and folded into MOZ_SANDBOX just before in bug 1375863.

https://searchfox.org/mozilla-central/rev/4d2b1f753871ce514f9dccfc5b1b5e867f499229/gfx/thebes/gfxFcPlatformFontList.cpp#1691
https://searchfox.org/mozilla-central/rev/4d2b1f753871ce514f9dccfc5b1b5e867f499229/gfx/thebes/gfxFcPlatformFontList.cpp#1850

Flags: needinfo?(jfkthame)
Blocks: 1815905

Created bug 1815905 for comment 47.

Flags: needinfo?(jfkthame)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: