Bug 227791 – [GPU Process] Regulate the WebPage RenderingUpdates from the WebProcess to the GPUProcess

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

[Said Abou-Hallawa]

Although RemoteRenderingBackend::stopListeningForIPC() removes its m_workQueue from the connection WorkQueueMessageReceiver HashMap, m_workQueue can still process accumulated received messages. Removing WorkQueueMessageReceiver will call the destructor ~WorkQueueMessageReceiver but the m_queue of the deleted object can still be running. The reason is the lambda of the WorkQueue::dispatch() in WorkQueueMessageReceiverQueue::enqueueMessage() captures the receiver which is RemoteRenderingBackend as a Ref pointer. Because RemoteRenderingBackend holds the last reference to WorkQueue, m_workQueue can run even after RemoteRenderingBackend::stopListeningForIPC() is called.

[Said Abou-Hallawa]

<rdar://78430639>

[Said Abou-Hallawa]

Created attachment 433122 [details]
Patch

[Said Abou-Hallawa]

Created attachment 433124 [details]
Patch

[Simon Fraser (smfr)]

This does not fix the "navigating away from the page" case. What's the plan for that?

[Simon Fraser (smfr)]

Comment on attachment 433124 [details]
Patch

r- pending an answer to the question

[Said Abou-Hallawa]

Created attachment 433568 [details]
Patch

Patch for EWS

[Said Abou-Hallawa]

Created attachment 433620 [details]
Patch

Patch for EWS

[Said Abou-Hallawa]

Created attachment 433729 [details]
Patch for EWS

[Said Abou-Hallawa]

Created attachment 433746 [details]
Patch for EWS

[Said Abou-Hallawa]

Created attachment 433771 [details]
Patch

[Sam Weinig]

Comment on attachment 433771 [details]
Patch

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

> Tools/WebKitTestRunner/TestOptions.cpp:124
> +            { "RenderingUpdateRegulateEnabled", false },

This should not be needed.

[Sam Weinig]

Comment on attachment 433771 [details]
Patch

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

> Tools/WebKitTestRunner/TestOptions.cpp:124
> +            { "RenderingUpdateRegulateEnabled", false },

This should not be needed.

[Said Abou-Hallawa]

Created attachment 433805 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 433805 [details]
Patch

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

> Source/WebCore/page/RenderingUpdateScheduler.h:70
> +    unsigned m_rescheduledRenderingUpdates { 0 };

m_rescheduledRenderingUpdateCount

Move it up before the bools to optimize padding.

> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:293
> +    m_didFinalizeRenderingUpdateVersion = std::min(didFinalizeRenderingUpdateVersion, m_finalizeRenderingUpdateVersion);

Isn't it a bug if didFinalizeRenderingUpdateVersion and m_finalizeRenderingUpdateVersion are out of order?

> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.h:158
> +    uint64_t m_finalizeRenderingUpdateVersion { 1 };
> +    uint64_t m_didFinalizeRenderingUpdateVersion { 1 };

I think these should be a type defined as MonotonicObjectIdentifier<>.

Can one be named as "last completed" or something? This is closer to the naming we use for TransactionIDs.

[Said Abou-Hallawa]

Created attachment 433861 [details]
Patch

[Said Abou-Hallawa]

Created attachment 433985 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 433805 [details]
Patch

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

>> Source/WebCore/page/RenderingUpdateScheduler.h:70
>> +    unsigned m_rescheduledRenderingUpdates { 0 };
> 
> m_rescheduledRenderingUpdateCount
> 
> Move it up before the bools to optimize padding.

Done.

>> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:293
>> +    m_didFinalizeRenderingUpdateVersion = std::min(didFinalizeRenderingUpdateVersion, m_finalizeRenderingUpdateVersion);
> 
> Isn't it a bug if didFinalizeRenderingUpdateVersion and m_finalizeRenderingUpdateVersion are out of order?

Yes it is a bug if we are the only who sends didFinalizeRenderingUpdateVersion larger than m_finalizeRenderingUpdateVersion. And this is why I added the debug ASSERT above. But if the argument of the IPC message RemoteRenderingBackendProxy::DidFinalizeRenderingUpdate was altered when sending it from GPUProcess to WebProcess, I thought we can protect ourselves from this underflow if we just use std::min().

     unsigned delayedRenderingUpdates() const { return m_finalizeRenderingUpdateVersion - m_didFinalizeRenderingUpdateVersion; }

>> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.h:158
>> +    uint64_t m_didFinalizeRenderingUpdateVersion { 1 };
> 
> I think these should be a type defined as MonotonicObjectIdentifier<>.
> 
> Can one be named as "last completed" or something? This is closer to the naming we use for TransactionIDs.

Done.

[Said Abou-Hallawa]

Created attachment 434227 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 434227 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        certain number of rescheduling.

reschedules

> Source/WebKit/Shared/MonotonicObjectIdentifier.h:129
> +template<typename T>
> +inline unsigned operator-(const MonotonicObjectIdentifier<T>& a, const MonotonicObjectIdentifier<T>& b)
> +{
> +    return a.toUInt64() - b.toUInt64();
> +}

I think this needs to return a signed Int64 value to avoid mistakes.

> Source/WebKit/WebProcess/WebPage/WebPage.h:402
> +    bool canTriggerRenderingUpdate(unsigned rescheduledRenderingUpdateCount) const;

canTriggerRenderingUpdate -> shouldTriggerRenderingUpdate

[Chris Dumez]

Comment on attachment 434227 [details]
Patch

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

> Source/WebKit/Shared/MonotonicObjectIdentifier.h:35
> +template<typename T> class MonotonicObjectIdentifier {

I think this could use a comment to explain its purpose and how it differs from ObjectIdentifier. Isn't an ObjectIdentifier monotonic? It definitely can only increment.

[Said Abou-Hallawa]

Created attachment 434253 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 434227 [details]
Patch

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

>> Source/WebCore/ChangeLog:12
>> +        certain number of rescheduling.
> 
> reschedules

Fixed.

>> Source/WebKit/Shared/MonotonicObjectIdentifier.h:35
>> +template<typename T> class MonotonicObjectIdentifier {
> 
> I think this could use a comment to explain its purpose and how it differs from ObjectIdentifier. Isn't an ObjectIdentifier monotonic? It definitely can only increment.

I added a comment describing the difference between the two and a FIXME suggesting merging them.

>> Source/WebKit/Shared/MonotonicObjectIdentifier.h:129
>> +}
> 
> I think this needs to return a signed Int64 value to avoid mistakes.

Done. I added a Checked calculation to deal with the overflow.

>> Source/WebKit/WebProcess/WebPage/WebPage.h:402
>> +    bool canTriggerRenderingUpdate(unsigned rescheduledRenderingUpdateCount) const;
> 
> canTriggerRenderingUpdate -> shouldTriggerRenderingUpdate

Fixed.

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 434253 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Said Abou-Hallawa]

Created attachment 434265 [details]
Patch

[EWS]

Committed r280337 (239984@main): <https://commits.webkit.org/239984@main>

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