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

Status: RESOLVED FIXED

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

Description

[Jonathan Kew [:jfkthame]]

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.

Comment 1

[Jonathan Kew [:jfkthame]]

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.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: patch 1 - Use fully-qualified name for mozilla::ipc::FileDescriptor in AutoMemMap.h, rather than depending on a 'using' declaration

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.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: 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

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

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: patch 3 - Move the gfxTextRange struct from gfxFont.h to InspectorFontFace.h, so that we can avoid including gfxFont.h here

And this reduces include pain for the InspectorFontFace code. (No functional change.)

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: patch 4 - Avoid including gfxTextRun.h in the CanvasRenderingContext2d.h header

Similarly, eliminate a troublesome include from CanvasRenderingContext2D.h. (No functional change.)

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: patch 5 - Include chromium-config.mozbuild in the moz.build for a bunch more directories, so that IPC-related headers are available

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

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: patch 6 - Move the helper functions that measure the 'distance' between values of font properties (weight, stretch, style) to gfxFontUtils.h so they can be used by the shared font list code

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: patch 7 - Consistently use the gfxPlatformFontList::PrefFontList type instead of repeatedly spelling out the array declaration

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

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: patch 8 - Provide a version of gfxSparseBitSet that is better suited to shared-memory use

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.

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: patch 9 - Create a FontFamily type that wraps either a gfxFontFamily pointer or a mozilla::fontlist::Family pointer (to be used by the shared font list), and use this in various places where we need to pass families around

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.

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: patch 10 - Basic implementation of a cross-process sharable font list, using shared memory to store the list of families & faces, and per-font character maps

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.

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: patch 11 - Adapt platform-font-list code to work with either the existing in-process font list or cross-process shared font list

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.

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: patch 12 - Implement macOS backend for the shared font list

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

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: patch 10 - Basic implementation of a cross-process sharable font list, using shared memory to store the list of families & faces, and per-font character maps

Comment 15

[Jonathan Kew [:jfkthame]]

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 16

[Jonathan Watt [:jwatt]]

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?

Comment 17

[Jonathan Watt [:jwatt]]

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?

Comment 18

[Jonathan Watt [:jwatt]]

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?

Comment 19

[Jonathan Watt [:jwatt]]

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)

Comment 20

[Jonathan Kew [:jfkthame]]

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

Comment 21

[Jonathan Kew [:jfkthame]]

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

Comment 22

[Jonathan Kew [:jfkthame]]

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

Comment 24

[Jonathan Kew [:jfkthame]]

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

Comment 25

[Jonathan Watt [:jwatt]]

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.

Comment 26

[Jonathan Kew [:jfkthame]]

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

Comment 27

[Eric Rahm [:erahm]]

(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

Comment 28

[Jonathan Kew [:jfkthame]]

(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

Comment 29

[Eric Rahm [:erahm]]

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.

Comment 30

[Jonathan Kew [:jfkthame]]

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

Comment 31

[Jonathan Kew [:jfkthame]]

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

Comment 32

[Jonathan Kew [:jfkthame]]

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

Comment 33

[[e-mail]]

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 34

[Jonathan Kew [:jfkthame]]

Attachment: 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

Depends on D22936

Comment 35

[Jonathan Kew [:jfkthame]]

Attachment: 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

Depends on D22937

Comment 36

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1514869 - patch 3 - Implement macOS backend for the shared font list. r?jwatt

Depends on D22938

Comment 37

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1514869 - patch 4 - Implement Linux/fontconfig backend for the shared font list. r?jwatt

Depends on D22939

Comment 38

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1514869 - patch 5 - Implement DirectWrite backend for the shared font list. r?jwatt

Depends on D22940

Comment 39

[Jonathan Kew [:jfkthame]]

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.

Comment 40

[Jonathan Kew [:jfkthame]]

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.

Comment 41

[Jonathan Kew [:jfkthame]]

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.

Comment 42

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1514869 - patch 6 - Hook up SetupFamilyCharMap for shared font-list Family records, to accelerate last-ditch fallback searches. r?jwatt

Depends on D22941

Comment 43

[Jonathan Kew [:jfkthame]]

Attachment: 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

Depends on D23355

Comment 44

[Jonathan Kew [:jfkthame]]

Attachment: 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

Comment 45

[Pulsebot]

Pushed by [e-mail]:
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

Comment 46

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/21ef00977ab6
https://hg.mozilla.org/mozilla-central/rev/095b3edec3c8
https://hg.mozilla.org/mozilla-central/rev/c154853c599a
https://hg.mozilla.org/mozilla-central/rev/d51c979e9930
https://hg.mozilla.org/mozilla-central/rev/ba0672dcd82d
https://hg.mozilla.org/mozilla-central/rev/0bad995bae22
https://hg.mozilla.org/mozilla-central/rev/b3fbab6d325a
https://hg.mozilla.org/mozilla-central/rev/7de7d6a0be86

Comment 47

[Haik Aftandilian [:haik]]

@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

Comment 48

[Jonathan Kew [:jfkthame]]

Created bug 1815905 for comment 47.