[webkit-reviews] review granted: [Bug 227229] [GPU Process] RELEASE_ASSERT in RemoteResourceCacheProxy::didFinalizeRenderingUpdate() may fire if GPUP is relaunched : [Attachment 431899] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 227229: [GPU Process] RELEASE_ASSERT in
RemoteResourceCacheProxy::didFinalizeRenderingUpdate() may fire if GPUP is
relaunched
https://bugs.webkit.org/show_bug.cgi?id=227229

Attachment 431899: Patch

https://bugs.webkit.org/attachment.cgi?id=431899&action=review




--- Comment #3 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 431899
  --> https://bugs.webkit.org/attachment.cgi?id=431899
Patch

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

> Source/WebKit/ChangeLog:18
> +	      corresponding entries form
m_fontIdentifierToLastRenderingUpdateVersionMap.

typo: "form"

> Source/WebKit/ChangeLog:21
> +	   3) We need to prepareForNextRenderingUpdate() even if no font will
be
> +	      removed from m_fontIdentifierToLastRenderingUpdateVersionMap.

prepareForNextRenderingUpdate() is a new function that's added by this patch,
so I don't quite understand why this list in the ChangeLog includes a detail of
its calling pattern.

> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:137
> +    m_currentRenderingUpdateVersion = 0;

RemoteResourceCacheProxy::cacheFont() has:

    auto result =
m_fontIdentifierToLastRenderingUpdateVersionMap.ensure(font.renderingResourceId
entifier(), [&] {
	return 0;
    });
    auto& lastVersion = result.iterator->value;
    if (lastVersion != m_currentRenderingUpdateVersion) {
	lastVersion = m_currentRenderingUpdateVersion;
	++m_numberOfFontsUsedInCurrentRenderingUpdate;
    }

If we set m_currentRenderingUpdateVersion to 0 here, then this "if" block will
not execute for new fonts that enter cacheFont() after clearFontMap() runs, so
we would end up telling the GPUP to cache the new font without incrementing
m_numberOfFontsUsedInCurrentRenderingUpdate.

> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:149
> +    if (!unusedFontCount || unusedFontCount <
minimumFractionOfUnusedFontCountToTriggerRemoval * totalFontCount) {

It looks like you changed the && to ||. Was that intentional?

If || is intentional, I think we can just simplify the entire check to "if
(unusedFontCount <= minimumFractionOfUnusedFontCountToTriggerRemoval *
totalFontCount)"

If not, and it's supposed to be && instead, then I think the check can be just
"if (!unusedFontCount)"

> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:150
> +	   prepareForNextRenderingUpdate();

I don't know if the ChangeLog mentions this, but this seems pretty important.
Previously, we weren't incrementing m_currentRenderingUpdateVersion in this
branch, which means we were treating adjacent rendering updates as not-distinct
if we felt that we didn't need to prune
m_fontIdentifierToLastRenderingUpdateVersionMap. Now, with this patch, even if
we feel that we don't need to prune
m_fontIdentifierToLastRenderingUpdateVersionMap, we're treating adjacent
rendering updates as distinct. I think this is important and valuable, and
probably worth mentioning in the ChangeLog.

> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:154
> +    Vector<RenderingResourceIdentifier> toBeRemoved;

What's the purpose of the vector? Why not put the call to
m_fontIdentifierToLastRenderingUpdateVersionMap.remove() inside the for (auto&
item : m_fontIdentifierToLastRenderingUpdateVersionMap) loop? Usually we use
these vectors to guarantee lifetimes of stuff using RefPtrs, but
RenderingResourceIdentifier doesn't guarantee any lifetimes.

> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:164
> +    for (auto& renderingResourceIdentifier : toBeRemoved)

Identifiers are just single scalar values, so I think "auto" is better than
"auto&"