Closed Bug 1403198 Opened 3 years ago Closed 3 years ago

264ms jank inside GetFontKeyForScaledFont during tab switching

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- unaffected
firefox58 --- unaffected

People

(Reporter: mstange, Assigned: lsalzman)

References

Details

(Keywords: perf, Whiteboard: [wr-mvp])

Attachments

(6 files)

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.
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
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5b62b5edbee
avoid copying font data in WebRenderBridgeChild::GetFontKeyForScaledFont. r=jrmuizel
Keywords: leave-open
OS: Mac OS X → All
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)
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)
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)
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)
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)
Keywords: leave-open
Depends on: 1413178
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+
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.