Bug 219672 – [GPU Process] Cache Font objects

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Ryosuke Niwa]

Reduce the cost of drawing glyph in GPU process by caching Font objects
when it's first used, and reference it later using a rendering resource identifier.

[Ryosuke Niwa]

Created attachment 415728 [details]
WIP

This WIP patch improves the core of Design from ~5 to 86 compared to non-GPU process score of 116 (~35% regression after the change).

[Simon Fraser (smfr)]

Comment on attachment 415728 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=415728&action=review

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:934
> +class CacheFont {

CacheFont is an odd name. CachedFont?

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:955
> +    uint32_t itemID() const { return m_itemID; }

This doesn't seem to be used?

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:966
> +    static WEBCORE_EXPORT uint32_t s_currentItemID;

Why WEBCORE_EXPORT? Can we give this a typedef?

[Wenson Hsieh]

Comment on attachment 415728 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=415728&action=review

>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:934
>> +class CacheFont {
> 
> CacheFont is an odd name. CachedFont?

As a display list item, CacheFont is a more reasonable name, since it describes what the item is doing.

But perhaps something like "RegisterCachedFont" would be more clear.

[Simon Fraser (smfr)]

Comment on attachment 415728 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=415728&action=review

>>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:934
>>> +class CacheFont {
>> 
>> CacheFont is an odd name. CachedFont?
> 
> As a display list item, CacheFont is a more reasonable name, since it describes what the item is doing.
> 
> But perhaps something like "RegisterCachedFont" would be more clear.

Oh I missed that this is a DL item because there's no inheritance. Maybe CacheFontItem?

[Wenson Hsieh]

(In reply to Simon Fraser (smfr) from comment #4)
> Comment on attachment 415728 [details]
> WIP
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415728&action=review
> 
> >>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:934
> >>> +class CacheFont {
> >> 
> >> CacheFont is an odd name. CachedFont?
> > 
> > As a display list item, CacheFont is a more reasonable name, since it describes what the item is doing.
> > 
> > But perhaps something like "RegisterCachedFont" would be more clear.
> 
> Oh I missed that this is a DL item because there's no inheritance. Maybe
> CacheFontItem?

Perhaps. But if we want to add an "Item" suffix here, I'd prefer to do that for all display list items for consistency.

[Ryosuke Niwa]

Created attachment 415847 [details]
Updated for ToT and enabled web fonts

[Ryosuke Niwa]

Turns out we have to use IPC to send the font over since it contains shared buffer in the case of web fonts. But the perf appears to be pretty similar ~84 vs. ~87 so it's probably fine for now. Some kind of pre-warming, etc... can be added in the future to further reduce the IPC cost in the future.

[Ryosuke Niwa]

Created attachment 415973 [details]
Adds Font cache

[Geoffrey Garen]

Comment on attachment 415973 [details]
Adds Font cache

View in context: https://bugs.webkit.org/attachment.cgi?id=415973&action=review

> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:153
> +void RemoteResourceCacheProxy::releaseMemory()
> +{
> +    m_fontLastRenderingUpdateMap.clear();
> +    m_numberOfFontsUsedInCurrentRenderingUpdate = 0;
> +    m_remoteRenderingBackendProxy.deleteAllFonts();
> +}

How does this cache lifetime compare to non-GPU process font cache lifetime?

[Ryosuke Niwa]

(In reply to Geoffrey Garen from comment #9)
> Comment on attachment 415973 [details]
> Adds Font cache
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415973&action=review
> 
> > Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:153
> > +void RemoteResourceCacheProxy::releaseMemory()
> > +{
> > +    m_fontLastRenderingUpdateMap.clear();
> > +    m_numberOfFontsUsedInCurrentRenderingUpdate = 0;
> > +    m_remoteRenderingBackendProxy.deleteAllFonts();
> > +}
> 
> How does this cache lifetime compare to non-GPU process font cache lifetime?

FontCache in WebCore just removes the inactive font but the cache in the GPU Process can be a bit more aggressive because we don't get sync JS API like updating layout or style on it. We can just remove the inactive fonts if this proves to be an issue in the future.

[Wenson Hsieh]

Comment on attachment 415973 [details]
Adds Font cache

View in context: https://bugs.webkit.org/attachment.cgi?id=415973&action=review

The rendering backend and DisplayList changes look reasonable to me.

> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:136
> +    if (unusedFontCount < totalFontCount / minimumFractionOfUnusedFontCountToTriggerRemoval && unusedFontCount <= maximumUnusedFontCountToSkipRemoval)

Nit - `minimumFractionOfUnusedFontCountToTriggerRemoval` sounds like it would be some fraction less than 1 (i.e. 0.25), and then we'd do something like:

unusedFontCount < minimumFractionOfUnusedFontCountToTriggerRemoval * totalFontCount

> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.h:69
> +    HashMap<WebCore::RenderingResourceIdentifier, uint64_t> m_fontLastRenderingUpdateMap;

Nit - maybe `m_fontIdentifierToLastRenderingUpdateVersionMap`?

> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.h:71
> +    uint64_t m_currentRenderingUpdateVersion { 1 };

Can we can add a `using RenderingUpdateVersion = uint64_t;` instead of just using uint64_t?

[Ryosuke Niwa]

(In reply to Wenson Hsieh from comment #11)
> Comment on attachment 415973 [details]
> Adds Font cache
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415973&action=review
> 
> The rendering backend and DisplayList changes look reasonable to me.
> 
> > Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:136
> > +    if (unusedFontCount < totalFontCount / minimumFractionOfUnusedFontCountToTriggerRemoval && unusedFontCount <= maximumUnusedFontCountToSkipRemoval)
> 
> Nit - `minimumFractionOfUnusedFontCountToTriggerRemoval` sounds like it
> would be some fraction less than 1 (i.e. 0.25), and then we'd do something
> like:

Yeah, I had that exact thought. Double precision math is slightly less efficient than division by unsigned but maybe clang can optimize it. It's not a useful perf cost anyway given we'd be sending a bunch of IPCs later. Done this.

> > Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.h:69
> > +    HashMap<WebCore::RenderingResourceIdentifier, uint64_t> m_fontLastRenderingUpdateMap;
> 
> Nit - maybe `m_fontIdentifierToLastRenderingUpdateVersionMap`?

Yeah, I had that exact thought. I was trying to come up with a more terse name but maybe it's okay to use a long name here. Done the rename.

> > Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.h:71
> > +    uint64_t m_currentRenderingUpdateVersion { 1 };
> 
> Can we can add a `using RenderingUpdateVersion = uint64_t;` instead of just
> using uint64_t?

I don't think it's a meaningful distinction since anyone cast it interchangeably with uint64_t. I'd keep it as uint64_t for now.

[Ryosuke Niwa]

Created attachment 416074 [details]
Patch for landing

[EWS]

Committed r270725: <https://trac.webkit.org/changeset/270725>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416074 [details].

[Radar WebKit Bug Importer]

<rdar://problem/72246783>

[Said Abou-Hallawa]

*** Bug 219666 has been marked as a duplicate of this bug. ***