1490792 - 140ms of FontFace::DoLoad when loading Google Doc

Status: RESOLVED FIXED

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

Description

[Markus Stange [:mstange]]

Loading a big Google Docs document such as [1] spends 140ms of main thread time in FontFace.load: https://perfht.ml/2xdJLaf

[1] https://docs.google.com/document/d/1CwPnZ7NnzzIt_UjqhaESbgWyI8VRDDzUQA_le9qaeG0/edit?usp=sharing

Comment 1

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

Setting this as targeting Firefox 67, since I suspect moving this off of the main thread is probably not a thing we could fit in sanely in the next few weeks.

Is that accurate, jfkthame? This is not a thing we could realistically fit into 64, right?

Comment 2

[Jonathan Kew [:jfkthame]]

Hmm, this is actually quite interesting: according to the profile, that time is being spent on looking up and instantiating a font or fonts loaded via src:local(...), not fonts from downloaded resources via src:url(...). I think we've always assumed that src:local() lookups should be pretty fast, so they're handled synchronously on the main thread, whereas src:url() downloads happen async.

So it may be worth changing that, and handling src:local() as an async font load as well; but I suspect doing so will have repercussions for code (either ours or websites') that assumes src:local() is sync. Most likely something somewhere will break, and we'll have to figure out what to do about it.

I can't see us doing this for 64, no. I think most of the relevant people are busy with things like WebRender- and Fission-related work that probably takes priority for now.

Comment 3

[Markus Stange [:mstange]]

(In reply to Jonathan Kew (:jfkthame) from comment #2)
> but I suspect doing so will have repercussions for code
> (either ours or websites') that assumes src:local() is sync.

How might websites rely on this assumption? The API returns a promise, which will always resolve asynchronously, won't it?

Comment 4

[Jonathan Kew [:jfkthame]]

E.g. assuming that a font defined with a local() source can be used immediately for drawing to <canvas>, without explicitly checking that it is "ready" first. This currently works because the font is loaded synchronously as soon as we try to shape text with it, but may fail if that load becomes async.

Comment 5

[Cameron McCormack (:heycam)]

The only real reason to use a local() source is to include it in a list with some url()s to download if the font is not installed locally.  If you're doing

  let f = new FontFace("Test", "local('Test')");
  f.load();

and then using using that font in <canvas> immediately you may as well just use the font name directly.  If, as I suspect, that most uses of local() are in conjunction with some url()s, then the author is expecting that in some cases the font won't be installed locally, and the Promise must be waited on for it to work.

So I feel like it might not be out of the question to make src() loading async.

Comment 6

[Cameron McCormack (:heycam)]

Coming back to this, I no longer see any time spent under LookupLocalFont, so maybe this is not relying on local() fonts any more.

My comment 5 was talking about making the load() call async, so that script could continue running, but still just deferred to run on the main thread, not on a different thread. I see a total 288 ms under gfxUserFontEntry::LoadPlatformFont, and this work is happening while the main thread is janked for 100 ms at a time, so I think it's still worth thinking about moving this work off to another thread.

Comment 7

[Cameron McCormack (:heycam)]

I'm seeing 72% of time under LoadPlatformFont performing the OTS sanitization. That feels like that should be easily movable OMT. The rest is MakePlatformFont -- how well would that work OMT, Jonathan?

Comment 8

[Jonathan Kew [:jfkthame]]

Yeah, this seems like it should be do-able. IIRC, we should be able to do MakePlatformFont from another thread on all the various back-ends. I believe FreeType requires us to protect the FT_Library with a lock while instantiating an FT_Face, but we already have a mutex in gfx::2d::Factory to use for that.

Comment 9

[Cameron McCormack (:heycam)]

I have a WIP patch that moves the sanitization work OMT. Here's a profile of that Google docs URL from comment 0 http://bit.ly/2IdeA3e where you can see the work under StartPlatformFontLoadOnWorkerThread on the FontLoader thread.

Mike, do you have any suggestions on how can tell whether this change is worth it? I'm guessing yes since I see other work happening on the main thread while the FontLoader thread is doing its thing, but is that all I should be looking for?

Comment 10

[Cameron McCormack (:heycam)]

Attachment: WIP (v0.1)

Comment 11

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

I suspect this is the sort of thing that would impact Google Docs page load and responsiveness.

We do, I believe, have Google Docs in the Raptor tp6 suite. You might try running that to see what kind of impact you get? It might also be worth running those Raptor tests on the 2017 reference device in CI (those are the "windows-10-64-ux" machines in try chooser / try fuzzy).

Hey jesup, do you have other suggestions on how heycam might test this patch? Are there other tests that your team has been working on that might be relevant here?

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

You might need the "right" kind of doc loaded to trigger that - the doc used in raptor is https://docs.google.com/document/d/1US-07msg12slQtI_xchzYxcKlTs6Fp7WqIc6W5GK5M8/edit (251 pages, with some math, music, images, etc).

Certainly the other things happening while loading the font imply there's a possible win here, though perhaps not much on a low-end machine. Maybe not much on mid-end, but the jank removal should help on both low and mid even so.

Minor note: MOZ_ASSERT(NS_GetCurrentThread() == sFontLoadingThread); won't work, you need to use sFontLoadingThread->IsOnCurrentThread(). :-) and 10s may be a bit long

Comment 13

[Cameron McCormack (:heycam)]

One thing I noticed while loading that document is that the majority of the time spent doing this font work is after document load. So it might not help with the actual page load times, but might well help with responsiveness of that work that happens later.

Comment 14

[Cameron McCormack (:heycam)]

I'm going to punt the moving of platform font creation OMT to bug 1556728, and just leave the sanitization stuff in this bug.

Comment 15

[Cameron McCormack (:heycam)]

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

Comment 16

[Cameron McCormack (:heycam)]

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

Comment 17

[Jonathan Kew [:jfkthame]]

:heycam, I just filed bug 1556991 to remove the pref controlling WOFF2 support, which will get it out of your way here.

Comment 18

[Cameron McCormack (:heycam)]

Pushing the patches to try, I don't see any change in the Raptor test results:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=026c30a1c046d2777c2b33b966db428b311d4672&newProject=try&newRevision=5e40c26684401ba53799af37adbe2700f6c71c0d&framework=10

There are a few changes in Talos tests, some up some down:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=026c30a1c046d2777c2b33b966db428b311d4672&newProject=try&newRevision=5e40c26684401ba53799af37adbe2700f6c71c0d&framework=1

though when I overlay them with m-c historical results it looks like they're all within a normal range.

Comment 19

[Cameron McCormack (:heycam)]

From spot checking in profiles, I do so time spent on the main thread doing work while the font load is happening on the FontLoader thread, so I think this should help with responsiveness during those times. But it's not something I've been able to verify.

Comment 20

[Cameron McCormack (:heycam)]

Attachment: Bug 1490792 - Part 1: Move font type determination into SanitizeOpenTypeData.

Comment 21

[Cameron McCormack (:heycam)]

Attachment: Bug 1490792 - Part 2: Make LoadPlatformFont length argument not a reference.

We don't write to it.

Comment 22

[Cameron McCormack (:heycam)]

Attachment: Bug 1490792 - Part 3: Split some work out of LoadPlatformFont.

Comment 23

[Cameron McCormack (:heycam)]

Attachment: Bug 1490792 - Part 4: Perform OpenType sanitization OMT.

Comment 24

[Cameron McCormack (:heycam)]

Idle thought: should we only send the font off to the FontLoader thread when it is bigger than a certain size?

Comment 25

[Jonathan Kew [:jfkthame]]

(In reply to Cameron McCormack (:heycam) from comment #24)

Idle thought: should we only send the font off to the FontLoader thread when it is bigger than a certain size?

That sounds like it could be sensible, yes. Not sure what a good threshold would be, though -- I suppose we could do some measurement to see how long it takes to sanitize a typical font of 1K, 10K, 100K....

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/478ac1179fa6
Part 1: Move font type determination into SanitizeOpenTypeData. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/ebecfbeb55c6
Part 2: Make LoadPlatformFont length argument not a reference. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/f38b9eb31da7
Part 3: Split some work out of LoadPlatformFont. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/1b1a7d2f27c4
Part 4: Perform OpenType sanitization OMT. r=jfkthame

Comment 27

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/478ac1179fa6
https://hg.mozilla.org/mozilla-central/rev/ebecfbeb55c6
https://hg.mozilla.org/mozilla-central/rev/f38b9eb31da7
https://hg.mozilla.org/mozilla-central/rev/1b1a7d2f27c4