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

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P2
normal
RESOLVED FIXED
5 months ago
11 days ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

(Blocks 3 bugs)

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Fission Milestone:M2, firefox68 fixed)

Details

Attachments

(8 attachments, 13 obsolete attachments)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Assignee

Description

5 months ago
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.
Assignee

Comment 1

5 months ago
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.
Assignee

Comment 2

5 months ago
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

Updated

5 months ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Assignee

Comment 3

5 months ago
This is to free users of the nsFontMetrics API from (indirectly) having to include gfxFont.h. (No functional change.)
Assignee

Comment 4

5 months ago
And this reduces include pain for the InspectorFontFace code. (No functional change.)
Assignee

Comment 5

5 months ago
Similarly, eliminate a troublesome include from CanvasRenderingContext2D.h. (No functional change.)
Assignee

Comment 6

5 months ago
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.)
Assignee

Comment 8

5 months ago
This makes it easier to change the type of the list in a subsequent patch. (no functional change here)
Assignee

Comment 9

5 months ago
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.
Assignee

Comment 10

5 months ago
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.
Assignee

Comment 11

5 months ago
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.
Assignee

Comment 12

5 months ago
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.
Assignee

Comment 13

5 months ago
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
Assignee

Updated

5 months ago
Attachment #9031980 - Attachment is obsolete: true
Assignee

Updated

5 months ago
Priority: -- → P2
Assignee

Updated

5 months ago
Attachment #9031962 - Flags: review?(jwatt)
Assignee

Updated

5 months ago
Attachment #9031963 - Flags: review?(jwatt)
Assignee

Updated

5 months ago
Attachment #9031968 - Flags: review?(jwatt)
Assignee

Updated

5 months ago
Attachment #9031971 - Flags: review?(jwatt)
Assignee

Updated

5 months ago
Attachment #9031974 - Flags: review?(jwatt)
Assignee

Updated

5 months ago
Attachment #9031975 - Flags: review?(jwatt)
Assignee

Updated

5 months ago
Attachment #9031976 - Flags: review?(jwatt)
Assignee

Updated

5 months ago
Attachment #9031977 - Flags: review?(jwatt)
Assignee

Updated

5 months ago
Attachment #9031978 - Flags: review?(jwatt)
Assignee

Comment 15

5 months ago
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+
Duplicate of this bug: 1522201

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
Assignee

Comment 28

4 months ago

(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.

Assignee

Comment 30

4 months ago

(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.

Assignee

Updated

2 months ago
Depends on: 1533395
Assignee

Updated

2 months ago
Depends on: 1533428
Assignee

Updated

2 months ago
Fission Milestone: --- → ?
Assignee

Updated

2 months ago
Depends on: 1533448
Assignee

Updated

2 months ago
Depends on: 1533462
Assignee

Updated

2 months ago
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.

Assignee

Comment 39

2 months ago
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
Assignee

Comment 40

2 months ago
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
Assignee

Updated

2 months ago
Attachment #9031963 - Attachment is obsolete: true
Assignee

Updated

2 months ago
Attachment #9031971 - Attachment is obsolete: true
Assignee

Updated

2 months ago
Attachment #9031974 - Attachment is obsolete: true
Assignee

Updated

2 months ago
Attachment #9031975 - Attachment is obsolete: true
Assignee

Updated

2 months ago
Attachment #9031976 - Attachment is obsolete: true
Assignee

Updated

2 months ago
Attachment #9031978 - Attachment is obsolete: true
Attachment #9031978 - Flags: review?(jwatt)
Assignee

Comment 41

2 months ago
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
Assignee

Updated

2 months ago
Attachment #9031981 - Attachment is obsolete: true
Assignee

Updated

2 months ago
Attachment #9031982 - Attachment is obsolete: true
Assignee

Updated

2 months ago
Attachment #9032120 - Attachment is obsolete: true

Updated

2 months ago
Fission Milestone: ? → M2

Updated

a month ago
Whiteboard: [4/11] patches under review

Comment 45

21 days ago
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

Updated

21 days ago
Whiteboard: [4/11] patches under review

Updated

20 days ago
Regressions: 1547914
Assignee

Updated

12 days ago
Blocks: 1550037
You need to log in before you can comment on or make changes to this bug.