Bug 185626 – A service worker process should app nap when all its clients app nap

• Component: Service Workers
• Status: RESOLVED FIXED

[Chris Dumez]

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.

[youenn fablet]

<rdar://problem/46785908>

[youenn fablet]

Created attachment 369227 [details]
Patch

[youenn fablet]

Created attachment 369298 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[youenn fablet]

Created attachment 369330 [details]
Patch

[EWS Watchlist]

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.

[Chris Dumez]

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.

[youenn fablet]

(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.

[Chris Dumez]

Discussed this with Youenn offline and I think we have a plan to maintain the suggested behavior while simplifying the implementation a bit.

[youenn fablet]

Created attachment 369578 [details]
Patch

[youenn fablet]

Created attachment 369599 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[youenn fablet]

ping review

[Chris Dumez]

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?

[Chris Dumez]

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"

[youenn fablet]

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.

[youenn fablet]

Created attachment 369872 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 369872 [details]
Patch for landing

Clearing flags on attachment: 369872

Committed r245299: <https://trac.webkit.org/changeset/245299>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tim Horton]

This caused a nasty bug: https://bugs.webkit.org/show_bug.cgi?id=197902