1891986 - Loading font files from %windir%\Fonts via broker causes regressions on some cold performance tests.

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, enhancement, P2)

Description

[Bob Owen (:bobowen)]

As USER_RESTRICTED access token level removes read access to %windir%\Fonts, a rule has to be added to allow access via the broker.
This causes regressions for some cold performance tests, for example the outlook browsertime tests:
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=fb588ebf747a42c931bb15afd5f5a65bd97987d6&newProject=try&newRevision=06f5caabc590a43e8fd417b81a382fd6b74f17c3&framework=13&page=1&showOnlyImportant=1

Comment 1

[Bob Owen (:bobowen)]

jfkthame - I'm guessing one way to try and mitigate this is to preload some common fonts.
For the outlook tests the list appears to be segoeui.ttf, segoeuil.ttf, segoeuisl.ttf, seguisb.ttf, times.ttf.
If that list is going to be very variable and large then that solution might not be workable.
Although it looks like the files are shared across processes.

Each file that is opened will cause a sync call through the sandbox's shared memory IPC.
One other thing that might help is shunting the loading of the fonts off to another thread, but I don't know how feasible that would be in the layout code or how DirectWrite copes with multithreaded code.

Comment 2

[Jonathan Kew [:jfkthame]]

In general, layout initiates the loading of a font during reflow, when it actually needs it in some way (to retrieve metrics, measure text, etc). At that point, delegating the load to another thread isn't likely to be helpful, I suspect, as layout can't proceed until the font information is available, so it'll just block waiting for the font-load thread to finish -- or maybe it could proceed, using fallback fonts/metrics, but then we'll have to do an extra reflow when the desired font is available, so that's probably a net loss.

Preloading could be useful, if we can predict what fonts will be needed before reflow gets to the point of needing them, but that's very dependent on what pages are being loaded. Maybe it's worth trying to preload a couple of common defaults like Times and Arial, and perhaps SegoeUI, though if we're opening some arbitrary web site (rather than presenting a default home page or known testcase) there'll be a significant risk that these fonts aren't actually used by the page we're loading, and so preloading them represents redundant work (including i/o, which may be a bottleneck if it's competing with other disk access) and could itself regress overall perf.

So I'm not at all sure what's the best way to try and mitigate this. :(

Comment 3

[Bob Owen (:bobowen)]

After, trying to preload the font files during CSS parse and seeing no difference, I looked closer at the activity during the outlook tests and it seems that we are opening the TTF files multiple times.
Obviously this causes us quite a problem with each open requiring a sync round-trip to the parent.

Hacking the sandbox logging slightly, gave me stack of these and they all seem to be due to ReadFaceNamesForFamily.
Hacking a very crude fix that caches the font file handles and just duplicates it each time it is opened seems to mostly resolve the performance issue:
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=0aeec35547c9ed966782d9919deb94bb373ca7c5&newProject=try&newRevision=61126e48a434ea5b916332d2832c4a5dff6ee299&framework=13&page=2&showOnlyComparable=1

Looking at whether we can prevent the multiple loads in a more elegant way.

Comment 4

[Jonathan Kew [:jfkthame]]

Hmm, interesting. I assume ReadFaceNamesForFamily is being called for each font by the gfxPlatformFontList::LoadFontInfo() code that runs on a timer shortly after startup, to initialize all the metadata needed for @font-face { src:local(...) } lookups. We do that so that we'll be able to do font lookups during reflow more efficiently, without having to block on loading font metadata at that time (and without relying on platform font lookup APIs that don't exactly match CSS-specified behavior).

(But even if we didn't do that, we'd end up doing much of the same font access via other code paths, e.g. when we need to find a fallback font for some obscure character in the content.)

Which actual DirectWrite API(s) lead to the file-open happening -- is this triggered by IDWriteFontList::GetFont, or by IDWriteFont::CreateFontFace, or does it happen separately for every individual call such as IDWriteFontFace::TryGetFontTable and IDWriteFontFace::GetInformationalStrings? Depending on the granularity of these accesses (or what caching DW is doing internally of the file handle), that might affect our options here.

Would holding onto handles to all the font files (which could be several thousand) be a problem in terms of resource usage? I guess it might also block users from modifying/uninstalling font files while the browser is running, which could be annoying.

Comment 5

[Bob Owen (:bobowen)]

(In reply to Jonathan Kew [:jfkthame] from comment #4)

Hmm, interesting. I assume ReadFaceNamesForFamily is being called for each font by the gfxPlatformFontList::LoadFontInfo() code that runs on a timer shortly after startup, to initialize all the metadata needed for @font-face { src:local(...) } lookups. We do that so that we'll be able to do font lookups during reflow more efficiently, without having to block on loading font metadata at that time (and without relying on platform font lookup APIs that don't exactly match CSS-specified behavior).

I think I have seen that, but for the outlook test, the stacks are multiples of the following per SEG*.TTF file (wrapped for brevity, I was logging out 25 from the file open) :
gfxDWriteFontList::ReadFaceNamesForFamily, mozilla::fontlist::FontList::FindFamily, gfxPlatformFontList::FindAndAddFamiliesLocked, gfxDWriteFontList::FindAndAddFamiliesLocked, gfxPlatformFontList::FindAndAddFamilies, gfxFontGroup::AddPlatformFont, gfxFontGroup::BuildFontList, gfxFontGroup::gfxFontGroup, nsFontMetrics::nsFontMetrics, nsFontCache::GetMetricsFor, nsLayoutUtils::GetFontMetricsForComputedStyle, nsLayoutUtils::GetFontMetricsForFrame

and then later:
gfxDWriteFontList::ReadFaceNamesForFamily, mozilla::fontlist::FontList::FindFamily, gfxPlatformFontList::FindAndAddFamiliesLocked, gfxDWriteFontList::FindAndAddFamiliesLocked, gfxPlatformFontList::FindAndAddFamilies, gfxFontGroup::AddPlatformFont, gfxFontGroup::BuildFontList, gfxFontGroup::UpdateUserFonts, nsFontCache::UpdateUserFonts, mozilla::ServoStyleSet::PreTraverseSync, mozilla::ServoStyleSet::StyleDocument, mozilla::RestyleManager::DoProcessPendingRestyles

(But even if we didn't do that, we'd end up doing much of the same font access via other code paths, e.g. when we need to find a fallback font for some obscure character in the content.)

Which actual DirectWrite API(s) lead to the file-open happening -- is this triggered by IDWriteFontList::GetFont, or by IDWriteFont::CreateFontFace, or does it happen separately for every individual call such as IDWriteFontFace::TryGetFontTable and IDWriteFontFace::GetInformationalStrings? Depending on the granularity of these accesses (or what caching DW is doing internally of the file handle), that might affect our options here.

Would holding onto handles to all the font files (which could be several thousand) be a problem in terms of resource usage? I guess it might also block users from modifying/uninstalling font files while the browser is running, which could be annoying.

It seems to be the TryGetFontTable that is the main culprit.
I notice that most of them seem to use gfxFontEntry::AutoTable, which caches to table data.
Possibly this also causes DirectWrite to cache the FontFileSream.
I can see some sort of caching in the disassembled DWrite code.
We don't have a gfxFontEntry in ReadFaceNamesForFamily.

Also, from your comments, I'm guessing there aren't any DirectWrite APIs (like GetFamilyNames) that can get the information we want. It's possible that they might avoid opening the file by using the data from DirectWrite's own cache (which we've now got working even in this locked down state \o/).

Comment 6

[Bob Owen (:bobowen)]

Attachment: Bug 1891986: Only read font faces names once per base family. r=jfkthame!

This prevents extra file I/O when reading the name table for the font.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a5dfeadfa976
Only read font faces names once per base family. r=jfkthame

Comment 8

[Sandor Molnar[:smolnar]]

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