Maintain timing for worker start/ready, for navigation requests. #1575

noamr · 2021-03-28T10:18:14Z

The timing when worker is started or ready are web-accessible via
the navigation timing API (https://w3c.github.io/navigation-timing/).

workerStart is specified to be the time when the service worker is run.

In Chromium and Gecko, worker ready time (exposed as fetchStart)
is set to the right before the time when the Fetch event can be
dispatched for the newly created worker.

In preparation for w3c/navigation-timing#136

Preview | Diff

noamr · 2021-03-29T15:40:17Z

@jakearchibald @wanderview r?

jakearchibald · 2021-04-08T09:28:38Z

Sorry for the delay, I've been on leave. I'll try to get to this this week, or early next week.

wanderview · 2021-04-08T15:02:59Z

Sorry, I missed I was tagged here. I generally defer to the editors to do spec reviews.

jakearchibald

Thanks for the PR, and sorry for the delay reviewing. @mfalken and I took a look.

It isn't clear to me how these values will be read. I'm slightly worried that there's one service worker, therefore one "start time" and one "ready time", but many requests. It feels like "start time" and "ready time" might be overwritten by another request in a race.

Maybe these values should instead be associated with the response, since the timing is from the perspective of the request/response. But I'm just guessing, because I'm not sure how these values will be used.

For instance, if the service worker was started a year ago, and never closed for some reason, should "worker start" be the time a year ago, or should it be the time a given request decided it needed a service worker?

jakearchibald · 2021-04-09T09:02:30Z

docs/index.bs

@@ -2971,6 +2975,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
      1. Let |preloadResponse| be a new [=promise=].
      1. Let |workerRealm| be null.
      1. Let |eventHandled| be null.
+      1. Let |crossOriginIsolatedCapability| be |request|'s [=request/client=]'s [=environment settings object/cross-origin isolated capability=].


Has someone familiar with isolation looked at this? Two timings are taken, once from "in parallel", and another from the service worker thread. Neither of these are the request's client, so does the request's client's cross-origin isolated capability apply here?

It's how things are done in FETCH. Not sure if it applies to service workers. in the same way. @annevk ?

This is interesting as there might be a mismatch between the service worker agent and the requesting agent. I suspect we have to consult both and only if they are both true you would get the high-resolution time. Does that make sense?

That might be a change from what is implemented today as that probably only considers the requesting agent.

(Fetch uses the requesting agent exclusively as it doesn't create any agents of its own.)

docs/index.bs

jakearchibald · 2021-04-09T09:05:42Z

docs/index.bs

@@ -3020,6 +3025,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
          1. If |shouldSoftUpdate| is true, then [=in parallel=] run the [=Soft Update=] algorithm with |registration|.
          1. Return null.
      1. If |activeWorker|'s <a>state</a> is "`activating`", wait for |activeWorker|'s <a>state</a> to become "`activated`".
+      1. Let |workerStartTime| be the  [=coarsened shared current time=] given <var>crossOriginIsolatedCapability</var>.


The previous step (If |activeWorker|'s <a>state</a> is "activating") probably should be included in this measurement, since we've decided we need a service worker, but it might not be ready. If the site is seeing long delays due to activation delays, that feels important.

docs/index.bs

wanderview · 2021-04-09T12:50:40Z

For instance, if the service worker was started a year ago, and never closed for some reason, should "worker start" be the time a year ago, or should it be the time a given request decided it needed a service worker?

Yea, I thought it would be the time the request decided it needed a service worker if the worker was already running.

noamr · 2021-04-09T13:07:09Z

Thanks for the PR, and sorry for the delay reviewing. @mfalken and I took a look.

It isn't clear to me how these values will be read. I'm slightly worried that there's one service worker, therefore one "start time" and one "ready time", but many requests. It feels like "start time" and "ready time" might be overwritten by another request in a race.

OK, I will change it to mean the "first" start/ready time. These values are only meant for Navigation Timing, I'll prepare a Navigation Timing PR that reads them to make it clearer. (they would have inter-dependencies so this one would have to go in first).

Maybe these values should instead be associated with the response, since the timing is from the perspective of the request/response. But I'm just guessing, because I'm not sure how these values will be used.

For instance, if the service worker was started a year ago, and never closed for some reason, should "worker start" be the time a year ago, or should it be the time a given request decided it needed a service worker?

The service worker's start time would be a year ago, but Navigation Timing would expose the later time between that time and the time FETCH has handed control over to the service worker.

noamr · 2021-04-11T09:47:37Z

I've refactored the PR, I agree that it makes much more sense to associate the info with the response rather than with the worker.
w3c/navigation-timing#143 is the navigation timing PR that shows how this info is going to be used. To complete the picture, we would need to modify two additional specs:

FETCH would have to send SW-FETCH the crossOriginIsolatedCapability as the second argument.
HTML would have to send the response's service worker timing when creating the navigation timing entry.

There are probably more ways to accomplish the same results, I went with the way that made the most sense to me but I'm open to refactor given an alternative approach.

annevk · 2021-04-13T12:06:18Z

docs/index.bs

+
+      Service workers mark certain points in times, that are later exposed by the {{PerformanceNavigationTiming|navigation timing}} API.
+
+      A [=/response=] has an associated null or [=service worker timing info=] <dfn export for=response>service worker timing</dfn>, initially set to null.


I would prefer if we defined this in Fetch and SW just does the marking. Mainly to keep response defined in a single place.

Is it correct that this information gets lost if a network error is returned instead at some point?

I would prefer if we defined this in Fetch and SW just does the marking. Mainly to keep response defined in a single place.

OK. I was trying to make it so that FETCH doesn't have to be modified every time we add a new service-worker metric. Should the struct live in FETCH as well?

Is it correct that this information gets lost if a network error is returned instead at some point?

This is only exposed through Navigation Timing. So, if there's a network error, the document won't be loaded and these values would be meaningless. In other words, correct.

I think it's okay to keep the struct here to make that easier.

@jakearchibald I want to make sure we're OK with how this whole process works, and then I'll publish:

A SW PR that defines the new struct

A FETCH PR that associates the struct with a response and adds arguments to the SW FETCH call

The marking part of the SW PR

jakearchibald

Minor changes, but generally LGTM

docs/index.bs

jakearchibald · 2021-04-15T12:34:24Z

docs/index.bs

@@ -2958,6 +2974,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

      : Input
      :: |request|, a [=/request=]
+      :: |crossOriginIsolatedCapability|, a boolean


It isn't clear to me which client this isolation refers to. Maybe it should just be useHighResPerformanceTimers or something which makes the scope of its usage clear?

jakearchibald · 2021-04-15T12:37:56Z

docs/index.bs

@@ -193,15 +193,31 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
        * Detects abnormal operation: such as infinite loops and tasks exceeding imposed time limits (if any) while handling the events.
    </section>

-    <section>
+   <section>


Unintentional change?

docs/index.bs

to report worker-specific timing information to Navigation Timing. In preparation for w3c#1575

to report worker-specific timing information to Navigation Timing. In preparation for #1575

jakearchibald · 2021-04-27T14:02:01Z

Give me a nudge when this is ready to go

noamr · 2021-04-27T15:32:49Z

Give me a nudge when this is ready to go

Thanks, will do! first I need to wait for the refs to apply and post a FETCH PR.

In preparation for w3c/ServiceWorker#1575 and w3c/navigation-timing#143 This will allow exposing the service worker timing as part of the Navigation Timing API.

Helps with w3c/ServiceWorker#1575 and w3c/navigation-timing#143.

The timing when worker is started or ready are web-accessible via the navigation timing API (https://w3c.github.io/navigation-timing/). `workerStart` is specified to be the time when the service worker is run. In Chromium and Gecko, `worker ready time` (exposed as `fetchStart`) is set to the right before the time when the Fetch event can be dispatched for the newly created worker. In preparation for w3c/navigation-timing#136

Co-authored-by: Jake Archibald <jaffathecake@gmail.com>

Temporarily remove response association, as that moves to FETCH.

noamr · 2021-05-11T06:24:09Z

Give me a nudge when this is ready to go

Nudge @jakearchibald :)

Service Workers now expose a "service worker timing info" that is associated with a request. HTML should pass it to Navigation Timing together with the other info. See also w3c/navigation-timing#143 and w3c/ServiceWorker#1575

annevk · 2021-05-11T11:41:58Z

What about #1575 (comment)?

noamr · 2021-05-12T06:23:10Z

What about #1575 (comment)?

Fixed in new revision.

docs/index.bs

jakearchibald · 2021-05-13T11:22:26Z

Thank you @noamr!

Service Workers now expose a "service worker timing info" that is associated with a request. HTML should pass it to Navigation Timing together with the other info. See also w3c/navigation-timing#143 and w3c/ServiceWorker#1575

Amend the parameters passed when creating the navigation timing info entry to include the service worker timing info. Also, pass 0 for redirectCount When cross-origin redirects occurred as part of the navigation. See also w3c/navigation-timing#143 and w3c/ServiceWorker#1575.

noamr mentioned this pull request Mar 29, 2021

4.6 Integrate the processing model with Fetch w3c/resource-timing#252

Closed

16 tasks

jakearchibald self-assigned this Apr 8, 2021

noamr mentioned this pull request Apr 8, 2021

4.1 Processing model should rely on Fetch/HTML w3c/navigation-timing#136

Closed

26 tasks

jakearchibald reviewed Apr 9, 2021

View reviewed changes

noamr mentioned this pull request Apr 11, 2021

Integrate with service worker timing w3c/navigation-timing#143

Merged

annevk reviewed Apr 13, 2021

View reviewed changes

annevk mentioned this pull request Apr 13, 2021

When response comes from a worker, some of the timing-info should pass through whatwg/fetch#1208

Closed

jakearchibald requested changes Apr 15, 2021

View reviewed changes

jakearchibald reviewed Apr 15, 2021

View reviewed changes

docs/index.bs Outdated Show resolved Hide resolved

noamr added a commit to noamr/ServiceWorker that referenced this pull request Apr 15, 2021

Expose a "service worker timing info" struct

2f50de4

to report worker-specific timing information to Navigation Timing. In preparation for w3c#1575

noamr mentioned this pull request Apr 15, 2021

Expose a "service worker timing info" struct #1581

Merged

jakearchibald pushed a commit that referenced this pull request Apr 27, 2021

Expose a "service worker timing info" struct (#1581)

b807be1

to report worker-specific timing information to Navigation Timing. In preparation for #1575

noamr force-pushed the nav-time-integ branch from 702858d to fd4c075 Compare April 27, 2021 13:31

noamr mentioned this pull request May 2, 2021

Associate service worker timing with a response whatwg/fetch#1228

Merged

annevk pushed a commit to whatwg/fetch that referenced this pull request May 10, 2021

Associate service worker timing info with a response

208e271

Helps with w3c/ServiceWorker#1575 and w3c/navigation-timing#143.

noamr and others added 6 commits May 10, 2021 15:15

Update docs/index.bs

62677b1

Co-authored-by: Jake Archibald <jaffathecake@gmail.com>

Update docs/index.bs

b5a7c05

Co-authored-by: Jake Archibald <jaffathecake@gmail.com>

Refactor to associate with response

f6debfe

Rename var

8ad6cdc

Rename variable

e480f64

Temporarily remove response association, as that moves to FETCH.

noamr added 2 commits May 10, 2021 15:15

Add TODO

f389b8c

Use association from FETCH

e556803

noamr force-pushed the nav-time-integ branch from fd4c075 to e556803 Compare May 11, 2021 06:23

noamr mentioned this pull request May 11, 2021

Update marking of navigation timing to include Service Worker Timing whatwg/html#6670

Merged

Apply worker's cross-origin isolation

29a7ef7

jakearchibald approved these changes May 13, 2021

View reviewed changes

docs/index.bs Outdated Show resolved Hide resolved

Update docs/index.bs

21510d9

jakearchibald merged commit 4b5d041 into w3c:main May 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Maintain timing for worker start/ready, for navigation requests. #1575

Maintain timing for worker start/ready, for navigation requests. #1575

noamr commented Mar 28, 2021 •

edited by pr-preview bot

Loading

noamr commented Mar 29, 2021

jakearchibald commented Apr 8, 2021

wanderview commented Apr 8, 2021

jakearchibald left a comment

jakearchibald Apr 9, 2021

noamr Apr 9, 2021

annevk Apr 13, 2021

jakearchibald Apr 9, 2021

wanderview commented Apr 9, 2021

noamr commented Apr 9, 2021

noamr commented Apr 11, 2021

annevk Apr 13, 2021

noamr Apr 13, 2021 •

edited

Loading

annevk Apr 13, 2021

noamr Apr 13, 2021

jakearchibald left a comment

jakearchibald Apr 15, 2021

jakearchibald Apr 15, 2021

jakearchibald commented Apr 27, 2021

noamr commented Apr 27, 2021

noamr commented May 11, 2021

annevk commented May 11, 2021

noamr commented May 12, 2021

jakearchibald commented May 13, 2021


		Service workers mark certain points in times, that are later exposed by the {{PerformanceNavigationTiming\|navigation timing}} API.

		A [=/response=] has an associated null or [=service worker timing info=] <dfn export for=response>service worker timing</dfn>, initially set to null.

Maintain timing for worker start/ready, for navigation requests. #1575

Maintain timing for worker start/ready, for navigation requests. #1575

Conversation

noamr commented Mar 28, 2021 • edited by pr-preview bot Loading

noamr commented Mar 29, 2021

jakearchibald commented Apr 8, 2021

wanderview commented Apr 8, 2021

jakearchibald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wanderview commented Apr 9, 2021

noamr commented Apr 9, 2021

noamr commented Apr 11, 2021

Choose a reason for hiding this comment

noamr Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakearchibald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakearchibald commented Apr 27, 2021

noamr commented Apr 27, 2021

noamr commented May 11, 2021

annevk commented May 11, 2021

noamr commented May 12, 2021

jakearchibald commented May 13, 2021

noamr commented Mar 28, 2021 •

edited by pr-preview bot

Loading

noamr Apr 13, 2021 •

edited

Loading