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

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

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

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

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

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?

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.

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

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

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

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

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.

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

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

Backout by cbook@mozilla.com: 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

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