Bug 185626 - A service worker process should app nap when all its clients app nap
Summary: A service worker process should app nap when all its clients app nap
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on: 185575
Blocks:
  Show dependency treegraph
 
Reported: 2018-05-14 14:26 PDT by Chris Dumez
Modified: 2019-05-14 18:44 PDT (History)
11 users (show)

See Also:


Attachments
Patch (54.80 KB, patch)
2019-05-06 21:44 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (51.31 KB, patch)
2019-05-07 10:02 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews215 for win-future (13.58 MB, application/zip)
2019-05-07 11:33 PDT, Build Bot
no flags Details
Patch (50.54 KB, patch)
2019-05-07 15:12 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (41.50 KB, patch)
2019-05-10 12:03 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (41.08 KB, patch)
2019-05-10 14:31 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews210 for win-future (13.60 MB, application/zip)
2019-05-10 16:15 PDT, Build Bot
no flags Details
Patch for landing (41.21 KB, patch)
2019-05-14 11:15 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2018-05-14 14:26:52 PDT
A service worker process should app nap when all its clients app nap, for performance / power reasons. Currently, App Nap is disabled for service worker processes.
Comment 1 youenn fablet 2019-05-06 18:46:06 PDT
<rdar://problem/46785908>
Comment 2 youenn fablet 2019-05-06 21:44:59 PDT
Created attachment 369227 [details]
Patch
Comment 3 youenn fablet 2019-05-07 10:02:08 PDT
Created attachment 369298 [details]
Patch
Comment 4 Build Bot 2019-05-07 10:05:09 PDT
Attachment 369298 [details] did not pass style-queue:


ERROR: LayoutTests/platform/ios-wk2/TestExpectations:368:  Path does not exist.  [test/expectations] [5]
ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/ios-wk2/TestExpectations:368:  Path does not exist.  [test/expectations] [5]
Total errors found: 2 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Build Bot 2019-05-07 11:33:49 PDT
Comment on attachment 369298 [details]
Patch

Attachment 369298 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12125261

New failing tests:
http/tests/css/filters-on-iframes.html
Comment 6 Build Bot 2019-05-07 11:33:56 PDT
Created attachment 369306 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 7 youenn fablet 2019-05-07 15:12:04 PDT
Created attachment 369330 [details]
Patch
Comment 8 Build Bot 2019-05-07 15:13:23 PDT
Attachment 369330 [details] did not pass style-queue:


ERROR: LayoutTests/platform/ios-wk2/TestExpectations:368:  Path does not exist.  [test/expectations] [5]
ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/ios-wk2/TestExpectations:368:  Path does not exist.  [test/expectations] [5]
Total errors found: 2 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Chris Dumez 2019-05-08 10:25:57 PDT
Comment on attachment 369330 [details]
Patch

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

This adds a lot of complexity for what it does. Could we try and find a simpler model with less code complexity and less changes of introducing bugs? For example, could we consider not worrying about the client's state and take a user activity for a while every time we ask something from the service worker? (e.g. intercept a load, start the user activity until the load is done, on a hysteresis. postMessage() take a user activity for a while). Basically let the service worker process manage its own process suppression state based on what it is asked to do, rather than worry about the clients. A client could be playing audio and thus not be suitable for app nap and yet not using its service worker process at all. The fact is that a lot of service workers are mostly useful during initial page load.

> Source/WebCore/page/Page.h:721
> +    bool processSuppressionEnabled() const { return m_processSuppressionEnabled; }

Seems like a layer violation for Page to know about process pression, the WebPage already does and it should be enough. I also renamed process suppression to app nap recently for clarity.
Comment 10 youenn fablet 2019-05-08 12:06:05 PDT
(In reply to Chris Dumez from comment #9)
> Comment on attachment 369330 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369330&action=review
> 
> This adds a lot of complexity for what it does. Could we try and find a
> simpler model with less code complexity and less changes of introducing
> bugs?

A part of the patch is very similar to what we will need to do to support client visibility.

A part of the patch could probably be simplified so that we take a per-service-worker process decision instead of storing a per service worker instance decision in the service worker process.

> For example, could we consider not worrying about the client's state
> and take a user activity for a while every time we ask something from the
> service worker? (e.g. intercept a load, start the user activity until the
> load is done, on a hysteresis. postMessage() take a user activity for a
> while). Basically let the service worker process manage its own process
> suppression state based on what it is asked to do, rather than worry about
> the clients.

I am unsure whether that this will be any simpler.

That could mean that any appnapped page that wants to do processing without app nap could do that in its service worker, provided it sends a postMessage every 30 or 40 seconds.

To fix that, we would need to pipe the app nap state of the page in the fetch/postMessage events. We would also need to handle the case of a service worker that postMessage to itself to not be app-napped.

There is also the case of a fetch event that takes more than 30 or 40 seconds to load. 
To handle it, we could keep the user activity until the fetch event is settled but then if the response is big or takes time to load, we would actually need to wait for the whole response to be loaded to not penalize the website.

Another potential issue is service workers doing long lasting processing, say server side events where they do a fetch and each time they receive a new message, they post it to their web page.
We would potentially penalize web pages using service workers to do that processing compared to a page doing that processing itself.

The proposed model (compute service worker state based on its clients) seems simpler to understand and the implications more easy to understand.

> > Source/WebCore/page/Page.h:721
> > +    bool processSuppressionEnabled() const { return m_processSuppressionEnabled; }
> 
> Seems like a layer violation for Page to know about process pression, the
> WebPage already does and it should be enough. I also renamed process
> suppression to app nap recently for clarity.

This is done for having a simple implementation to enable app-nap from Internals.
This can be implemented by adding a testRunner API with IPC from UIProcess to WebProcess to enable/disable app-nap.
Comment 11 Chris Dumez 2019-05-08 15:44:38 PDT
Discussed this with Youenn offline and I think we have a plan to maintain the suggested behavior while simplifying the implementation a bit.
Comment 12 youenn fablet 2019-05-10 12:03:33 PDT
Created attachment 369578 [details]
Patch
Comment 13 youenn fablet 2019-05-10 14:31:09 PDT
Created attachment 369599 [details]
Patch
Comment 14 Build Bot 2019-05-10 16:15:25 PDT
Comment on attachment 369599 [details]
Patch

Attachment 369599 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12157069

New failing tests:
security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star-with-AllowContentSecurityPolicySourceStarToMatchAnyProtocol-enabled.html
Comment 15 Build Bot 2019-05-10 16:15:28 PDT
Created attachment 369613 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 16 youenn fablet 2019-05-13 11:11:46 PDT
ping review
Comment 17 Chris Dumez 2019-05-13 15:17:22 PDT
Comment on attachment 369599 [details]
Patch

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

> Source/WebCore/workers/service/SWClientConnection.h:89
> +    virtual void updateThrottleState() = 0;

I don't think this needs to be public. Can probably be protected.

> Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp:283
> +    if (!m_isThrottleable)

You're registering a new client, how come this cannot get us out of throttleable state?
Comment 18 Chris Dumez 2019-05-13 15:21:03 PDT
Comment on attachment 369599 [details]
Patch

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

> Source/WebKit/WebProcess/WebProcess.h:272
> +    bool arePagesThrottleable() const;

I would have called this: "areAllPagesThrottleable"
Comment 19 youenn fablet 2019-05-13 15:33:01 PDT
Comment on attachment 369599 [details]
Patch

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

>> Source/WebCore/workers/service/SWClientConnection.h:89
>> +    virtual void updateThrottleState() = 0;
> 
> I don't think this needs to be public. Can probably be protected.

This is used by WebPage to tells its connection to update its throttle state when the page state is changing.

>> Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp:283
>> +    if (!m_isThrottleable)
> 
> You're registering a new client, how come this cannot get us out of throttleable state?

I am not sure to understand your comment.
A new client might be created by a page that is app napped (say a worker or an iframe), the WebSWServerConnection throttle state is updated elsewhere.
As of the 'if check', the new client may disable the throttleability of its related context connection only if the new client is not throttleable.
Comment 20 youenn fablet 2019-05-14 11:15:45 PDT
Created attachment 369872 [details]
Patch for landing
Comment 21 WebKit Commit Bot 2019-05-14 12:48:57 PDT
Comment on attachment 369872 [details]
Patch for landing

Clearing flags on attachment: 369872

Committed r245299: <https://trac.webkit.org/changeset/245299>
Comment 22 WebKit Commit Bot 2019-05-14 12:48:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Tim Horton 2019-05-14 18:44:53 PDT
This caused a nasty bug: https://bugs.webkit.org/show_bug.cgi?id=197902