Bug 228673 – [GPUProcess] REGRESSION: A noticeable slow down when browsing Live Photos album on iCloud.com

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Said Abou-Hallawa]

Appending FlushContext to the DL makes GPUProcess do two things:
1. Flushes the backend context.
2. Sends a DidFlush message back to the WebProcess.

One of the callers of RemoteImageBufferProxy::flushDrawingContextAsync() is RemoteImageBufferProxy::flushDrawingContext() which assumes FlushContext is added all the times and this is why it waits for 3 seconds and then gives up. But flushDrawingContextAsync() adds FlushContext only 'if (!m_drawingContext.displayList().isEmpty())'. The opposite can happen very often since clearDisplayList() is called from flushDrawingContextAsync() itself. So we can waste three seconds waiting for a message that will never be received because the DisplayList is empty.

If 'm_drawingContext.displayList().isEmpty()' is true, appending FlushContext may not be needed. But we do not know for sure if the GPUProcess is applying or will be applying items from the SharedBuffer for this ImageBuffer. We need to ensure all the items are processed and we may be waiting for a DidFlush message. So the solution is to make RemoteImageBufferProxy::flushDrawingContextAsync() append FlushContext to its DisplayList always.

[Said Abou-Hallawa]

Created attachment 434683 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/81353138>

[Wenson Hsieh]

Comment on attachment 434683 [details]
Patch

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

Is there a reason why we don't just check `!m_drawingContext.displayList().isEmpty()` in `flushDrawingContext()`?

> Source/WebKit/ChangeLog:15
> +        the backend context is flushed. Also we need GPUProces to send the DidFush

Nits - GPUProces => GPUProcess, DidFush => DidFlush

[Said Abou-Hallawa]

Created attachment 434872 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 434683 [details]
Patch

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

I replaced 'if (!m_drawingContext.displayList().isEmpty())' by 'if (!hasPendingFlush())'. I think this makes more sense.

Suppose CanvasRenderingContext2DBase::prepareForDisplay() calls RemoteImageBufferProxy::flushDrawingContextAsync() and this one adds the FlushContext item. Later and before we receive the DidFlush, ConcreteImageBuffer::copyNativeImage() calls flushDrawingContext(). In this case we will realize there is a pending FlushContext and we are not going to send a new one. We wait for DidFlush and it arrives as expected.

Now suppose for example the destructor RemoteImageBufferProxy::~RemoteImageBufferProxy() calls flushDrawingContext(). It will add the FlushContext since it does not have a pending one. flushDrawingContext() waits for DidFlush and it arrives as expected.

>> Source/WebKit/ChangeLog:15
>> +        the backend context is flushed. Also we need GPUProces to send the DidFush
> 
> Nits - GPUProces => GPUProcess, DidFush => DidFlush

Fixed.

[Said Abou-Hallawa]

Created attachment 434890 [details]
Patch

[Said Abou-Hallawa]

I changed the patch to add !hasPendingFlush() as a condition to add FlushContext item.

Keeping the condition "!m_drawingContext.displayList().isEmpty()" will help with a sequence like the following:

    FillRect       // Drawing to RemoteImageBufferProxy
    ...
    FlushContext   // Added by flushDrawingContextAsync() because the DL is not empty (there is no pending FlushContext)
    ...
    StrokeRect     // Continue drawing to RemoteImageBufferProxy
    ...
    FlushContext   // Added by flushDrawingContextAsync() because the DL is not empty (there is a pending FlushContext)

The condition "!hasPendingFlush()" will add the FlushContext even if the DL is empty which will force sending WakeUpAndApplyDisplayList to GPUP so we can guarantee the GPUP will send a DidFlush message.

[EWS]

Committed r280652 (240263@main): <https://commits.webkit.org/240263@main>

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