1314932 - Improve gecko startup for content process by avoiding CTFontManagerCopyAvailableFontFamilyNames

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Jonathan Kew [:jfkthame]]

During startup, we set up our list of available font families using gfxPlatformFontList::InitFontList(), which calls a platform-specific InitFontListForPlatform() method to query the available fonts.

It turns out that on OS X, this is quite expensive; it calls a Core Text function CTFontManagerCopyAvailableFontFamilyNames that takes at least 10 ms on my MacBook Pro running 10.11, for example. Altogether, InitFontList() ends up taking 20+ ms.

Telemetry suggests that this is comparable to what our users are seeing in the wild: https://telemetry.mozilla.org/new-pipeline/evo.html#!aggregates=median&cumulative=0&end_date=null&keys=&max_channel_version=nightly%252F52&measure=MAC_INITFONTLIST_TOTAL&min_channel_version=nightly%252F49&os=Darwin&product=Firefox&sanitize=1&sort_keys=submissions&start_date=null&trim=1&use_submission_date=0

When e10s is enabled, we pay this cost in _both_ the chrome and content processes, which suggests that the overall startup time for Firefox with e10s on Mac OS X typically includes around 50-60 ms to enumerate the available fonts.

We should be able to save somewhere in the region of 10-15 ms in the content process if we make it fetch the list of available fonts from its parent chrome process via IPC, instead of calling Core Text again.

(FWIW, we do something similar to this on Android/B2G already, in gfxFT2FontList.)

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Let content process on OS X get the list of installed font families from the chrome process, rather than querying Core Text again

On my MBPro, this shaves around 15ms off the startup time of the content process. (I'm not sure if any of our existing talos measurements will reflect this, but it's measurable locally by instrumenting InitFontList(), and should be significant enough show up clearly in telemetry graphs once it's in Nightly builds.)

Comment 2

[Markus Stange [:mstange]]

Comment on attachment 8807220 [details] [diff] [review]
Let content process on OS X get the list of installed font families from the chrome process, rather than querying Core Text again

Review of attachment 8807220 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxMacPlatformFontList.mm
@@ +761,5 @@
> +        // Content process: ask the Chrome process to give us its list,
> +        // because it's 10x faster than querying Core Text.
> +        InfallibleTArray<FontFamilyListEntry> families;
> +        mozilla::dom::ContentChild::GetSingleton()->
> +            SendReadFontFamilyList(&families);

Hmm, so depending on what the parent process is busy with, this sync message could also take a long time.

It would be nicer to have the parent process send along the font list as it starts the content process. I don't know how easy it would be to do that.

::: gfx/thebes/gfxPlatformMac.h
@@ +18,5 @@
>  } // namespace mozilla
>  
> +namespace mozilla {
> +    namespace dom {
> +        class FontFamilyListEntry;

We usually don't indent for namespaces in forward declarations.

Comment 3

[Markus Stange [:mstange]]

Mike, what do you think about the sync message this patch adds?

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to Markus Stange [:mstange] from comment #2)
> Comment on attachment 8807220 [details] [diff] [review]
> Let content process on OS X get the list of installed font families from the
> chrome process, rather than querying Core Text again
> 
> Review of attachment 8807220 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxMacPlatformFontList.mm
> @@ +761,5 @@
> > +        // Content process: ask the Chrome process to give us its list,
> > +        // because it's 10x faster than querying Core Text.
> > +        InfallibleTArray<FontFamilyListEntry> families;
> > +        mozilla::dom::ContentChild::GetSingleton()->
> > +            SendReadFontFamilyList(&families);
> 
> Hmm, so depending on what the parent process is busy with, this sync message
> could also take a long time.

True; though if the chrome process is too busy to handle the message promptly, we're probably janking badly enough that content process startup time is less of an issue. In my instrumented experimentation, I've been consistently seeing sub-millisecond times for this (whereas calling Core Text takes over 10 ms), so it seems like a pretty reliable win.

> It would be nicer to have the parent process send along the font list as it
> starts the content process. I don't know how easy it would be to do that.

Perhaps a followup, if we can figure it out? (Offhand, I don't know how that would be done.)

Comment 5

[Markus Stange [:mstange]]

Sounds reasonable. So yeah, let's land this improvement as is.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

I think we should be avoiding the "send sync message soon after content process start-time" anti-pattern as much as possible. We're currently in the process of trying to get rid of a bunch of them in bug 1303096. If possible, could you piggyback on that work instead?

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Let content process on OS X get the list of installed font families from the chrome process, rather than querying Core Text again

Argh, sorry... I just realized that I posted an earlier version of this patch, not the final one I intended. After the call to CTFontManagerCopyAvailableFontFamilyNames(), the next most expensive part of InitFontList() is InitSystemFontNames(), which resolves and stores the text- and display-size system font names. We can save several ms extra by adding a couple of entries to the list we pass over from the parent process, so that we avoid re-resolving the system font in the child. Marking r? again as this is a significant change to the patch that was originally reviewed.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Instead of adding a new sync message, is it possible to piggy back ontop of a pre-existing sync message? Like this one?: http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/dom/ipc/PContent.ipdl#683

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8807294 [details] [diff] [review]
Let content process on OS X get the list of installed font families from the chrome process, rather than querying Core Text again

I've changed my mind and would prefer to not land this until we've removed the extra sync message.

Comment 10

[Jonathan Kew [:jfkthame]]

(In reply to Mike Conley (:mconley) - (digging out of review / needinfo backlog) from comment #8)
> Instead of adding a new sync message, is it possible to piggy back ontop of
> a pre-existing sync message? Like this one?:

I guess we probably could, though that would mean giving it extra return values that are used for a single platform only; does that matter? Or we could add platform-specific additional return values in the .ipdl, guarded by #ifdef or something.... but AFAICT that's not currently supported in ipdl, is it?

Comment 11

[Mike Conley (:mconley) (:⚙️)]

(In reply to Jonathan Kew (:jfkthame) from comment #10)
> (In reply to Mike Conley (:mconley) - (digging out of review / needinfo
> backlog) from comment #8)
> > Instead of adding a new sync message, is it possible to piggy back ontop of
> > a pre-existing sync message? Like this one?:
> 
> I guess we probably could, though that would mean giving it extra return
> values that are used for a single platform only; does that matter?

It's probably fine. This GetXPCOMProcessAttributes message is one we're hoping to get rid of anyways, so I think it'd be better to piggyback onto that one instead of adding a new one. I think sending empty an empty array down for the unaffected platforms is probably fine.

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: Reduce content-process startup time on MacOSX by passing the system font list from chrome to content process via the GetXPCOMProcessAttributes message

Here's a version that piggy-backs on GetXPCOMProcessAttributes, as suggested, to avoid adding a new sync ipc message.

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 8810463 [details] [diff] [review]
Reduce content-process startup time on MacOSX by passing the system font list from chrome to content process via the GetXPCOMProcessAttributes message

Review of attachment 8810463 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: gfx/thebes/gfxMacPlatformFontList.mm
@@ +739,5 @@
> +    // Note: We rely on the records for mSystemTextFontFamilyName and
> +    // mSystemDisplayFontFamilyName (if present) being *before* the main
> +    // font list, so that those names are known in the content process
> +    // by the time we add the actual family records to the font list.
> +    aList->AppendElement(FontFamilyListEntry(mSystemTextFontFamilyName,

Does this compile? Doesn't it need to be FontFamilyListEntry { ... } with curly braces?

@@ +740,5 @@
> +    // mSystemDisplayFontFamilyName (if present) being *before* the main
> +    // font list, so that those names are known in the content process
> +    // by the time we add the actual family records to the font list.
> +    aList->AppendElement(FontFamilyListEntry(mSystemTextFontFamilyName,
> +                                             true, true, false));

I'd be much happier if this used enums and a bit field, instead of a list of booleans.

Comment 14

[Jonathan Kew [:jfkthame]]

(In reply to Markus Stange [:mstange] from comment #13)
> > +    aList->AppendElement(FontFamilyListEntry(mSystemTextFontFamilyName,
> 
> Does this compile? Doesn't it need to be FontFamilyListEntry { ... } with
> curly braces?

No, this seems to work fine (and follows the pattern we have in some other messages). I guess the IPDL types generate the necessary constructors/assignments to let this work.

> 
> @@ +740,5 @@
> > +    // mSystemDisplayFontFamilyName (if present) being *before* the main
> > +    // font list, so that those names are known in the content process
> > +    // by the time we add the actual family records to the font list.
> > +    aList->AppendElement(FontFamilyListEntry(mSystemTextFontFamilyName,
> > +                                             true, true, false));
> 
> I'd be much happier if this used enums and a bit field, instead of a list of
> booleans.

Ah, I thought you might say that! Sorry, I was a bit lazy..... but OK, I'll update it.

Comment 15

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc0e63d09f83724c32ac7abe92c75b041b1c9b03
Bug 1314932 - Reduce content-process startup time on MacOSX by passing the system font list from chrome to content process via the GetXPCOMProcessAttributes message. r=mstange

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8a81bf64c4e
Backed out changeset bc0e63d09f83 for causing merge conflicts with mozilla-central/autoland and so blocking m-i to m-c merge

Comment 17

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/efe1e3b8cc1714dbdc456b80dfa4578a5192f5b7
Bug 1314932 - Reduce content-process startup time on MacOSX by passing the system font list from chrome to content process via the GetXPCOMProcessAttributes message. r=mstange

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/efe1e3b8cc17