Bug 203186 – Suspend dedicated worker threads while in the back/forward cache

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Chris Dumez]

Suspend ActiveDOMObjects in dedicated workers while in the back/forward cache, to avoid wasting CPU.

[Chris Dumez]

Created attachment 381395 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/56447493>

[Ryosuke Niwa]

Comment on attachment 381395 [details]
Patch

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

> Source/WebCore/workers/WorkerMessagingProxy.cpp:138
> +        m_workerThread->runLoop().postTask(WTFMove(task));

Is it okay that the worker would continue to run until it gets this message?

> Source/WebCore/workers/WorkerMessagingProxy.cpp:140
> +        m_queuedEarlyTasks.append(makeUnique<ScriptExecutionContext::Task>(WTFMove(task)));

Is it really necessary to enqueue this task if there is no worker thread??

[Ryosuke Niwa]

Do we have a mechanism to stop the worker when the cached frame is discarded?

[Chris Dumez]

Comment on attachment 381395 [details]
Patch

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

>> Source/WebCore/workers/WorkerMessagingProxy.cpp:138
>> +        m_workerThread->runLoop().postTask(WTFMove(task));
> 
> Is it okay that the worker would continue to run until it gets this message?

I don’t see why not. It is only an optimization, we are stopping the objects as soon as we can.

>> Source/WebCore/workers/WorkerMessagingProxy.cpp:140
>> +        m_queuedEarlyTasks.append(makeUnique<ScriptExecutionContext::Task>(WTFMove(task)));
> 
> Is it really necessary to enqueue this task if there is no worker thread??

I followed the pattern in postMessage. It seems safer. Otherwise, if the page gets suspended while the worker thread is still starting, it may not know about suspension and run as if not suspended. Similarly as we don’t want to miss messages posted to workers , we do not want them to miss requests to suspend / resume.

[Chris Dumez]

Comment on attachment 381395 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Suspend ActiveDOMObjects in dedicated workers while in the back/forward cache, to avoid wasting CPU.

We may be able to suspend workers more aggressively by leveraging Thread::suspend() / Thread::resume().

[Ryosuke Niwa]

Comment on attachment 381395 [details]
Patch

Ok, perhaps we can do that in a follow up.

[Chris Dumez]

Created attachment 381444 [details]
Patch

[Chris Dumez]

Comment on attachment 381444 [details]
Patch

Geoff has a better idea.

[Chris Dumez]

Created attachment 381458 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 381458 [details]
Patch

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

> Source/WebCore/workers/Worker.h:102
> +    bool isSuspendedForBackForwardCache { false };

Maybe put this after m_shouldBypassMainWorldContentSecurityPolicy to pack better?

> Source/WebCore/workers/WorkerGlobalScopeProxy.h:57
> +    // Back / Forward cache support.
> +    virtual void suspend() = 0;
> +    virtual void resume() = 0;

Why don't we just call them suspendForBackForwardCache & resumeForBackForwardCache instead of adding a comment?

> Source/WebCore/workers/WorkerThread.h:141
> +    Lock m_suspensionLock;
> +    Condition m_suspensionCondition;

Use BinarySemaphore instead?

[Chris Dumez]

Created attachment 381493 [details]
Patch

[Chris Dumez]

Created attachment 381498 [details]
Patch

[Geoffrey Garen]

Comment on attachment 381498 [details]
Patch

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

> Source/WebCore/workers/Worker.h:100
> +    bool isSuspendedForBackForwardCache { false };

m_isSuspendedForBackForwardCache

[Chris Dumez]

Created attachment 381503 [details]
Patch

[Chris Dumez]

(In reply to Geoffrey Garen from comment #14)
> Comment on attachment 381498 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381498&action=review
> 
> > Source/WebCore/workers/Worker.h:100
> > +    bool isSuspendedForBackForwardCache { false };
> 
> m_isSuspendedForBackForwardCache

Oops. Thanks for catching it.

[WebKit Commit Bot]

Comment on attachment 381503 [details]
Patch

Clearing flags on attachment: 381503

Committed r251416: <https://trac.webkit.org/changeset/251416>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.