[webkit-reviews] review granted: [Bug 231414] [GPU Process] Unique RenderingResourceIdentifiers Part 9: Finish migrating RemoteResourceCache to QualifiedRenderingResourceIdentifier : [Attachment 440909] Patch

[bugzilla-daemon at webkit.org]

Said Abou-Hallawa <sabouhallawa at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 231414: [GPU Process] Unique RenderingResourceIdentifiers Part 9: Finish
migrating RemoteResourceCache to QualifiedRenderingResourceIdentifier
https://bugs.webkit.org/show_bug.cgi?id=231414

Attachment 440909: Patch

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




--- Comment #6 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 440909
  --> https://bugs.webkit.org/attachment.cgi?id=440909
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?