Bug 219672

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

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?

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.

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?

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

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

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.

Created attachment 415973 [details] Adds Font cache

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?

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

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?

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

Created attachment 416074 [details] Patch for landing

Committed r270725: <https://trac.webkit.org/changeset/270725> All reviewed patches have been landed. Closing bug and clearing flags on attachment 416074 [details].

<rdar://problem/72246783>

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