Bug 217558 – [GPU Process][Resource caching 3/7]: Introduce RemoteResourceCacheProxy to manage the remote resources in Web Process

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

[Said Abou-Hallawa]

RemoteRenderingBackend will own an instance of RemoteResourceCache. One type of these resources will be the RemoteImageBuffer. We can split the responsibilities of the remote rendering in Web Process among these classes like this:

-- RemoteRenderingBackend: is responsible for sending messages to and receiving messages from the RemoteRenderingBackendProxy. It will delegate all the resource management to its RemoteResourceCache.
-- RemoteResourceCache: is responsible for managing the remote resources including the actual life cycle of these resources and syncing with the GPU Process RemoteResourceCacheProxy.
-- RemoteImageBuffer: 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.

RemoteResourceCache has ensure the remote resources in GPU Process are available till all the clients which are referencing these remote resources are replayed back.

[Said Abou-Hallawa]

Created attachment 411007 [details]
Patch

[Said Abou-Hallawa]

Created attachment 411025 [details]
Patch for review

[Simon Fraser (smfr)]

Comment on attachment 411025 [details]
Patch for review

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

> Source/WebKit/ChangeLog:17
> +        -- m_remoteImageBufferHashMap maps ImageBuffer* -> RemoteResourceIdentifier

Why can't RemoteImageBuffer just have a RemoteResourceIdentifier?

> Source/WebKit/ChangeLog:19
> +        -- m_remoteImageBufferMessageHandlerHashMap maps RemoteResourceIdentifier
> +           -> RemoteImageBufferMessageHandler*

Can RemoteImageBuffer just own the handler?

[Radar WebKit Bug Importer]

<rdar://problem/70404552>

[Said Abou-Hallawa]

Created attachment 411784 [details]
Patch

[Said Abou-Hallawa]

Created attachment 411787 [details]
Patch

[Said Abou-Hallawa]

Created attachment 411797 [details]
Patch

[Said Abou-Hallawa]

Created attachment 411815 [details]
Patch

[Said Abou-Hallawa]

Created attachment 411819 [details]
Patch

[Said Abou-Hallawa]

Created attachment 411829 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 411829 [details]
Patch

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

Renaming can be done in later patches.

> Source/WebCore/platform/graphics/ImageBuffer.h:65
> +    virtual RemoteResourceIdentifier remoteResourceIdentifier() const { return { }; }

Maybe RemoteResourceIdentifier should be RenderingResourceIdentifier so it doesn't look so out of place here.

> Source/WebKit/WebProcess/GPU/graphics/PlatformRemoteImageBufferProxy.h:47
> +SPECIALIZE_TYPE_TRAITS_BEGIN(WebKit::UnacceleratedRemoteImageBufferProxy)
> +    static bool isType(const WebCore::ImageBuffer& imageBuffer) { return imageBuffer.remoteResourceIdentifier() && !imageBuffer.isAccelerated(); }
> +SPECIALIZE_TYPE_TRAITS_END()
> +
> +#if HAVE(IOSURFACE)
> +SPECIALIZE_TYPE_TRAITS_BEGIN(WebKit::AcceleratedRemoteImageBufferProxy)
> +    static bool isType(const WebCore::ImageBuffer& imageBuffer) { return imageBuffer.remoteResourceIdentifier() && imageBuffer.isAccelerated(); }
> +SPECIALIZE_TYPE_TRAITS_END()

This makes me nervous. Future refactoring could easily make isAccelerated() a dynamic thing on an ImageBuffer. Type traits need to reliably detect which concrete class an object is, not ask questions about its behavior.

I would suggest adding isUnacceleratedRemoteImageBufferProxy(), isAcceleratedRemoteImageBufferProxy() etc.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:69
> +    void commitFlushDisplayList(DisplayListFlushIdentifier flushIdentifier)

Is this a "didCommitFlush" that comes from the GPUP?

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:98
> +            const_cast<RemoteImageBufferProxy&>(*this).flushDisplayList(displayList);

I think we should remove the "flush" terminology for sending bits of display lists. There are really only 2 things we need (with preliminary names):

send(displayList)
waitForFlush()

I also think we should avoid using "commit" in this context.
Also, maybe the flush identifier can just be a special kind of display list item that acts like a marker in the stream.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:130
> +        static constexpr unsigned maxWaitingFlush = 3;

We have to get rid of this timeout. If the wait goes too long, we should just kill the GPU Process.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:137
> +    void flushDisplayList(const WebCore::DisplayList::DisplayList& displayList) override

Why isn't this just send()?

> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:118
> +    sendSync(Messages::RemoteRenderingBackend::GetImageData(outputFormat, srcRect, remoteResourceIdentifier), Messages::RemoteRenderingBackend::GetImageData::Reply(imageDataReference), m_renderingBackendIdentifier, 1_s);

1s timeout is bad. What do we do if it hits?

> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:148
> +    if (imageBuffer->isAccelerated())
> +        downcast<AcceleratedRemoteImageBufferProxy>(*imageBuffer).createBackend(logicalSize, backendSize, resolutionScale, colorSpace, WTFMove(handle));
> +    else
> +        downcast<UnacceleratedRemoteImageBufferProxy>(*imageBuffer).createBackend(logicalSize, backendSize, resolutionScale, colorSpace, WTFMove(handle));

Why is this not just one call to a virtual function?

> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:160
> +    if (imageBuffer->isAccelerated())
> +        downcast<AcceleratedRemoteImageBufferProxy>(*imageBuffer).commitFlushDisplayList(flushIdentifier);
> +    else
> +        downcast<UnacceleratedRemoteImageBufferProxy>(*imageBuffer).commitFlushDisplayList(flushIdentifier);

Why is this not just one call to a virtual function?

[Said Abou-Hallawa]

Created attachment 412367 [details]
Patch

[Said Abou-Hallawa]

Created attachment 412380 [details]
Patch

[Said Abou-Hallawa]

Created attachment 412382 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 411829 [details]
Patch

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

>> Source/WebCore/platform/graphics/ImageBuffer.h:65
>> +    virtual RemoteResourceIdentifier remoteResourceIdentifier() const { return { }; }
> 
> Maybe RemoteResourceIdentifier should be RenderingResourceIdentifier so it doesn't look so out of place here.

Done. Type was renamed to RenderingResourceIdentifier.

>> Source/WebKit/WebProcess/GPU/graphics/PlatformRemoteImageBufferProxy.h:47
>> +SPECIALIZE_TYPE_TRAITS_END()
> 
> This makes me nervous. Future refactoring could easily make isAccelerated() a dynamic thing on an ImageBuffer. Type traits need to reliably detect which concrete class an object is, not ask questions about its behavior.
> 
> I would suggest adding isUnacceleratedRemoteImageBufferProxy(), isAcceleratedRemoteImageBufferProxy() etc.

An ImageBufferBackend should know whether it isAccelerated or not statically since this won't change at run time.  The ImageBuffer::isAccelerated() is virtual so it is a dynamic method . But ConcreteImageBuffer overrides this method to return the BackendType::isAccelerated. So It is dynamic but at compile time only. Do we plan to change the backend of an ImageBuffer at run time?

>> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:69
>> +    void commitFlushDisplayList(DisplayListFlushIdentifier flushIdentifier)
> 
> Is this a "didCommitFlush" that comes from the GPUP?

Yes. RemoteRenderingBackendProxy receives the message FlushDisplayListWasCommitted. RemoteRenderingBackendProxy::flushDisplayListWasCommitted() finds the required RemoteImageBufferProxy and calls its commitFlushDisplayList().

>> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:98
>> +            const_cast<RemoteImageBufferProxy&>(*this).flushDisplayList(displayList);
> 
> I think we should remove the "flush" terminology for sending bits of display lists. There are really only 2 things we need (with preliminary names):
> 
> send(displayList)
> waitForFlush()
> 
> I also think we should avoid using "commit" in this context.
> Also, maybe the flush identifier can just be a special kind of display list item that acts like a marker in the stream.

I agree that the names need wordsmith but we need to look at the whole workflow to decide the naming perfectly. The currently workflow is the following:

WebP: ConcreteImageBuffer::draw() gets called.
WebP: RemoteImageBufferProxy::flushDrawingContext() gets called next.
WebP: RenderingBackendProxy::flushDisplayListAndCommit() gets called next.
WebP: Messages::RemoteRenderingBackend::FlushDisplayListAndCommit is sent with to GPUP.
WebP: RenderingBackendProxy::waitForFlushDisplayListWasCommitted() gets called next and blocks execution.
GPUP: Messages::RemoteRenderingBackend::FlushDisplayListAndCommit is received by GPUP.
GPUP: RemoteRenderingBackend::flushDisplayListAndCommit() gets called next.
GPUP: RemoteImageBuffer::flushDisplayList() gets called next.
GPUP: Messages::RemoteRenderingBackendProxy::FlushDisplayListWasCommitted() is sent to WebP
WebP: Messages::RemoteRenderingBackendProxy::FlushDisplayListWasCommitted() is received by WebP
WebP: RemoteRenderingBackendProxy::flushDisplayListWasCommitted() get called next.
WebP: RemoteImageBufferProxy::commitFlushDisplayList()
WebP: RenderingBackendProxy::waitForFlushDisplayListWasCommitted() returns. 
WebP: RemoteImageBufferProxy::flushDrawingContext()
WebP: ConcreteImageBuffer::draw() draws the ImageBuffer.

RenderingBackendProxy and RenderingBackend are the mediators for all the messages sent by and to RemoteImageBufferProxy and RemoteImageBuffer. So I think the name "send" is a little be insufficient especially there are two types and hence two messages for flushing a displayList: RemoteRenderingBackend::FlushDisplayList and RemoteRenderingBackend::FlushDisplayListAndCommit. 

Also the verb "commit" might not be suitable in this context. But we need a way to differentiate between "flush with reply" and "flush without reply".

>> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:130
>> +        static constexpr unsigned maxWaitingFlush = 3;
> 
> We have to get rid of this timeout. If the wait goes too long, we should just kill the GPU Process.

The loop here is not to extend the waiting time. The loop here tries to wait for the FlushDisplayListWasCommitted message which makes isPendingFlush() false. With batch flushing and eager flushing we may be sending multiple FlushDisplayList messages before requesting to commit all of them.

>> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:118
>> +    sendSync(Messages::RemoteRenderingBackend::GetImageData(outputFormat, srcRect, remoteResourceIdentifier), Messages::RemoteRenderingBackend::GetImageData::Reply(imageDataReference), m_renderingBackendIdentifier, 1_s);
> 
> 1s timeout is bad. What do we do if it hits?

I am not sure. We may need address all the sync and waiting cases and see how to timeout all of them.

>> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:148
>> +        downcast<UnacceleratedRemoteImageBufferProxy>(*imageBuffer).createBackend(logicalSize, backendSize, resolutionScale, colorSpace, WTFMove(handle));
> 
> Why is this not just one call to a virtual function?

Because ImageHandle is defined in WebKit and the first common ancestor of AcceleratedRemoteImageBufferProxy and UnacceleratedRemoteImageBufferProxy is ImageBuffer.

>> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:160
>> +        downcast<UnacceleratedRemoteImageBufferProxy>(*imageBuffer).commitFlushDisplayList(flushIdentifier);
> 
> Why is this not just one call to a virtual function?

Because commitFlushDisplayList and DisplayListFlushIdentifier are only known in WebKit.

[EWS]

Committed r269022: <https://trac.webkit.org/changeset/269022>

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