Summary: | [GPUProcess] REGRESSION: A noticeable slow down when browsing Live Photos album on iCloud.com | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Said Abou-Hallawa <sabouhallawa> | ||||||||
Component: | Canvas | Assignee: | Said Abou-Hallawa <sabouhallawa> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | dino, simon.fraser, webkit-bug-importer, wenson_hsieh | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=239799 | ||||||||||
Attachments: |
|
Description
Said Abou-Hallawa
2021-07-30 17:27:15 PDT
Created attachment 434683 [details]
Patch
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 Created attachment 434872 [details]
Patch
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. Created attachment 434890 [details]
Patch
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. 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]. |