Bug 228222 – [GPU Process] Add resource use counter infrastructure to RemoteResourceCache

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[GPU Process] Add resource use counter infrastructure to RemoteResourceCache

[Myles C. Maxfield]

Created attachment 434067 [details]
Patch for reviewing

[Myles C. Maxfield]

Created attachment 434072 [details]
Patch for EWS

[Myles C. Maxfield]

Created attachment 434094 [details]
Patch for reviewing

[Said Abou-Hallawa]

Comment on attachment 434094 [details]
Patch for reviewing

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

> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:63
> +void RemoteResourceCache::incrementOpenCount(WebCore::RenderingResourceIdentifier renderingResourceIdentifier)

I think this function is not needed. See below.

> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:103
> +void RemoteResourceCache::recordResourceUse(WebCore::RenderingResourceIdentifier renderingResourceIdentifier)

This function is not called in this patch.

> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:130
> +bool RemoteResourceCache::prune()
> +{
> +    for (auto renderingResourceIdentifier : m_resourcesForMaybeRemoving) {
> +        auto iterator = m_resourceUseCounters.find(renderingResourceIdentifier);
> +        ASSERT(iterator != m_resourceUseCounters.end());
> +        auto messagesAreInOrder = maybeRemoveResource(renderingResourceIdentifier, iterator);
> +        if (!messagesAreInOrder)
> +            return false;
> +    }
> +    m_resourcesForMaybeRemoving.clear();
> +    return true;
> +}

Instead of running this pass at the end of replaying the DL items, can't it be done for each resource individually? For example Replayer::applyItem() can get the renderingResourceIdentifier from the DL item and it call a function in ReplayerDelegate which will call RemoteResourceCache::recordResourceUse(). 

RemoteResourceCache::recordResourceUse() will increment the useCount and it is it in to_be_removed_state and the useCount matches the useCount of the WebProcess, the resource will be deleted.

> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.h:73
> +        uint64_t openCount { 0 }; // The number of cacheFoo messages we've received without a corresponding release message

I think this is incorrect. The job of RemoteResourceCacheProxy is manage the sequence to be always: cacheFoo(), releaseResource(), cacheFoo(), releaseResource(), ... etc

RemoteResourceCacheProxy keeps HashMaps for the resources which are cached in GPUP. So it sends a cacheFoo() message only once for the resource and it does not send other cacheFoo() as long as it is in one of these HashMaps. The resource is removed from the cache when it sends releaseResource(). And I think the IPC messages have to be dispatched in the same order they are sent with.

I think the problem we want to address is if the resource is added to m_resourcesForMaybeRemoving when the first releaseResource() is received and it stays there till the second cacheFoo() is received.

> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.h:75
> +        uint64_t useCount { 0 }; // The number of rendering commands we've executed which reference this resource
> +        uint64_t totalUseCount { 0 }; // Once useCount hits this limit, we can free the resource

I have an idea for these counters which may or may not be correct. I think we can use a single counter which can serve as useOrPendingCount.

1. useOrPendingCount is signed int and is initialized with { 0 };
2. When the resource is moved from cached_state to to_be_removed_state: useOrPendingCount -= useCount     // Now it is used as pendingCount and it will be either zero or negative
3. When the resource is moved from to_be_removed_state to cached_state: useOrPendingCount is not changed  // Now it is used as useCount even if it is negative. It will be incremented by the DL items which were sent after the old and the new cacheFoo() messages.
4. When the resource is used, useOrPendingCount will be incremented.
5. When the resource is used and it is in to_be_removed_state and useOrPendingCount == 0, the resource will be removed.

[Myles C. Maxfield]

Comment on attachment 434094 [details]
Patch for reviewing

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

>> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:103
>> +void RemoteResourceCache::recordResourceUse(WebCore::RenderingResourceIdentifier renderingResourceIdentifier)
> 
> This function is not called in this patch.

Right, it's called in the follow-ups. This patch just adds the infrastructure. I split the patch up to aid reviewing.
- https://bugs.webkit.org/show_bug.cgi?id=228224
- https://bugs.webkit.org/show_bug.cgi?id=228225

>> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:130
>> +}
> 
> Instead of running this pass at the end of replaying the DL items, can't it be done for each resource individually? For example Replayer::applyItem() can get the renderingResourceIdentifier from the DL item and it call a function in ReplayerDelegate which will call RemoteResourceCache::recordResourceUse(). 
> 
> RemoteResourceCache::recordResourceUse() will increment the useCount and it is it in to_be_removed_state and the useCount matches the useCount of the WebProcess, the resource will be deleted.

The ChangeLog explains this under the notes for WebKit::RemoteResourceCache::recordResourceUse(). We can't actually delete the resource inside Replayer::Delegate::apply(), because after the replayer delegate returns, the DL item needs to actually execute, and in order to execute, it needs access to the resource. So, we have to wait until the item has finished executing before we can actually delete the resource.

>> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.h:73
>> +        uint64_t openCount { 0 }; // The number of cacheFoo messages we've received without a corresponding release message
> 
> I think this is incorrect. The job of RemoteResourceCacheProxy is manage the sequence to be always: cacheFoo(), releaseResource(), cacheFoo(), releaseResource(), ... etc
> 
> RemoteResourceCacheProxy keeps HashMaps for the resources which are cached in GPUP. So it sends a cacheFoo() message only once for the resource and it does not send other cacheFoo() as long as it is in one of these HashMaps. The resource is removed from the cache when it sends releaseResource(). And I think the IPC messages have to be dispatched in the same order they are sent with.
> 
> I think the problem we want to address is if the resource is added to m_resourcesForMaybeRemoving when the first releaseResource() is received and it stays there till the second cacheFoo() is received.

I think you're right about having to support resources migrating back and forth between the "between a cacheResource / release pair" state and the "after the release message" state.

> I think this is incorrect.

What does "this" refer to?

>> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.h:75
>> +        uint64_t totalUseCount { 0 }; // Once useCount hits this limit, we can free the resource
> 
> I have an idea for these counters which may or may not be correct. I think we can use a single counter which can serve as useOrPendingCount.
> 
> 1. useOrPendingCount is signed int and is initialized with { 0 };
> 2. When the resource is moved from cached_state to to_be_removed_state: useOrPendingCount -= useCount     // Now it is used as pendingCount and it will be either zero or negative
> 3. When the resource is moved from to_be_removed_state to cached_state: useOrPendingCount is not changed  // Now it is used as useCount even if it is negative. It will be incremented by the DL items which were sent after the old and the new cacheFoo() messages.
> 4. When the resource is used, useOrPendingCount will be incremented.
> 5. When the resource is used and it is in to_be_removed_state and useOrPendingCount == 0, the resource will be removed.

I think this is correct, with one slight note: After step 2, pendingCount can be positive.

Consider this example:
The web process sends:
1. Cache resource
2. Use 2 times
3. Release, with useCount = 2
4. Cache the same resource again
5. Use 4 times
6. Release, with useCount = 4

The GPU Process sees:
1. Cache resource
2. Use 2 times
3. Use 3 more times, from the second batch. This is totally possible.
4. Release, with useCount = 2. We could choose to delete the resource itself now (or not!), but we definitely can't forget the useCount. We have to remember that we've seen 5 uses (either by storing the value 5 itself, or by storing  5 - 2 = 3)
5. [if we see the last use here, our existing infrastructure will automatically delay it until later]
6. Cache the resource again
7. Release, with useCount = 4. Now, we have to realize that this means we only need one more use before we can delete the resource
8. The last use. Delete the resource.

(We're relying on the fact that, if the first cache resource message and the second cache resource message share the same renderingResourceIdentifier, the resource data itself is identical. So, step 3 in the GPU process's block above is okay.)

[Myles C. Maxfield]

Comment on attachment 434094 [details]
Patch for reviewing

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

>>> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.h:73
>>> +        uint64_t openCount { 0 }; // The number of cacheFoo messages we've received without a corresponding release message
>> 
>> I think this is incorrect. The job of RemoteResourceCacheProxy is manage the sequence to be always: cacheFoo(), releaseResource(), cacheFoo(), releaseResource(), ... etc
>> 
>> RemoteResourceCacheProxy keeps HashMaps for the resources which are cached in GPUP. So it sends a cacheFoo() message only once for the resource and it does not send other cacheFoo() as long as it is in one of these HashMaps. The resource is removed from the cache when it sends releaseResource(). And I think the IPC messages have to be dispatched in the same order they are sent with.
>> 
>> I think the problem we want to address is if the resource is added to m_resourcesForMaybeRemoving when the first releaseResource() is received and it stays there till the second cacheFoo() is received.
> 
> I think you're right about having to support resources migrating back and forth between the "between a cacheResource / release pair" state and the "after the release message" state.

When you say "I think this is incorrect," what does "this" refer to?

[Said Abou-Hallawa]

openCount has to be "1" always. It can't be something else. The assumption that cacheFoo() can be received multiple times for the same resource without receiving the releaseResource() in between is "incorrect".

[Myles C. Maxfield]

Created attachment 434137 [details]
Reworked struct ResourceUseCounter

[Said Abou-Hallawa]

Comment on attachment 434094 [details]
Patch for reviewing

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

>>> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:130
>>> +}
>> 
>> Instead of running this pass at the end of replaying the DL items, can't it be done for each resource individually? For example Replayer::applyItem() can get the renderingResourceIdentifier from the DL item and it call a function in ReplayerDelegate which will call RemoteResourceCache::recordResourceUse(). 
>> 
>> RemoteResourceCache::recordResourceUse() will increment the useCount and it is it in to_be_removed_state and the useCount matches the useCount of the WebProcess, the resource will be deleted.
> 
> The ChangeLog explains this under the notes for WebKit::RemoteResourceCache::recordResourceUse(). We can't actually delete the resource inside Replayer::Delegate::apply(), because after the replayer delegate returns, the DL item needs to actually execute, and in order to execute, it needs access to the resource. So, we have to wait until the item has finished executing before we can actually delete the resource.

Then we need to call RemoteResourceCache::recordResourceUse() "after" using the resource.

[Myles C. Maxfield]

Created attachment 434138 [details]
Reworked struct ResourceUseCounter

[Myles C. Maxfield]

Created attachment 434139 [details]
Reworked struct ResourceUseCounter

[Myles C. Maxfield]

Created attachment 434145 [details]
Reworked struct ResourceUseCounter and Replayer::Delegate

[Said Abou-Hallawa]

Comment on attachment 434145 [details]
Reworked struct ResourceUseCounter and Replayer::Delegate

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

> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:67
> +    auto result = m_resourceUseCounters.ensure(renderingResourceIdentifier, [&] {
> +        return ResourceUseCounter { };
> +    });

Shouldn't we use m_resourceUseCounters.add() instead? I think initializing the ResourceUseCounter is very cheap.

> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:70
> +    auto& state = result.iterator->value.state;
> +    RELEASE_ASSERT(state == ResourceState::ToBeDeleted);
> +    state = ResourceState::Alive;

It is strange that if the resource is added to the HashMap we initialize its state to ResourceState::ToBeDeleted and we change it immediately to ResourceState::Alive. Can't we change the struct ResourceUseCounter below to initialize the state to ResourceState::Alive and handle the case of isNewEntry separately?

> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:78
> +bool RemoteResourceCache::maybeRemoveResource(RenderingResourceIdentifier renderingResourceIdentifier, ResourceUseCountersMap::iterator iterator)

Should it be ResourceUseCountersMap::iterator&?

> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:84
> +    // It's actually okay if useCount > 0.
> +    // It just means that there's a cache message pending, and we've eagerly used a previously-cached resource.
> +    // Which is fine.
> +    // FIXME: Maybe we shouldn't actually remove the resource in this situation?
> +    // We may actually be able to get a head start on some display list items if they're ready for us and we have the resources they depend on.

I think the case (state == ToBeDeleted && useCount > 0) will never happen if recordResourceUse() is called from Replayer::applyItem(). The resource will be removed if useOrPendingCount is zero. Next reference to this resource will return MissingCachedResource and will delay replaying the DL item till the resource is recached.

> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:90
> +    if (!value.useOrPendingCount)

As in the above comment, this can't be false. If state.state == ToBeDeleted is true, value.useOrPendingCount has to be zero. Also it does not make sense to keep the resource in m_resourceUseCounters but removing it from the HashMaps below if this condition happens to be false.

> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:112
> +    if (iterator == m_resourceUseCounters.end()) {
> +        // The display list items won the race against the cacheFont message.
> +        // When we receive the cacheFont message, we will retry the display list item.
> +        return;
> +    }

I think this should never happen. This function should be called while replaying the DL items and after the resource is used. So the resource should be in one of the HashMaps: m_imageBuffers, m_nativeImages or m_fonts. And it should be cashed in m_resourceUseCounters.

I think this should be replaced by ASSERT(iterator != m_resourceUseCounters.end());

> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.h:73
> +        ResourceState state { ResourceState::ToBeDeleted };

I think this should be initialized with ResourceState::Alive.

[Myles C. Maxfield]

Comment on attachment 434145 [details]
Reworked struct ResourceUseCounter and Replayer::Delegate

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

>> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:84
>> +    // We may actually be able to get a head start on some display list items if they're ready for us and we have the resources they depend on.
> 
> I think the case (state == ToBeDeleted && useCount > 0) will never happen if recordResourceUse() is called from Replayer::applyItem(). The resource will be removed if useOrPendingCount is zero. Next reference to this resource will return MissingCachedResource and will delay replaying the DL item till the resource is recached.

How should the situation I described at the end of https://bugs.webkit.org/show_bug.cgi?id=228222#c5 work? I think the GPU process's step 4 will see state == ToBeDeleted && useCount > 0.

>> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:90
>> +    if (!value.useOrPendingCount)
> 
> As in the above comment, this can't be false. If state.state == ToBeDeleted is true, value.useOrPendingCount has to be zero. Also it does not make sense to keep the resource in m_resourceUseCounters but removing it from the HashMaps below if this condition happens to be false.

In the situation linked to above, I think the GPU process's step 4 may or may not choose to remove the resource, which is what the comment above on lines 83-84 is about. However, I think the GPU process's step 4 definitely has to keep the resource in m_resourceUseCounters, because its step 8 has to realize that there's only 1 outstanding use, rather than 4 outstanding uses.

>> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:112
>> +    }
> 
> I think this should never happen. This function should be called while replaying the DL items and after the resource is used. So the resource should be in one of the HashMaps: m_imageBuffers, m_nativeImages or m_fonts. And it should be cashed in m_resourceUseCounters.
> 
> I think this should be replaced by ASSERT(iterator != m_resourceUseCounters.end());

Right, this makes sense. If the resource is in m_imageBuffers, m_nativeImages or m_fonts, then it should be guaranteed to be in m_resourceUseCounters. I can make sure at the call site to only call this function if the resource is present in m_imageBuffers, m_nativeImages or m_fonts.

[Myles C. Maxfield]

Created attachment 434159 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 434145 [details]
Reworked struct ResourceUseCounter and Replayer::Delegate

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

>>> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:84
>>> +    // We may actually be able to get a head start on some display list items if they're ready for us and we have the resources they depend on.
>> 
>> I think the case (state == ToBeDeleted && useCount > 0) will never happen if recordResourceUse() is called from Replayer::applyItem(). The resource will be removed if useOrPendingCount is zero. Next reference to this resource will return MissingCachedResource and will delay replaying the DL item till the resource is recached.
> 
> How should the situation I described at the end of https://bugs.webkit.org/show_bug.cgi?id=228222#c5 work? I think the GPU process's step 4 will see state == ToBeDeleted && useCount > 0.

Oh, now that I think about it some more, this situation might actually be impossible if:
1. No resources are released and recached in the middle of a rendering update (which is true today)
2. Each rendering update starts a new display list
3. The trigger that starts the GPU process executing the new DL is a message

If all of those things are true, then the GPU Process can’t start executing the second frame’s DL until it has seen the previous frame’s release message.

I’ll do some more investigation and testing to see if it’s possible.

[Myles C. Maxfield]

Comment on attachment 434145 [details]
Reworked struct ResourceUseCounter and Replayer::Delegate

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

>>>> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:84
>>>> +    // We may actually be able to get a head start on some display list items if they're ready for us and we have the resources they depend on.
>>> 
>>> I think the case (state == ToBeDeleted && useCount > 0) will never happen if recordResourceUse() is called from Replayer::applyItem(). The resource will be removed if useOrPendingCount is zero. Next reference to this resource will return MissingCachedResource and will delay replaying the DL item till the resource is recached.
>> 
>> How should the situation I described at the end of https://bugs.webkit.org/show_bug.cgi?id=228222#c5 work? I think the GPU process's step 4 will see state == ToBeDeleted && useCount > 0.
> 
> Oh, now that I think about it some more, this situation might actually be impossible if:
> 1. No resources are released and recached in the middle of a rendering update (which is true today)
> 2. Each rendering update starts a new display list
> 3. The trigger that starts the GPU process executing the new DL is a message
> 
> If all of those things are true, then the GPU Process can’t start executing the second frame’s DL until it has seen the previous frame’s release message.
> 
> I’ll do some more investigation and testing to see if it’s possible.

Yep - I verified that this is possible. If JS enqueues a bunch of commands which are fast to enqueue but slow to execute, the WP can enqueue 2 frames of commands, and the GPUP will execute the 2 frames synchronously because of the loop in RemoteRenderingBackend::nextDestinationImageBufferAfterApplyingDisplayLists(). The cache/release messages don't get a chance to execute in between the execution of these 2 frames, so we execute the second frame's DrawGlyphs using the first frame's font (which is okay, since it's the same font with the same renderingResourceIdentifier).

[Myles C. Maxfield]

Comment on attachment 434145 [details]
Reworked struct ResourceUseCounter and Replayer::Delegate

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

>>>>> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:84
>>>>> +    // We may actually be able to get a head start on some display list items if they're ready for us and we have the resources they depend on.
>>>> 
>>>> I think the case (state == ToBeDeleted && useCount > 0) will never happen if recordResourceUse() is called from Replayer::applyItem(). The resource will be removed if useOrPendingCount is zero. Next reference to this resource will return MissingCachedResource and will delay replaying the DL item till the resource is recached.
>>> 
>>> How should the situation I described at the end of https://bugs.webkit.org/show_bug.cgi?id=228222#c5 work? I think the GPU process's step 4 will see state == ToBeDeleted && useCount > 0.
>> 
>> Oh, now that I think about it some more, this situation might actually be impossible if:
>> 1. No resources are released and recached in the middle of a rendering update (which is true today)
>> 2. Each rendering update starts a new display list
>> 3. The trigger that starts the GPU process executing the new DL is a message
>> 
>> If all of those things are true, then the GPU Process can’t start executing the second frame’s DL until it has seen the previous frame’s release message.
>> 
>> I’ll do some more investigation and testing to see if it’s possible.
> 
> Yep - I verified that this is possible. If JS enqueues a bunch of commands which are fast to enqueue but slow to execute, the WP can enqueue 2 frames of commands, and the GPUP will execute the 2 frames synchronously because of the loop in RemoteRenderingBackend::nextDestinationImageBufferAfterApplyingDisplayLists(). The cache/release messages don't get a chance to execute in between the execution of these 2 frames, so we execute the second frame's DrawGlyphs using the first frame's font (which is okay, since it's the same font with the same renderingResourceIdentifier).

I verified it by simulating a command which is fast to enqueue but slow to execute, by adding a conditional sleep in DrawGlyphs::apply():

if (m_glyphs.size() == 5) {
    WTFLogAlways("First drawGlyphs command. Sleeping...");
    usleep(5000000);
    WTFLogAlways("Done sleeping...");
} else {
    WTFLogAlways("Second drawGlyphs command.");
}

and executing this JS:

function frame() {
    switch (count) {
        case 0:
            context.font = "100px 'Helvetica'";
            context.fillText("Hello", 100, 300);
        case 1:
        case 2:
        case 3:
        case 4:
        case 5:
        case 6:
            requestAnimationFrame(frame);
            break;
        case 7:
            context.fillText("bb", 200, 200);
            break;
    }
    count++;
}
requestAnimationFrame(frame);

After adding various logging, the GPUP's order of execution is:

1. Cache font
2. Enter the loop in RemoteRenderingBackend::nextDestinationImageBufferAfterApplyingDisplayLists()
3. RemoteRenderingBackend::submit() starts executing
4. First drawGlyphs() command
5. Another call to RemoteRenderingBackend::submit() starts executing
6. Second drawGlyphs command
7. Exit the loop in RemoteRenderingBackend::nextDestinationImageBufferAfterApplyingDisplayLists()
8. Release the font
9. Cache the font
10. Release the font

[Said Abou-Hallawa]

Comment on attachment 434159 [details]
Patch

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

> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:37
> +    m_imageBuffers.add(renderingResourceIdentifier, WTFMove(imageBuffer));

If the resource identifier is in the HashMap, the newly decoded resource will be dropped and the existing resource in the HashMap will be reused. Is this okay?

> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:91
> +    if (!value.useOrPendingCount)
> +        m_resourceUseCounters.remove(iterator);

You are right. The scenario you described in https://bugs.webkit.org/show_bug.cgi?id=228222#c5 is possible. And I see now why you are keeping the entry of the resource in m_resourceUseCounters and you are deleting the resource itself below. The assumption is if:

1. value.state == ResourceState::ToBeDeleted
2. value.useOrPendingCount > 0

Then we should have received a ReleaseRemoteResource message and we are waiting for a CacheFoo message for the same resource. But for data integrity and to make the states of the resource caching simpler, I think we should keep both entries in m_resourceUseCounters and the HashMaps always together. The RemoteResourceCache::cacheFoo() methods are now assuming the same resource can be recached.

So I think we can say:

if (value.useOrPendingCount)
    return;
m_resourceUseCounters.remove(iterator);
...

[Myles C. Maxfield]

Comment on attachment 434159 [details]
Patch

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

>> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:37
>> +    m_imageBuffers.add(renderingResourceIdentifier, WTFMove(imageBuffer));
> 
> If the resource identifier is in the HashMap, the newly decoded resource will be dropped and the existing resource in the HashMap will be reused. Is this okay?

Yes, I think this is okay because these resource should be immutable, and the association between renderingResourceIdentifier and resource should be unchanging. So, if two resources share a renderingResourceIdentifier, I think they're supposed to be the same resource with the same data inside them.

(We can hit this situation in the case where the WP does
1. Send cache message
2. Use the resource
3. Send release message with useCount = 1
4. Send cache message again for the same resource
5. [more stuff]

and the GPUP sees
1. Cache message
2. Release message (but we defer the actual removal)
3. Cache message <=== this part here
4. Use the resource
5. [more stuff])

>> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:91
>> +        m_resourceUseCounters.remove(iterator);
> 
> You are right. The scenario you described in https://bugs.webkit.org/show_bug.cgi?id=228222#c5 is possible. And I see now why you are keeping the entry of the resource in m_resourceUseCounters and you are deleting the resource itself below. The assumption is if:
> 
> 1. value.state == ResourceState::ToBeDeleted
> 2. value.useOrPendingCount > 0
> 
> Then we should have received a ReleaseRemoteResource message and we are waiting for a CacheFoo message for the same resource. But for data integrity and to make the states of the resource caching simpler, I think we should keep both entries in m_resourceUseCounters and the HashMaps always together. The RemoteResourceCache::cacheFoo() methods are now assuming the same resource can be recached.
> 
> So I think we can say:
> 
> if (value.useOrPendingCount)
>     return;
> m_resourceUseCounters.remove(iterator);
> ...

Okay, makes sense. I'll update the patch.

[Myles C. Maxfield]

Created attachment 434264 [details]
Patch

[Myles C. Maxfield]

Committed r280334 (239981@main): <https://commits.webkit.org/239981@main>

[Radar WebKit Bug Importer]

<rdar://problem/81141027>