264ms jank inside GetFontKeyForScaledFont during tab switching

RESOLVED FIXED in mozilla58

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: mstange, Assigned: lsalzman)

Tracking

({perf})

Trunk
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 unaffected, firefox58 unaffected)

Details

(Whiteboard: [wr-mvp])

Attachments

(6 attachments)

Priority: -- → P2
Whiteboard: [wr-mvp]
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.
Assignee

Comment 2

2 years ago
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.
Attachment #8918006 - Flags: review?(jmuizelaar)
Attachment #8918006 - Flags: review?(jmuizelaar) → review+
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Priority: P2 → P1

Comment 3

2 years ago
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5b62b5edbee
avoid copying font data in WebRenderBridgeChild::GetFontKeyForScaledFont. r=jrmuizel
Assignee

Updated

2 years ago
Keywords: leave-open
OS: Mac OS X → All
Assignee

Comment 5

2 years ago
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.
Attachment #8924960 - Flags: review?(jmuizelaar)
Assignee

Comment 6

2 years ago
This patch depends on this upstream WR pull request that adds a FreeType NativeFontHandle to WR: https://github.com/servo/webrender/pull/1983
Attachment #8924962 - Flags: review?(jmuizelaar)
Assignee

Comment 7

2 years ago
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.
Attachment #8924964 - Flags: review?(jmuizelaar)
Assignee

Comment 8

2 years ago
Nothing too fancy here. Just reads the postscript name of the font and sends it over, like webrender_api does when its serializes NativeFontHandle.
Attachment #8924966 - Flags: review?(jmuizelaar)
Assignee

Comment 9

2 years ago
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.
Attachment #8924967 - Flags: review?(jmuizelaar)
Assignee

Updated

2 years ago
Keywords: leave-open
Assignee

Updated

2 years ago
Depends on: 1413178
Comment hidden (obsolete)
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.
Attachment #8924960 - Flags: review?(jmuizelaar) → review+
Attachment #8924962 - Flags: review?(jmuizelaar) → review+
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.
Attachment #8924964 - Flags: review?(jmuizelaar) → review+
Attachment #8924966 - Flags: review?(jmuizelaar) → review+
Attachment #8924967 - Flags: review?(jmuizelaar) → review+

Comment 13

2 years ago
Pushed by lsalzman@mozilla.com:
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
Depends on: 1435062
You need to log in before you can comment on or make changes to this bug.