core(tracehouse): expose navigationStart only as timeOrigin by patrickhulce · Pull Request #11034 · GoogleChrome/lighthouse

patrickhulce · 2020-06-29T17:23:58Z

Summary
First of several refactors to reduce our dependence on navigationStart as a foundation for all lighthouse trace work. This PR renames the exported property from trace-processor.js from navigationStart to timeOrigin but leaves the timeOrigin value as navigationStart. has no effect on behavior (except one bug fix in asset-saver) and is purely naming to enhance the clarity throughout the codebase of where we use navigationStart only as a timeOrigin and when we use it because we really need navigationStart.

Less Lighthouse-centric naming/data model for the tracehouse API
Required for eventual consistency between simulated/observed metrics that was punted to 7.0
Required for eventual handling of SPA navigations

Future Work

Split the determination of timeOrigin out from the main trace processing.
Offer multiple methods of determining the timeOrigin (first resource send, user timing event, etc).
Expose those methods in the rest of Lighthouse core.
Add testing for metrics in both modes for handling complex client-side redirects.
Silently record the metric values using both methods to be placed in the JSON for HTTPArchive evaluation.
Change the default method for all metric definitions to the first resource sendStart if investigation seems promising.

Related Issues/PRs
ref #10325 #9519 #1769 #8984

patrickhulce · 2020-07-15T16:12:36Z

Friendly ping

brendankenny

except one bug fix in asset-saver

what was this fix? Did it have to do with the pwmetrics-events change?

I'm not sure if I see a completely smooth transition for all of trace-processor in the future (going back to no FCP is an issue in a lot of places, for instance) and the loss of the self-documenting name is kind of a downer, but it seems necessary. LGTM

brendankenny · 2020-07-17T16:42:28Z

 * Creates a simple trace that fits the desired options. Useful for basic trace
 * generation, e.g a trace that will result in particular long-task quiet
 * periods. Input times should be in milliseconds.
- * @param {{navigationStart?: number, traceEnd?: number, topLevelTasks?: Array<TopLevelTaskDef>}} options


since this is making a navigationStart event out of this timestamp, seems like it should stay navigationStart? And then generalize if/when we need this to generate traces without a navStart

Hm, not sure I agree here. The primary goal after this change is to have all the places where we say navigationStart do so because conceptually they absolutely require a navigationStart event and not just a timeOrigin timestamp.

All the places where we use createTestTrace today don't need absolutely require a navigationStart event (other than the fact that it's an implementation detail of determining our timeOrigin at this moment), they're just trying to set the timeOrigin. If and when we have tests that explicitly require a navigationStart event because it's the only thing that allows the computation to work, then I agree we should reintroduce navigationStart as a secondary option in this object.

I guess what I'm saying is, everything that sets navigationStart in createTestTrace today would be fine if we replaced the navigationStart event with say FetchStart if it were the timeOriginEvt in TraceProcessor.

Does that make sense?

Does that make sense?

I still disagree, but too late now :P

I guess what I'm saying is, everything that sets navigationStart in createTestTrace today would be fine if we replaced the navigationStart event with say FetchStart if it were the timeOriginEvt in TraceProcessor.

but not a user timing event.

We don't have a definition for TTI for an arbitrary starting point since it requires FCP and DCL. By removing specificity we make every instance of timeOrigin into a hunt into other files for where the trace came from and/or what starting point we were interested in (instead of just saying, oh, yeah, navStart and moving on in the code). Instead I think it's better to stay specific whenever possible (like in test code for tests explicitly written based on navStart) and only get general when we need to be (e.g. in the case of create-test-trace, navigationStart could have been made optional and other possible origins added, with the generated trace adapted depending on which one is used)

brendankenny · 2020-07-17T16:47:09Z

+    if (!timeOriginMetric) return;
+    try {
+      const frameIds = TraceProcessor.findMainFrameIds(this._traceEvents);
+      return {pid: frameIds.pid, tid: frameIds.tid, ts: timeOriginMetric.ts};


what's going on with this change vs what it does today?

before it was kind of abusing navstart to also reuse the main frame ids for other metric markers, this makes the connection more explicit that we really just want the time origin timestamp and main frame pid/tid

brendankenny · 2020-07-17T17:01:31Z

    /** @type {TraceTimesWithoutFCP} */
    const timings = {
-      navigationStart: 0,
+      timeOrigin: 0,


imagining future uses of this with non-navStart-timeOrigins, seems like we're still going to want a navStart entry? Though maybe it should be more specific, like lastNavigationStart or something.

One thing this would help with is answering the question "what's the timeOrigin being used here?". At the very least, you'd be able to compare it against navStart and see that they're equal.

imagining future uses of this with non-navStart-timeOrigins, seems like we're still going to want a navStart entry?

Maybe. But there are certainly cases where there will be 0 navStart entries to return, and I'd rather reintroduce navStart into this chain only where absolutely necessary (similar to above explanation).

Though maybe it should be more specific, like lastNavigationStart or something.

That makes sense to me though I'm not sure anything at all would use it right now. Are you OK holding off on this one until we have the first case where something needs it to better understand the requirements?

Maybe. But there are certainly cases where there will be 0 navStart entries to return, and I'd rather reintroduce navStart into this chain only where absolutely necessary (similar to above explanation).

It doesn't make sense to me to try to be general in solution but not report on navigationStart (if found in the trace), but I guess we'll wait for when it's needed.

One thing this PR makes harder to address is still the "what's the timeOrigin being used here?" question, though.

patrickhulce · 2020-07-17T17:56:52Z

what was this fix? Did it have to do with the pwmetrics-events change?

correct, the pwmetrics-events was erroring on traces from asset-saver until the explicit mainframe id change I made here

brendankenny · 2020-07-20T21:08:37Z

For reference, it's mostly inherent complexity but it suuucks that the answer to "when is performance.now() counting from?" is "it's complicated":

If the script's global object is a Window, the time origin is determined as follows:

If the current Document is the first one loaded in the Window, the time origin is the time at which the browser context was created.

If during the process of unloading the previous document which was loaded in the window, a confirmation dialog was displayed to let the user confirm whether or not to leave the previous page, the time origin is the time at which the user confirmed that navigating to the new page was acceptable.

If neither of the above determines the time origin, then the time origin is the time at which the navigation responsible for creating the window's current Document took place.

If the script's global object is a WorkerGlobalScope (that is, the script is running as a web worker), the time origin is the moment at which the worker was created.

https://developer.mozilla.org/en-US/docs/Web/API/DOMHighResTimeStamp#The_time_origin

and that's without a user also able to pick an arbitrary point in time :) So hopefully we can come up with a good way that it can be figured out at any point in the pipeline.

patrickhulce added 4 commits June 29, 2020 11:34

core(tracehouse): expose navigationStart only as timeOrigin

a523fb0

fix tests

763b19e

fix asset saver test now that bug is gone

011e481

more navStart renames

cb53e0e

patrickhulce requested a review from a team as a code owner June 29, 2020 17:23

patrickhulce requested review from brendankenny and removed request for a team June 29, 2020 17:23

googlebot added the cla: yes label Jun 29, 2020

devtools-bot assigned brendankenny Jun 29, 2020

devtools-bot added the waiting4reviewer label Jun 29, 2020

patrickhulce mentioned this pull request Jun 30, 2020

[DRAFT] Move away from lastNavStart as timeOrigin #10325

Closed

Merge branch 'master' into time_origin_refactor

38e92da

vercel Bot deployed to Preview July 15, 2020 17:12 View deployment

brendankenny approved these changes Jul 17, 2020

View reviewed changes

revert speed index changes

0755733

feedback

ae6cf1f

vercel Bot deployed to Preview July 18, 2020 18:20 View deployment

patrickhulce merged commit 14e5f18 into master Jul 20, 2020

patrickhulce deleted the time_origin_refactor branch July 20, 2020 20:35

patrickhulce mentioned this pull request Aug 7, 2020

trace file for simple action: LHError.errors.NO_NAVSTART aslushnikov/tracium#14

Open

patrickhulce mentioned this pull request Aug 25, 2020

Flow Support (Fraggle Rock) #11313

Closed

patrickhulce mentioned this pull request Sep 29, 2020

RFC: Lighthouse Subpackage Proposal (report UI and tracehouse) #9519

Closed

Conversation

patrickhulce commented Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickhulce commented Jul 15, 2020

Uh oh!

brendankenny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickhulce commented Jul 17, 2020

Uh oh!

brendankenny commented Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

patrickhulce commented Jun 29, 2020 •

edited

Loading

brendankenny commented Jul 20, 2020 •

edited

Loading