In bug 1314932, we improved the startup time of content processes on macOS by avoiding the (expensive) call to retrieve the list of installed fonts from Core Text, and instead passed the list from the chrome process to content. This gave us an improvement of typically around 15ms on content process initialization for Firefox 53. However, bug 1303096 (which landed for Firefox 54) broke this; since it landed, the child process no longer uses the font list passed by chrome, but again ends up calling Core Text itself. The original improvement (early in the 53 cycle, 17 Nov 2016) and subsequent regression (1/3 through 54, 8 Feb 2017) can clearly be seen in the InitFontList telemetry: https://telemetry.mozilla.org/new-pipeline/evo.html#!aggregates=median!5th-percentile!95th-percentile&cumulative=0&e10s=true&end_date=null&keys=&max_channel_version=nightly%252F57&measure=MAC_INITFONTLIST_TOTAL&min_channel_version=nightly%252F51&os=Darwin&processType=content&product=Firefox&sanitize=1&sort_keys=submissions&start_date=null&trim=1&use_submission_date=0 Code from bug 1314932 that landed to implement this is still present in the tree, but apparently isn't being used. (Or is being used unsuccessfully: actually, we still call gfxPlatform::GetSystemFontFamilyList() to get the list that's supposed to be passed to the new child process, but then we fail to use it and the child falls back to the Core Text API.)

(In reply to Jonathan Kew (:jfkthame) from comment #0) > Code from bug 1314932 that landed to implement this is still present in the > tree, but apparently isn't being used. (Or is being used unsuccessfully: > actually, we still call gfxPlatform::GetSystemFontFamilyList() to get the > list that's supposed to be passed to the new child process, but then we fail > to use it and the child falls back to the Core Text API.) Do we know why? Is there someone (other than the usual person I'd ask for this type of situation: you :) who should investigate?

Presumably the people who were involved in bug 1303096, where this got revised (and broke) might be able to comment further. Without having studied that bug in detail, my guess is that previously, the font list was sent to the child using a sync IPC message; now, it's done async, but what ends up happening is that it isn't received/processed in time for when gfxMacPlatformFontList::InitFontListForPlatform() needs it (during gfxPlatform initialization). In a content process, InitFontListForPlatform() tries to use a list provided as ContentChild::SystemFontFamilyList(), but if that's empty (which it will be if the async message hasn't been received yet, presumably) then it falls back to the brute-force Core Text approach. Given that the SystemFontFamilyList() is needed early in platform startup, I suspect we need to go back to using a sync IPC message, or some other (sync) method to pass it early. If we don't receive it until after the child has initialized its gfxPlatform, it's too late to do any good.

Actually, I think it's simpler than that. AFAICS the problem is just that ContentChild::RecvSetXPCOMProcessAttributes doesn't handle the list (although it's successfully passed in XPCOMInitData.fontFamilies), so it gets dropped on the floor.

This is essentially a followup to finish up an incomplete aspect of bug 1303096. Can you review this one, as :billm is apparently away at the moment? Thanks.

Comment on attachment 8907793 [details] [diff] [review] Use font family list passed from the chrome process to initialize the platform font family list on macOS, to avoid expensive Core Text font iteration code path Review of attachment 8907793 [details] [diff] [review]: ----------------------------------------------------------------- Tentative r+ - though if we can keep the FontFamilyListEntry member in the struct, I think that makes more sense (since it keeps more of the init data together). ::: dom/ipc/ContentChild.cpp @@ +563,3 @@ > { > mLookAndFeelCache = aLookAndFeelIntCache; > + mFontFamilies = aFontFamilyList; This seems like the most important part, I guess. Why, out of curiosity, is it necessary to extract the FontFamilyListEntry member from the XPCOMInitData struct?

(In reply to Mike Conley (:mconley) (:⚙️) from comment #5) > This seems like the most important part, I guess. Why, out of curiosity, is > it necessary to extract the FontFamilyListEntry member from the > XPCOMInitData struct? Indeed, that's the real fix. Separating out the nsTArray<FontFamilyListEntry> isn't strictly necessary; it was mostly done to parallel the treatment of the other array (aLookAndFeelIntCache) here. Looking back at the reasoning given for that in bug 1303096 comment 24, though, I don't think it's applicable here; we could equally well leave it in the XPCOMInitData struct and it builds without complaint. However, there's another reason to pass the arrays as separate (rvalue) parameters here: this allows us to Move() them instead of copying, when assigning to the members in ContentChild::RecvSetXPCOMProcessAttributes. I omitted to do that in the posted patch (and the original code doesn't take advantage of it for aLookAndFeelIntCache), but I think we should add it; aFontFamilyList, in particular, is likely to be an array of several hundred records, each containing a string (plus a couple other flags), so there will be a significant benefit to moving rather than copying. We can't do that if the arrays are embedded in the XPCOMInitData struct (passed by const reference). So that suggests we should do ::: dom/ipc/ContentChild.cpp @@ +563,3 @@ > { > - mLookAndFeelCache = aLookAndFeelIntCache; > + mLookAndFeelCache = Move(aLookAndFeelIntCache); > + mFontFamilies = Move(aFontFamilyList); here. I've checked that this builds OK (and results in code that uses SwapElements on both the arrays). Can I assume you're happy for me to carry forward r+ with that adjustment?

Yessir! Thanks!

Pushed by jkew@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0bdbc08e691c Use font family list passed from the chrome process to initialize the platform font family list on macOS, to avoid expensive Core Text font iteration code path. r=mconley