Bug 217554 – [GPU Process][Resource caching 2/7]: Introduce RemoteResourceCache to manage the resources in the GPU Process

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

RemoteRenderingBackendProxy will own an instance of RemoteResourceCacheProxy. One type of these resources will be the RemoteImageBufferProxy. We can split the responsibilities of the remote rendering among these classes like this:

-- RemoteRenderingBackendProxy: is responsible for sending messages to and receiving messages from the RemoteRenderingBackend. It will delegate all the resource management to its RemoteResourceCacheProxy
-- RemoteResourceCacheProxy: is responsible for managing the remote resources including the actual life cycle of these resources.
-- RemoteImageBufferProxy: is responsible for the ImageBuffer functionalities taking into consideration it is a remote resource, i.e drawing is recorded in WebProcess and backend lives in the GPU Process.

This makes it easy to get rid of RemoteImageBufferMessageHandlerProxy because all it does is sending messages to WebProcess but we can simplify the interface of RemoteImageBufferProxy by removing it and moving sending the messages to RemoteRenderingBackendProxy.

[Said Abou-Hallawa]

Created attachment 411005 [details]
Patch

[Said Abou-Hallawa]

Created attachment 411006 [details]
Patch for review

[Simon Fraser (smfr)]

Comment on attachment 411006 [details]
Patch for review

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

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.cpp:74
> +void RemoteRenderingBackendProxy::remoteCreateImageBufferBackend(const FloatSize& logicalSize, const IntSize& backendSize, float resolutionScale, ColorSpace colorSpace, ImageBufferBackendHandle handle, RemoteResourceIdentifier remoteResourceIdentifier)

Or createRemoteImageBufferBackend?

Does this have to do IPC? Could we create the image buffer backend lazily the first time it's referenced by something?

> Source/WebKit/GPUProcess/graphics/RemoteResourceCacheProxy.cpp:76
> +    if (auto image = flushDrawingContext(displayList, remoteResourceIdentifier)) {
> +        image->flushContext();

It's confusing that we have both flushDrawingContext() and flushContext(), but we also have the "commit" terminology for flushContext().

> Source/WebKit/GPUProcess/graphics/RemoteResourceCacheProxy.h:50
> +    WebCore::ImageBuffer* flushDrawingContext(const WebCore::DisplayList::DisplayList&, WebCore::RemoteResourceIdentifier);
> +    WebCore::ImageBuffer* flushDrawingContextAndCommit(const WebCore::DisplayList::DisplayList&, WebCore::RemoteResourceIdentifier);

I'm a bit confused by flushDrawing() need to take a DisplayList argument. In my mind, "flush" means "execute drawing commands that you've already received".

> Source/WebKit/GPUProcess/graphics/RemoteResourceCacheProxy.h:58
> +    RemoteImageBufferHashMap m_remoteImageBufferHashMap;

Don't put "HashMap" in the name. Maybe m_identifierToBufferMap, or just m_imageBuffers.

[Said Abou-Hallawa]

Created attachment 411199 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 411006 [details]
Patch for review

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

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.cpp:74
>> +void RemoteRenderingBackendProxy::remoteCreateImageBufferBackend(const FloatSize& logicalSize, const IntSize& backendSize, float resolutionScale, ColorSpace colorSpace, ImageBufferBackendHandle handle, RemoteResourceIdentifier remoteResourceIdentifier)
> 
> Or createRemoteImageBufferBackend?
> 
> Does this have to do IPC? Could we create the image buffer backend lazily the first time it's referenced by something?

Name was changed. Eventually we will not send this message to the Web Process because we do not want it to have access to the backend. We need it now because the canvas ImageBuffer is still drawn to the layer in Web Process which should not happen.

>> Source/WebKit/GPUProcess/graphics/RemoteResourceCacheProxy.cpp:76
>> +        image->flushContext();
> 
> It's confusing that we have both flushDrawingContext() and flushContext(), but we also have the "commit" terminology for flushContext().

I renamed it to applyDisplayList() since all RemoteImageBufferProxy does is applying a DisplayList to the backend.

>> Source/WebKit/GPUProcess/graphics/RemoteResourceCacheProxy.h:58
>> +    RemoteImageBufferHashMap m_remoteImageBufferHashMap;
> 
> Don't put "HashMap" in the name. Maybe m_identifierToBufferMap, or just m_imageBuffers.

Done.

[Simon Fraser (smfr)]

Comment on attachment 411199 [details]
Patch

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

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.cpp:76
> +    send(Messages::RemoteRenderingBackend::CreateRemoteImageBufferBackend(logicalSize, backendSize, resolutionScale, colorSpace, WTFMove(handle), remoteResourceIdentifier), m_renderingBackendIdentifier);

Do we need separate IPC to create the remote buffer backend, or can we just create them when first referenced, and send the IDs in a reply?

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.cpp:79
> +void RemoteRenderingBackendProxy::commitApplyDisplayList(ImageBufferFlushIdentifier flushIdentifier, RemoteResourceIdentifier remoteResourceIdentifier)

"commitApply" is weird naming. Can this just use 'flush"?

> Source/WebKit/GPUProcess/graphics/RemoteResourceCacheProxy.cpp:41
> +void RemoteResourceCacheProxy::createImageBuffer(const FloatSize& logicalSize, RenderingMode renderingMode, float resolutionScale, ColorSpace colorSpace, RemoteResourceIdentifier remoteResourceIdentifier)

Should the cache be the one to create the buffer? Normally you just add things to caches, not have the cache create things.

> Source/WebKit/GPUProcess/graphics/RemoteResourceCacheProxy.cpp:75
> +        image->flushContext();
> +        m_remoteRenderingBackendProxy.commitApplyDisplayList(flushIdentifier, remoteResourceIdentifier);

The mixture of "commit" and "flush" is confusing here.

Should the function on m_remoteRenderingBackendProxy be called didFlushDrawingForResource?

> Source/WebKit/GPUProcess/graphics/RemoteResourceCacheProxy.cpp:79
> +RefPtr<ImageData> RemoteResourceCacheProxy::getImageData(AlphaPremultiplication outputFormat, const IntRect& srcRect, RemoteResourceIdentifier remoteResourceIdentifier) const

This doesn't seem like it should live on the resource cache.

[Said Abou-Hallawa]

Created attachment 411459 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 411199 [details]
Patch

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

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.cpp:76
>> +    send(Messages::RemoteRenderingBackend::CreateRemoteImageBufferBackend(logicalSize, backendSize, resolutionScale, colorSpace, WTFMove(handle), remoteResourceIdentifier), m_renderingBackendIdentifier);
> 
> Do we need separate IPC to create the remote buffer backend, or can we just create them when first referenced, and send the IDs in a reply?

When ImageBuffer::create() is called for a GPU based ImageBuffer, we have to create an instance of RemoteImageBuffer. RemoteImageBuffer offers the GraphicsContext which is backed by a DisplayList::Recorder. Usually the caller will not need the backend immediately because it has to draw first. At this time RemoteImageBufferProxy is created in GPU Process and it sends its ImageBufferHandle to RemoteImageBuffer. So this workflow works nicely with the common case of using ImageBuffer and at the same time it is asynchronous. So I am not sure why do we have to delay sending this message till the backend is needed? This should cause a delay while sending the message, waiting for the replay and creating the backend.

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.cpp:79
>> +void RemoteRenderingBackendProxy::commitApplyDisplayList(ImageBufferFlushIdentifier flushIdentifier, RemoteResourceIdentifier remoteResourceIdentifier)
> 
> "commitApply" is weird naming. Can this just use 'flush"?

Function was renamed commitFlushDisplayList().

>> Source/WebKit/GPUProcess/graphics/RemoteResourceCacheProxy.cpp:41
>> +void RemoteResourceCacheProxy::createImageBuffer(const FloatSize& logicalSize, RenderingMode renderingMode, float resolutionScale, ColorSpace colorSpace, RemoteResourceIdentifier remoteResourceIdentifier)
> 
> Should the cache be the one to create the buffer? Normally you just add things to caches, not have the cache create things.

I was seeing RemoteResourceCacheProxy not only a resource cache but also a resource manager. But having the "Cache" in the name makes it confusing to make it create the objects as well. So I moved creating the ImageBuffers back to RemoteRenderingBackendProxy.

>> Source/WebKit/GPUProcess/graphics/RemoteResourceCacheProxy.cpp:75
>> +        m_remoteRenderingBackendProxy.commitApplyDisplayList(flushIdentifier, remoteResourceIdentifier);
> 
> The mixture of "commit" and "flush" is confusing here.
> 
> Should the function on m_remoteRenderingBackendProxy be called didFlushDrawingForResource?

I moved this function back to RemoteRenderingBackendProxy.

>> Source/WebKit/GPUProcess/graphics/RemoteResourceCacheProxy.cpp:79
>> +RefPtr<ImageData> RemoteResourceCacheProxy::getImageData(AlphaPremultiplication outputFormat, const IntRect& srcRect, RemoteResourceIdentifier remoteResourceIdentifier) const
> 
> This doesn't seem like it should live on the resource cache.

I moved this function back to RemoteRenderingBackendProxy.

[Said Abou-Hallawa]

Created attachment 411634 [details]
Patch

[Simon Fraser (smfr)]

(In reply to Said Abou-Hallawa from comment #8)
> Comment on attachment 411199 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411199&action=review
> 
> >> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.cpp:76
> >> +    send(Messages::RemoteRenderingBackend::CreateRemoteImageBufferBackend(logicalSize, backendSize, resolutionScale, colorSpace, WTFMove(handle), remoteResourceIdentifier), m_renderingBackendIdentifier);
> > 
> > Do we need separate IPC to create the remote buffer backend, or can we just create them when first referenced, and send the IDs in a reply?
> 
> When ImageBuffer::create() is called for a GPU based ImageBuffer, we have to
> create an instance of RemoteImageBuffer. RemoteImageBuffer offers the
> GraphicsContext which is backed by a DisplayList::Recorder. Usually the
> caller will not need the backend immediately because it has to draw first.
> At this time RemoteImageBufferProxy is created in GPU Process and it sends
> its ImageBufferHandle to RemoteImageBuffer. So this workflow works nicely
> with the common case of using ImageBuffer and at the same time it is
> asynchronous. So I am not sure why do we have to delay sending this message
> till the backend is needed? This should cause a delay while sending the
> message, waiting for the replay and creating the backend.

This is in GPUProcess/graphics/RemoteRenderingBackendProxy, so I assume it's sending IPC from GPU to WebProcess, which is confusing because the WebProcess should be the one creating buffer proxies which are then materialized in the GPU process.

> 
> >> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.cpp:79
> >> +void RemoteRenderingBackendProxy::commitApplyDisplayList(ImageBufferFlushIdentifier flushIdentifier, RemoteResourceIdentifier remoteResourceIdentifier)
> > 
> > "commitApply" is weird naming. Can this just use 'flush"?
> 
> Function was renamed commitFlushDisplayList().

Not ideal but we can wordsmith later.

> >> Source/WebKit/GPUProcess/graphics/RemoteResourceCacheProxy.cpp:41
> >> +void RemoteResourceCacheProxy::createImageBuffer(const FloatSize& logicalSize, RenderingMode renderingMode, float resolutionScale, ColorSpace colorSpace, RemoteResourceIdentifier remoteResourceIdentifier)
> > 
> > Should the cache be the one to create the buffer? Normally you just add things to caches, not have the cache create things.
> 
> I was seeing RemoteResourceCacheProxy not only a resource cache but also a
> resource manager. But having the "Cache" in the name makes it confusing to
> make it create the objects as well. So I moved creating the ImageBuffers
> back to RemoteRenderingBackendProxy.

Seems better.

[Said Abou-Hallawa]

Created attachment 411645 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 411199 [details]
Patch

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

>>>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.cpp:76
>>>> +    send(Messages::RemoteRenderingBackend::CreateRemoteImageBufferBackend(logicalSize, backendSize, resolutionScale, colorSpace, WTFMove(handle), remoteResourceIdentifier), m_renderingBackendIdentifier);
>>> 
>>> Do we need separate IPC to create the remote buffer backend, or can we just create them when first referenced, and send the IDs in a reply?
>> 
>> When ImageBuffer::create() is called for a GPU based ImageBuffer, we have to create an instance of RemoteImageBuffer. RemoteImageBuffer offers the GraphicsContext which is backed by a DisplayList::Recorder. Usually the caller will not need the backend immediately because it has to draw first. At this time RemoteImageBufferProxy is created in GPU Process and it sends its ImageBufferHandle to RemoteImageBuffer. So this workflow works nicely with the common case of using ImageBuffer and at the same time it is asynchronous. So I am not sure why do we have to delay sending this message till the backend is needed? This should cause a delay while sending the message, waiting for the replay and creating the backend.
> 
> This is in GPUProcess/graphics/RemoteRenderingBackendProxy, so I assume it's sending IPC from GPU to WebProcess, which is confusing because the WebProcess should be the one creating buffer proxies which are then materialized in the GPU process.

RemoteRenderingBackendProxy messages were renamed: ImageBufferBackendWasCreated and FlushDisplayListWasCommitted.

[Radar WebKit Bug Importer]

<rdar://problem/70401828>

[Said Abou-Hallawa]

Created attachment 411656 [details]
Patch

[EWS]

Committed r268637: <https://trac.webkit.org/changeset/268637>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411656 [details].