-
Notifications
You must be signed in to change notification settings - Fork 30
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
workerStart and redirects #131
base: gh-pages
Are you sure you want to change the base?
Conversation
I also need to review how |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks for working on this!
index.html
Outdated
@@ -312,8 +312,11 @@ <h3> | |||
time immediately before the user agent <a data-cite= | |||
"service-workers#fetchevent">fired an event named `fetch`</a> at | |||
the <a data-cite="service-workers#dfn-active-worker">active | |||
worker</a>. Otherwise, if there is no active worker this attribute | |||
MUST return zero. | |||
worker</a>. If there were redirects, this is the start time of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if this is the behavior of user agents right now? In particular, does this require new tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing current WPT tests, I think we'll need to add some to confirm this all
…rom different origins
…en XO and SO), and add workerStart in
Today, there can be delays between fetchStart and workerStart in implementations. Does workerStart need to be clearly defined as possibly after fetchStart in the diagram and spec or is the intent to change that? |
Oh hi Todd :) I think the diagram is supposed to imply that, but it should probably be made clearer by moving |
@nicjansma did you accidentally remove the PR preview link from the first comment? I was hoping to look at the new diagram since I just realized I commented on the old one. |
My point is that I think we DO want a space in between fetchStart and workerStart but the diagram doesn't show that. When I see a delay between fetchStart and workerStart, I've historically recommended that the site should consider not blocking load on the serviceworker if the design can allow for it so having the gap is valuable. I haven't considered what to name that bucket of time but the goal is to allow all time to be measured and broken down so a web page author can either communicate with the browser vendor OR improve their own site to improve E2E timing. |
@npm1 Yeah, I had to remove the Preview link as it was stopping me from making edits to the comment for some reason. I've added it back in. (I didn't realize what those links did initially, that's very useful!) @toddreifsteck I think Agreed that we can help make that more clear. Right now I've put Alternatively, I could add a new "phase" called "Service Worker Startup Time" (or just "Worker" or whatever), to make it even more clear. |
Discussions from the W3C call on 9/24:
For example:
In this case,
Instead, if we kept the current model where I think it's important to try to measure "Worker Startup Time" for sites that have Service Workers deployed, and this time can often be seen to be a measurable amount (>50ms in some cases). If so, should we add a new timestamp at the end of SW startup? Maybe Then Thoughts? |
Yea, this makes sense, I think this is what they're calling |
Ahh great, thanks for pointing me to that one. I'll add |
@nicjansma I think workerStart can't be before fetchStart on the very first navigation to a web page, can it? I'm also unsure if it is before or after fetchStart when navigation preload is used. https://w3c.github.io/ServiceWorker/#service-worker-registration-navigationpreload For a page with a service worker installed, it does seem that serviceWorkerStart could be the first event but I always believed the intent of fetchStart was to mark when the UA determined the fetch algorithm should be processed on a URL and to clearly show when unload is complete and the next phase is starting. If discussions have shown that isn't how the metrics are implemented or are used, please accurately specify them and don't block on my gut belief. 👍 :) |
Apologies for missing this. I see w3c/resource-timing#119 has been linked to which explains some of the issues around here. As that issue notes, I'm wondering about the decision to use the first request of the last same-origin redirect chain for I wonder if it's more consistent to just always use the final request for |
For the scenario where a visitor has never been to a site before, and thus the browser does not have an active Service Worker registration? In that case,
Or are you talking about another scenario?
Great question, nothing in this spec deals with Navigation Preload. Do you want to file a separate issue to track that?
Yeah I think before we acknowledged the existence of Service Workers in this spec (and in RT), |
Thanks for sharing that. If I understand it correctly:
The intent was to be able to capture the "worker bootup" or "worker startup" time for a domain, i.e. before a domain handles its first request. We were hoping the first request in the chain got us that. If we were to (re)set
That's a good question, we don't discuss scope at all. My assumption is the SW is not "boot"ed if the scope doesn't match in that first request, right? So in that scenario the SW would "boot" for the second+ request, and that bootup time would only be reflected in the Do you think we should clarify the processing model to be something like
But I think in that case the cost of the SW bootup time will always be "0"ish any time there's a redirect. And regardless, if we really want to be able to measure SW bootup time if there are redirects, we need a Above all else I think we all want to try to make the NT and RT definition and processing model align everywhere that's possible. So if we want these (and/or more) changes in NT we should also have agreement they belong in RT as well. |
I think the new definition of workerStart will allow sites to measure the overhead of Service Worker startup on the main page as the gap between fetchStart-workerStart and they can measure the total time as responseEnd-workerStart if a SW is involved so this seems to solve that problem at a high level. I don't know how many sites use Navigation Preload but the spec should handle it cleanly. Please open an issue if you believe it is worth tracking. I'm not active in spec work in my new role. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this makes sense to me modulo some comments! It would be nice to have someone from SW working group review this to see if it makes sense to them as well, modulo the hand-waviness which is pending on fetch integration.
"service-workers#dfn-containing-service-worker-registration">active | ||
service worker registration</a> [[SERVICE-WORKERS]], this attribute MUST | ||
return zero.</li> | ||
<li>If there were redirects, this attribute MUST return a {{DOMHighResTimeStamp}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think these if statements are a bit hard to follow. In this if statement, we only see If there were redirect
but the condition is more than that: there were redirects and we had to run a worker? Or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other words it seems this should be split into two, similar to the next two ifs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, to simplify it maybe something like:
If there were redirects, this attribute MUST return a {{DOMHighResTimeStamp}}
with the `workerStart` time recorded by the first request in final same-origin redirect chain.
Or is that too much of a circular dependency?
Sorry for the delay, I took some days off.
I may be missing something, but the two specs seem to have
This makes sense. I think the "in scope of a service worker" would be a worthwhile clarification... or rather it should something like "the first request that is in the same scope of the FINAL in-scope request that is same-origin to the final request in the redirect chain". Suppose there are two scopes:
Agreed that |
Ah, and I'm not as familiar with the Fetch spec steps, so I had originally read this differently (that when I think part of the discrepancy is the description of
And I agree per your reasoning if However the processing model goes in a different "timestamp order":
From this processing model, in order, it seems Stepping back, I think part of the confusion is So maybe what I'm arguing here is that In practice today, Chrome seems to consistently set
👍
Awesome, let's work towards that! |
Tried to summarize where we're at for the WebPerf WG https://docs.google.com/presentation/d/1r3FwT1UTo7lpjZvYe-YV7cNAee8co-qCxIU5SdERalQ/edit |
Following the work I was doing on RT/Fetch integration, I want to make a concrete proposal for discussion here about how to handle redirects (beyond the diagram). First of all, I think this should be in ResourceTiming and not in NavigationTiming, as workerStart is relevant for NT only because NT is an augmentation of RT (RT is more connected with fetching, NT more with document life-cycle). The problem with The problem is that in the case of redirects, any of these metrics could have several values, and due to a mixture of caching/workers/http, the "last" one might be ambiguous - for example, the last I propose doing the following:
|
We had a further discussion on this as well on March 18th 2021 in the WebPerfWG call, with ServiceWorker folks: https://w3c.github.io/web-performance/meetings/2021/2021-03-18/index.html I will address that and @noamr's feedback in this PR soon, and probably will need to just wait until #141 goes in for simplicity. |
Thanks for putting your suggestions together! Overall I agree on the simplification.
I think this would cause some "reduced insight" into resources vs. today, as you would lose details of DNS/TCP/req/res phases for all resources if a SW is active, right? If the worker was just operating as a "pass through" for a resource, it seems like we should still get those breakdown in timings (assuming origin check passes). |
Yes, though we should make it more clearer in FETCH. I created a new issue for that: whatwg/fetch#1208 |
Where are we on this PR? What's the next step? |
Based on the conversations we had at WG, I think this PR covers the issue.
|
As long as fetch callers pass in the necessary data through the request concept, they will not have to make additional calls to get timing reported accurately. Note that this does not work if callers want to use useParallelQueue. Downstream PRs: * whatwg/html#7722 * whatwg/xhr#347 * w3c/csswg-drafts#7355 * w3c/beacon#75 * w3c/resource-timing#321 * https://github.com/w3c/navigation-timing/pull/1760 Closes #1208 and closes w3c/navigation-timing#131.
I think this can be closed now. @nicjansma ? |
@nicjansma - friendly ping :) |
This addresses a few issues around
workerStart
especially in the case of redirects:workerStart
needed to be added to the diagram worker start needs to be added to diagram #128 (see screenshot below)workerStart
timestamp before the start of the Worker Startup phaseworkerStart
definition was cleaned up. Notably, it's:0
for no SW (same as before)fetch
(same as before)worker-start-step
that is split out from the step that was settingunloadEventEnd
workerStart
was zero-ed out in the case of same-origin redirects -- it should be the startup of the worker from the first request in the final same-origin redirect chain worker start needs to be added to diagram #128 (comment)workerStart
, and will still jump back tofetch-start-step
so it doesn't overwrite the worker startup time for the same originworkerStart
was the value of the cross-origin fetch during cross-origin redirects -- it should be0
(or updated by later same-origin SWs) worker start needs to be added to diagram #128 (comment)workerStart
was set for, and will resetworkerStart
if the origin ever changes."fail"
, so added a new step to check for no redirects and return"pass"
.workerStart
to the list of things that NavTiming2 has over NavTiming1Current diagram:
I will also be reviewing the current WPTs to lock-in this behavior, assuming we all agree to the above changes.
Preview | Diff