Closed Bug 1490792 Opened Last year Closed 3 months ago

140ms of FontFace::DoLoad when loading Google Doc

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox64 --- wontfix
firefox69 --- fixed

People

(Reporter: mstange, Assigned: heycam)

References

Details

(Whiteboard: [qf:p1:pageload])

Attachments

(5 files)

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
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?
Flags: needinfo?(jfkthame)
Whiteboard: [qf] → [qf:p1:f67]
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.
Flags: needinfo?(jfkthame)
(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?
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.
Priority: -- → P3
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.
Whiteboard: [qf:p1:f67] → [qf:p1:pageload]

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.

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?

Flags: needinfo?(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.

Flags: needinfo?(jfkthame)

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?

Flags: needinfo?(mconley)
Attached patch WIP (v0.1)Splinter Review
Assignee: nobody → cam
Status: NEW → ASSIGNED

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?

Flags: needinfo?(mconley) → needinfo?(rjesup)

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

Flags: needinfo?(rjesup)

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.

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

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

Depends on: 1556991

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.

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

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

Pushed by cmccormack@mozilla.com:
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
Regressions: 1557819
Regressions: 1559093
Regressions: 1560306
You need to log in before you can comment on or make changes to this bug.