Bug 231414 – [GPU Process] Unique RenderingResourceIdentifiers Part 9: Finish migrating RemoteResourceCache to QualifiedRenderingResourceIdentifier

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Myles C. Maxfield]

Finish migrating RemoteResourceCache to QualifiedRenderingResourceIdentifier

[Radar WebKit Bug Importer]

<rdar://problem/84015584>

[Myles C. Maxfield]

Created attachment 440576 [details]
Patch

[Myles C. Maxfield]

Created attachment 440640 [details]
Patch

[Myles C. Maxfield]

Created attachment 440884 [details]
Patch

[Myles C. Maxfield]

Created attachment 440909 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 440909 [details]
Patch

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

> Source/WebCore/platform/graphics/displaylists/DisplayListResourceHeap.h:47
> +class ResourceHeapImpl : public ResourceHeap {

This name does not reveal the difference between ResourceHeapImpl and QualifiedResourceHeap. Can't it be named LocalResourceHeap or RenderingResourceHeap or something else?

> Source/WebCore/platform/graphics/displaylists/DisplayListResourceHeap.h:51
> +        m_resources.add(renderingResourceIdentifier, RefPtr<ImageBuffer>(WTFMove(imageBuffer)));

Why do we need to"add()" a RefPtr<> even though the input is a Ref<>? Also I think RefPtr<ImageBuffer>(WTFMove(imageBuffer)) can be just WTFMove(imageBuffer).

> Source/WebCore/platform/graphics/displaylists/DisplayListResourceHeap.h:96
>      using Resource = Variant<RefPtr<ImageBuffer>, RefPtr<NativeImage>, RefPtr<Font>>;

I think this should be a Variant of Ref<> resources since we never add a null pointer to it.

> Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:49
> +        add<WebCore::ImageBuffer>(renderingResourceIdentifier, WTFMove(imageBuffer), m_imageBufferCount);

Do we need to use explicit template specialization here? Can't it be implicit 

    add(renderingResourceIdentifier, WTFMove(imageBuffer), m_imageBufferCount);

> Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:109
> +        return remove<WebCore::ImageBuffer>(renderingResourceIdentifier, m_imageBufferCount);

Do we need to make an early return here:

    if (!m_imageBufferCount)
        return false;

> Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:124
> +        m_resources.removeIf([] (const auto& resource) {

Do we need to make an early return here:

    if (!m_fontCount)
        return;

> Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:136
> +            ++counter;

Maybe we need to assert here

    ASSERT(m_resources.size() == m_imageBufferCount + m_nativeImageCount + m_fontCount);

> Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:146
> +        if (!WTF::holds_alternative<RefPtr<T>>(iterator->value))
> +            return nullptr;

I think we should ASSERT(WTF::holds_alternative...) since it should be a bug in our code to get an Image with the ResourceIdentifier of a font for example.

> Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:153
> +        auto iterator = m_resources.find(renderingResourceIdentifier);

Maybe we need to assert here

    ASSERT(counter);

> Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:157
> +        if (!WTF::holds_alternative<RefPtr<T>>(iterator->value))
> +            return false;

I think we should ASSERT(WTF::holds_alternative...) since it should be a bug in our code to remove an Image with the ResourceIdentifier of a font for example.

> Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:164
> +    using Resource = Variant<RefPtr<WebCore::ImageBuffer>, RefPtr<WebCore::NativeImage>, RefPtr<WebCore::Font>>;

I think this should be a Variant of Ref<> resources since we never add a null pointer to it.

> Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:169
> +    HashMap<QualifiedRenderingResourceIdentifier, Resource> m_resources;
> +    WebCore::ProcessIdentifier m_webProcessIdentifier;
> +    unsigned m_imageBufferCount { 0 };
> +    unsigned m_nativeImageCount { 0 };
> +    unsigned m_fontCount { 0 };

What is the benefit of having a HashMap of the variant and three counters over having three HashMaps one for each resource type?

[Myles C. Maxfield]

Comment on attachment 440909 [details]
Patch

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

>> Source/WebCore/platform/graphics/displaylists/DisplayListResourceHeap.h:96
>>      using Resource = Variant<RefPtr<ImageBuffer>, RefPtr<NativeImage>, RefPtr<Font>>;
> 
> I think this should be a Variant of Ref<> resources since we never add a null pointer to it.

Yep! Alex just taught me how to do this. Very cool.

>> Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:109
>> +        return remove<WebCore::ImageBuffer>(renderingResourceIdentifier, m_imageBufferCount);
> 
> Do we need to make an early return here:
> 
>     if (!m_imageBufferCount)
>         return false;

Good idea. I should probably put it in remove<>() though, so it's shared.

>> Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:136
>> +            ++counter;
> 
> Maybe we need to assert here
> 
>     ASSERT(m_resources.size() == m_imageBufferCount + m_nativeImageCount + m_fontCount);

Yeah, this is a good idea since it's an invariant. I'll put this in a helper and call the helper in a bunch of different places.

>> Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:157
>> +            return false;
> 
> I think we should ASSERT(WTF::holds_alternative...) since it should be a bug in our code to remove an Image with the ResourceIdentifier of a font for example.

I thought so too, but in this specific case it's actually not:

bool RemoteResourceCache::maybeRemoveResource(...)
{
    ...
    if (m_resourceHeap.removeImageBuffer(renderingResourceIdentifier)
        || m_resourceHeap.removeNativeImage(renderingResourceIdentifier)) {
        updateHasActiveDrawables();
        return true;
    }
    if (m_resourceHeap.removeFont(renderingResourceIdentifier))
        return true;
   ...
}

add() and get() should ASSERT() though.

>> Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:169
>> +    unsigned m_fontCount { 0 };
> 
> What is the benefit of having a HashMap of the variant and three counters over having three HashMaps one for each resource type?

https://bugs.webkit.org/show_bug.cgi?id=231411#c7

Three reasons:
1. Less wasted space in the maps
2. It's a bug if differently typed resources shared the same RRI
3. Simon says "it’s semantically better"

[Myles C. Maxfield]

Created attachment 440980 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 440985 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 440992 [details]
Patch for committing

[Myles C. Maxfield]

Committed r284048 (242877@main): <https://commits.webkit.org/242877@main>