1403198 - 264ms jank inside GetFontKeyForScaledFont during tab switching

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P1)

Description

[Markus Stange [:mstange]]

Profile: https://perfht.ml/2xx66hz

I loaded https://www.reddit.com/r/firefox/comments/72kljc/firefox_quantum_developer_edition_the_fastest/ in a background tab and then switched to that tab.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

I suspect this might be the very large Apple Color Emoji font. We should probably just be pulling that from the system instead of getting the font data and passing it all around. We should also look at making this path faster, it looks like we are doing multiple copies of the font data.

Comment 2

[Lee Salzman [:lsalzman]]

Attachment: avoid copying font data in WebRenderBridgeChild::GetFontKeyForScaledFont

This avoids using a temporary buffer and does the actual resource queuing inside the font callback, so that we can just read straight from mmap'd buffers and such.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5b62b5edbee
avoid copying font data in WebRenderBridgeChild::GetFontKeyForScaledFont. r=jrmuizel

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 5

[Lee Salzman [:lsalzman]]

Attachment: send font descriptors to WR instead of raw fonts where possible

This makes it possible to send font descriptors over to WebRender.

This patch is mostly just plumbing through callbacks and IPDL to serialize the descriptor and then send it over.

Comment 6

[Lee Salzman [:lsalzman]]

Attachment: support WR font descriptors with Fontconfig

This patch depends on this upstream WR pull request that adds a FreeType NativeFontHandle to WR: https://github.com/servo/webrender/pull/1983

Comment 7

[Lee Salzman [:lsalzman]]

Attachment: support WR font descriptors with DWrite

Have to plumb through whether the UnscaledFontDWrite is from the system font collection or not, since this is where dwrote-rs currently looks for family names that are sent over.

The basic approach is to get the font's family and style info, then check if it matches up when we query it in the same manner as dwrote-rs would.

Comment 8

[Lee Salzman [:lsalzman]]

Attachment: support WR font descriptors on Mac

Nothing too fancy here. Just reads the postscript name of the font and sends it over, like webrender_api does when its serializes NativeFontHandle.

Comment 9

[Lee Salzman [:lsalzman]]

Attachment: support WR native font handles in blob image

Blob image currently expects all fonts to be transmitted via raw font data, otherwise it crashes. This fixes blob image to properly support this new native font handle/descriptor mechanism so that everything works again.

Comment 10

[Darkspirit]

(In reply to Lee Salzman [:lsalzman] from comment #5)
Would
> Created attachment 8924960 [details] [diff] [review]
> send font descriptors to WR instead of raw fonts where possible
and
> Created attachment 8924967 [details] [diff] [review]
> support WR native font handles in blob image
fix bug 1412545 comment 13?

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8924960 [details] [diff] [review]
send font descriptors to WR instead of raw fonts where possible

Review of attachment 8924960 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/wr/WebRenderBridgeChild.cpp
@@ +339,5 @@
>    wr::FontKey fontKey = { wr::IdNamespace { 0 }, 0};
>    if (!mFontKeys.Get(aUnscaled, &fontKey)) {
>      wr::IpcResourceUpdateQueue resources(GetShmemAllocator());
>      FontFileDataSink sink = { &fontKey, this, &resources };
> +    if (!aUnscaled->GetWRFontDescriptor(WriteFontDescriptor, &sink) &&

It might be worth adding a comment about how we try to get a descriptor first otherwise we get the data. I had too think too long about how this would work.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8924964 [details] [diff] [review]
support WR font descriptors with DWrite

Review of attachment 8924964 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/UnscaledFontDWrite.h
@@ +48,3 @@
>  private:
>    RefPtr<IDWriteFontFace> mFontFace;
> +  RefPtr<IDWriteFont> mFont;

Maybe add a comment that we only have an mFont when it's a system font.

::: gfx/webrender_bindings/src/bindings.rs
@@ +992,5 @@
> +) -> NativeFontHandle {
> +    let wchars = bytes.convert_into_vec::<u16>();
> +    FontDescriptor {
> +        family_name: String::from_utf16(&wchars).unwrap(),
> +        weight: FontWeight::from_u32(index & 0xffff),

Passing all of these things in in index is sort of crazy. Can you add a comment about why it's necessary.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0ad40dad503
send font descriptors to WR instead of raw fonts where possible. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/87fb72d343ed
support WR font descriptors with Fontconfig. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/d619753f890b
support WR font descriptors with DWrite. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e450204ab2e
support WR font descriptors on Mac. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a38089c0375
support WR native font handles in blob image. r=jrmuizel

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e0ad40dad503
https://hg.mozilla.org/mozilla-central/rev/87fb72d343ed
https://hg.mozilla.org/mozilla-central/rev/d619753f890b
https://hg.mozilla.org/mozilla-central/rev/8e450204ab2e
https://hg.mozilla.org/mozilla-central/rev/5a38089c0375