New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Optimized No-Fetch Service Workers #718

Closed
fatlotus opened this Issue Jul 2, 2015 · 29 comments

Comments

Projects
None yet
10 participants
@fatlotus

fatlotus commented Jul 2, 2015

Hey all-

In Blink, main resource fetch with a cold Service Worker causes sadness (median Android/470 and Linux/210 ms), even if the worker doesn't intercept requests. To reduce this a bit, I've been looking into a) detecting Service Workers without fetch event handlers, and b) immediately falling back to the network. Unfortunately, as things stand today, fetch event handlers can be registered at any point, making (a) rather difficult.

A discussion in #195 about install and activate ended up with a devtools warning, but it seems (naively) like that has less of a performance impact. Does this warrant more harsh restrictions in fetch? We're mainly worried about i) non-deterministic registration for a given SW version, and ii) registration after the end of install.

(I've written a basic Design Document for this feature, if you're curious.)

@annevk

This comment has been minimized.

Member

annevk commented Jul 2, 2015

I think the answer Mozilla has been given whenever we brought this up was that you should not have service workers without fetch. :-/

@jakearchibald

This comment has been minimized.

Collaborator

jakearchibald commented Jul 2, 2015

@fatlotus are we seeing many SWs without a fetch event? I know there's a few at the moment to trigger Chrome's app install banner, but we're going to start tightening the heuristics here, we only want to promote add-to-homescreen for sites that have some offline capability.

Listeners should be deterministically added to the SW global during initial execution. Both the Math.random case and adding listeners later are antipatterns.

+@slightlyoff

@annevk

This comment has been minimized.

Member

annevk commented Jul 2, 2015

Observing listeners is also an established anti-pattern however, as we mentioned, and a much older one at that.

@mattto

This comment has been minimized.

Member

mattto commented Jul 2, 2015

We're seeing some sites that use SW just for Push Notifications. Pinterest is one.

@annevk

This comment has been minimized.

Member

annevk commented Jul 2, 2015

Right, that's exactly the kind of feedback Mozilla gave for why we should not treat fetch as a builtin.

@jakearchibald

This comment has been minimized.

Collaborator

jakearchibald commented Jul 2, 2015

Right, and Pinterest will get the poor user experience of "user clicks notification, gets generic browser offline page".

That said, the workarounds in the doc seem pretty sensible.

@wanderview

This comment has been minimized.

Member

wanderview commented Jul 2, 2015

I think many of these sites care a lot about push on the desktop where they couldn't reasonably get it in the past. Offline for mobile is more of a "nice to have" since they already have native apps there.

@kinu

This comment has been minimized.

Contributor

kinu commented Jul 3, 2015

From implementation & optimization pov it'd be definitely good if we can clearly say listeners should be added to the SW global during initialization somewhere in the spec. On the other hand if we decide not to spec it out (e.g. for not promoting such usage) there seems to be still a way to optimize the implementation without affecting the Web-facing behavior, though the code would become less clean.

@jungkees

This comment has been minimized.

Collaborator

jungkees commented Jul 6, 2015

Currently we settled on #225 (comment) but people didn't want to override addEventListener to record the registered event types.

I'd like to try to add the list of event types to handle on the service worker's environment settings object created in step 5 of Run Service Worker algorithm. And set its value after step 14 like the following:

14. Jump to the script's code entry-point, and let that run until it either returns...
15. Set settingsObject's _list of event types to handle_ to the set of event types retrieved from the _associated list of event listeners_ of settingsObject's global object.
16. Run the responsible event loop specified by settingsObject until it is destroyed.

As step 15 runs before the event loop starts running, the list of event types to handle will retain only the event types that have been added during the first script eval.

And in step 14 of Handle Fetch algorithm, we can do:
If registration's active worker's environment settings object's list of event types to handle doesn't contain fetch, return null (i.e, fall back to network).

@jungkees

This comment has been minimized.

Collaborator

jungkees commented Jul 6, 2015

Ah I think we'll have to check the list of event types to handle even before a service worker starts running, so the property itself should be an internal slot of a service worker instead of the environment settings object. Anyway, the idea is just get the set of event types from the initial script eval and use that to check whether we should not actually run a worker but rather just fall back to network.

@jungkees

This comment has been minimized.

Collaborator

jungkees commented Jul 7, 2015

Added the set of event types to handle internal slot and made the Handle Fetch use it to enforce an early return: f8d94f5. Also added a note about showing a warning for non-deterministically added listeners.

@wanderview

This comment has been minimized.

Member

wanderview commented Jul 7, 2015

@mattto

This comment has been minimized.

Member

mattto commented Jul 22, 2015

Consensus at the Service Worker informal working session F2F was to not observe listeners, and instead add a method like disableFetch() to be called in the install event handler.

Reasoning:

  • observing listeners would break the invariant that addEventListener has no side effects, potentially making SW spec's event model diverge from DOM spec's.
  • having an explicit listener list is an alternative; however fetch event is the only one you need it for as the others are already opt-in.
  • an explicit listener enableFetch() would break current sites
  • therefore, disableFetch() was chosen
@kinu

This comment has been minimized.

Contributor

kinu commented Jul 23, 2015

Let me make sure. If the impl could make the optimization without changing the addEventListener behavior (though supporting all corner cases is hard) it should be fine / up to UA, right?

I kinda agree that we probably wouldn't want to spec out something that affects generic addEventListener behavior.

@annevk

This comment has been minimized.

Member

annevk commented Jul 23, 2015

@kinu sure, non-observable optimizations are fine.

@wanderview

This comment has been minimized.

Member

wanderview commented Jul 23, 2015

I think we felt spec'ing the optimization was necessary previously because it is observable. If script does setTimeout(addEventListener.bind(self, handlerFunc)) then the optimization prevents the late addEventListener call from working. This is definitely observable.

Anyway, the explicit API avoids this.

@kinu

This comment has been minimized.

Contributor

kinu commented Jul 24, 2015

@wanderview Yeah there're several tricky cases that make non-observable optimization less (if not) practical. For the particular setTimeout case you wrote, though, it looks technically the impl could just flip the optimization flag when it's called (or UA may just kill the SW before the timer fires regardless of the optimization)? I agree that there could be various more tricky cases.

Anyway, the explicit API avoids this.

Yup.

@aykevl

This comment has been minimized.

aykevl commented Jul 25, 2015

With this proposal, it's impossible to use ServiceWorker for other tasks and just forget about caching (while keeping good performance). You have to do one of these two things:

  • Properly implement the fetch event (with caching, full offline capabilities, or whatever).
  • Call disableFetch() so the browser knows it can go straight to the network without firing a fetch event first.

Personally, I dislike the opt-out nature of this proposal. It's too easy to forget if you don't want to use offline capabilities, and adds more boilerplate code. It's also counter-intuitive to me.

Many other capabilities have been mentioned for service workers, that are not dependent on offline capabilities, like push notifications, background sync, and geolocations. We don't know yet where service workers will actually be used for in 5 years.

Additional reasons why I think opt-out is a bad idea:

  • It isn't currently visible that there is a performance problem when forgetting disableFetch() in applications that don't care about offline capabilities. Maybe browsers will eventually have to implement some form of fetch event detection for badly written service workers. Though with disableFetch() I think it's a good idea to throw a warning in the developer console if no fetch handlers are registered when disableFetch() has not been called by the end of the install event.
  • Some applications are inherently dependent on the network. A feature like background sync can't work without network access.

Note that, again, service workers are very new so everyone using them now should expect things can break with every new browser release. Consider ServiceWorkerMessageEvent which suddenly moves from window to navigator.serviceWorker (offtopic: good move!). Now displaying warnings about how the fetch event is going to change in a few versions and requiring to opt-in to the events doesn't seem like a big problem to me. The longer you wait with such changes, the harder it is to change.

(This is my opinion on the matter, as someone who doesn't (yet) work professionally as web developer.)

@kinu

This comment has been minimized.

Contributor

kinu commented Jul 27, 2015

@aykevl I don't think that is true, developers can appropriately specify the scope attribute in the Registration Options when they register a SW to say what URLs the site wants their SW to take care of for fetch events. That's one of the most basic fundamental of how SW is designed to work.

Also a feature like background sync is a way to give more flexible options for a site to choose when it talks to the server, it can't work without network access but it'd be nearly useless if its SW doesn't handle fetch events when network access is not available.

@kinu

This comment has been minimized.

Contributor

kinu commented Jul 27, 2015

(Well I just learned that there's also discussion on the concept of scope.)

@wanderview

This comment has been minimized.

Member

wanderview commented Oct 5, 2015

Will the navigator.serviceWorker.controlled attribute still reference the service worker matching the page's scope even when disableFetch() has been called?

@jakearchibald

This comment has been minimized.

Collaborator

jakearchibald commented Oct 5, 2015

From IRC:

My hunch would be that the pages are still controlled. It allows you to defer idb upgrades to an activate event of a sw, avoiding multiple tabs with different versions. Also makes sense with clients.matchAll.

@slightlyoff

This comment has been minimized.

Contributor

slightlyoff commented Oct 28, 2015

Pages are still controlled by scope if you disableFetch. I think we've covered all the bases here. Just need to spec disableFetch()

@slightlyoff slightlyoff added this to the Version 1 milestone Oct 28, 2015

@mattto

This comment has been minimized.

Member

mattto commented Jan 22, 2016

Implementor feedback from Chrome: We don't really plan to implement disableFetch(). It's an irreversible change if devs start using it, and I think we're not yet convinced it's needed. We're OK taking the perf hit for now and remain hopeful that a non-observable optimization is possible.

@jakearchibald

This comment has been minimized.

Collaborator

jakearchibald commented Jan 26, 2016

F2F resolution: We're going to observe addEventListener and optimise based on that.

addEventListener should throw if it's called after the initial execution of the script.

@mattto

This comment has been minimized.

Member

mattto commented Jan 28, 2016

Cool! Just to make sure, initial execution of the script refers to the very first time the SW runs (before it's installed) or every time the SW is run?

The current spec seems to observe event types every time in Run Service Worker, which wouldn't allow for this optimization. We want to persist a bit on the installed script indicating whether there's a fetch event handler. So next time you go to the website, we can decide whether the SW needs to run.

@mkruisselbrink

This comment has been minimized.

Collaborator

mkruisselbrink commented Jan 28, 2016

For the optimization initial execution indeed means to record which event listeners have been registered for at the end of the very first time the SW runs.

Furthermore every time the SW runs, after its initial execution has completed (for that run) addEventListener on the global scope should throw (but the list of event listeners is not updated after the first initial execution). And this is not just about fetch, but would apply to every event in ServiceWorkerGlobalScope.

@jungkees

This comment has been minimized.

Collaborator

jungkees commented Jan 29, 2016

We're going to observe addEventListener and optimise based on that.

Relevant changes for this and enforcing the early return to all the functional events have been addressed: b80ba58

addEventListener should throw if it's called after the initial execution of the script.

I'll make a PR to DOM spec for this.

@jungkees

This comment has been minimized.

Collaborator

jungkees commented Feb 12, 2016

Closed by b80ba58 and whatwg/dom#155

@jungkees jungkees closed this Feb 12, 2016

makotoshimazu added a commit to makotoshimazu/Service-Worker-Performance that referenced this issue Apr 25, 2016

Add a test to measure the overhead of bypassing requests to SW
As discussion[1] shows, handling a request via SW having no fetch
handler causes degradation of performance comparing to bypassing SW.
This new test is created to track this issue (related to an issue[2]).

[1]: w3c/ServiceWorker#718
[2]: http://crbug.com/605844
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment