RESOLVED DUPLICATE of bug 227229 227277
REGRESSION(r279104): [GPU Process] Canvas layout tests became flaky
https://bugs.webkit.org/show_bug.cgi?id=227277
Summary REGRESSION(r279104): [GPU Process] Canvas layout tests became flaky
Said Abou-Hallawa
Reported 2021-06-22 21:37:51 PDT
The change r279104 tried to fix some issues in RemoteResourceCacheProxy::didFinalizeRenderingUpdate(). It fixed the early return from: if (unusedFontCount < minimumFractionOfUnusedFontCountToTriggerRemoval * totalFontCount && unusedFontCount <= maximumUnusedFontCountToSkipRemoval) return; // which is equivalent to the following code because unusedFontCount and maximumUnusedFontCountToSkipRemoval are both unsigned and maximumUnusedFontCountToSkipRemoval = 0. // if (!unusedFontCount) // return; to: if (unusedFontCount <= minimumFractionOfUnusedFontCountToTriggerRemoval * totalFontCount) { prepareForNextRenderingUpdate(); return; } And it fixed the synchronization of the cached fonts between WebProcess and GPUP: from: // This code removes the font object from GPUP but keeps the font identifier in WebProcess for (auto& item : m_fontIdentifierToLastRenderingUpdateVersionMap) { if (m_currentRenderingUpdateVersion - item.value >= minimumRenderingUpdateCountToKeepFontAlive) m_remoteRenderingBackendProxy.releaseRemoteResource(item.key); } to: m_fontIdentifierToLastRenderingUpdateVersionMap.removeIf([&] (const auto& item) { if (m_currentRenderingUpdateVersion - item.value < minimumRenderingUpdateCountToKeepFontAlive) return false; m_remoteRenderingBackendProxy.releaseRemoteResource(item.key); return true; }); But when I looked closely at the original RemoteResourceCacheProxy::didFinalizeRenderingUpdate() before r279104, I found it was always making an early return and basically it was doing nothing. Here is the explanation. 1. When the fonts are cached for the first RenderingUpdate to a canvas, all the entries in m_fontIdentifierToLastRenderingUpdateVersionMap will have value = 1 since m_currentRenderingUpdateVersion is initialized with 1. 2. m_numberOfFontsUsedInCurrentRenderingUpdate will be equal to the number of fonts cached during the first RenderingUpdate. 3. RemoteResourceCacheProxy::didFinalizeRenderingUpdate(), will find unusedFontCount == 0 because m_numberOfFontsUsedInCurrentRenderingUpdate == m_fontIdentifierToLastRenderingUpdateVersionMap.size(). So it will return without changing m_numberOfFontsUsedInCurrentRenderingUpdate or m_currentRenderingUpdateVersion. 4. If in the next RenderingUpdate a new font is cached, it will inherit m_currentRenderingUpdateVersion which is still 1. And m_numberOfFontsUsedInCurrentRenderingUpdate will be incremented by 1. 5. Caching more fonts will increment m_numberOfFontsUsedInCurrentRenderingUpdate as if all the cached fonts were used for m_currentRenderingUpdateVersion = 1. 6. RemoteResourceCacheProxy::didFinalizeRenderingUpdate() will make an early return again because unusedFontCount is always zero. So the change r279104 exercises a new code path which was never exercised before. Removing the font from the cache in GPUP and recaching it causes RemoteRenderingBackend to wakeUpAndApplyDisplayList() more often if the Font was missing in the GPUP cache. I think the solution is to remove RemoteResourceCacheProxy::didFinalizeRenderingUpdate() since it has been doing nothing since it was written.
Attachments
Patch (7.25 KB, patch)
2021-06-22 21:45 PDT, Said Abou-Hallawa
no flags
Patch (7.25 KB, patch)
2021-06-22 22:43 PDT, Said Abou-Hallawa
ews-feeder: commit-queue-
Patch (7.25 KB, patch)
2021-06-22 23:04 PDT, Said Abou-Hallawa
no flags
Patch (7.25 KB, patch)
2021-06-22 23:07 PDT, Said Abou-Hallawa
no flags
Patch (7.25 KB, patch)
2021-06-22 23:08 PDT, Said Abou-Hallawa
no flags
Said Abou-Hallawa
Comment 1 2021-06-22 21:45:48 PDT
Said Abou-Hallawa
Comment 2 2021-06-22 22:43:11 PDT
Said Abou-Hallawa
Comment 3 2021-06-22 23:04:31 PDT
Said Abou-Hallawa
Comment 4 2021-06-22 23:07:34 PDT
Said Abou-Hallawa
Comment 5 2021-06-22 23:08:10 PDT
Said Abou-Hallawa
Comment 6 2021-06-23 10:36:21 PDT
*** This bug has been marked as a duplicate of bug 227229 ***
Note You need to log in before you can comment on or make changes to this bug.