Bug 203186

Summary: Suspend dedicated worker threads while in the back/forward cache
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Page LoadingAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, commit-queue, ggaren, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 203107    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2019-10-20 15:15:56 PDT
Suspend ActiveDOMObjects in dedicated workers while in the back/forward cache, to avoid wasting CPU.
Comment 1 Chris Dumez 2019-10-20 15:55:30 PDT
Created attachment 381395 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2019-10-20 15:56:34 PDT
<rdar://problem/56447493>
Comment 3 Ryosuke Niwa 2019-10-20 18:28:22 PDT
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??
Comment 4 Ryosuke Niwa 2019-10-20 18:29:12 PDT
Do we have a mechanism to stop the worker when the cached frame is discarded?
Comment 5 Chris Dumez 2019-10-20 18:40:43 PDT
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.
Comment 6 Chris Dumez 2019-10-21 12:53:04 PDT
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().
Comment 7 Ryosuke Niwa 2019-10-21 13:00:51 PDT
Comment on attachment 381395 [details]
Patch

Ok, perhaps we can do that in a follow up.
Comment 8 Chris Dumez 2019-10-21 13:48:48 PDT
Created attachment 381444 [details]
Patch
Comment 9 Chris Dumez 2019-10-21 14:10:54 PDT
Comment on attachment 381444 [details]
Patch

Geoff has a better idea.
Comment 10 Chris Dumez 2019-10-21 14:33:41 PDT
Created attachment 381458 [details]
Patch
Comment 11 Ryosuke Niwa 2019-10-21 15:49:03 PDT
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?
Comment 12 Chris Dumez 2019-10-21 18:21:51 PDT
Created attachment 381493 [details]
Patch
Comment 13 Chris Dumez 2019-10-21 19:05:58 PDT
Created attachment 381498 [details]
Patch
Comment 14 Geoffrey Garen 2019-10-21 20:09:46 PDT
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
Comment 15 Chris Dumez 2019-10-21 20:29:16 PDT
Created attachment 381503 [details]
Patch
Comment 16 Chris Dumez 2019-10-21 20:29:34 PDT
(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.
Comment 17 WebKit Commit Bot 2019-10-21 21:51:39 PDT
Comment on attachment 381503 [details]
Patch

Clearing flags on attachment: 381503

Committed r251416: <https://trac.webkit.org/changeset/251416>
Comment 18 WebKit Commit Bot 2019-10-21 21:51:40 PDT
All reviewed patches have been landed.  Closing bug.