Skip to content
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

[DRAFT] Move away from lastNavStart as timeOrigin #10325

Closed
wants to merge 1 commit into from

Conversation

patrickhulce
Copy link
Collaborator

@patrickhulce patrickhulce commented Feb 12, 2020

Summary

This turned out to be an absolute nightmare to leave so close to release, and I honestly might recommend punting this to 7.0 and explicitly dedicating some time to better handle client-side redirect across all of our audits before tackling this.

We have several options here and going down the rabbit hole of my preferred approach cascades a lot of other required changes, so before I go update the world...

Questions

What Time Origin To Use?

  1. First document ResourceSendRequest ts (this PR)
  2. First navigationStart
  3. Last navigationStart (Give up on this, and move lantern to ignore all nodes before last nav start instead)

Going with ResourceSendRequest aligns us the closest to what I feel the user cares about, the lantern simulation model, and should fix the cases where Chrome unloading about:blank randomly takes a long time (navigationStart is spec-defined as fetchStart + time taken to unload previous document), but this also involves lots of other changes through the codebase and affects runs even without redirects.

Going with first navigation start would be more a gradual change, allow us to even ignore the timeOrigin component of this PR, and keep our existing "navstart" references (depending on the answer to the next question).

Going with last navigation start would make things consistent by changing lantern to just ignore everything before last navstart.

How to Handle Cases Where the Redirecting Page Rendered

  1. Ignore all progress and events before last navStart (This PR)
  2. Consider prior page only for SI
  3. Consider prior page for FCP and SI
  4. Consider prior page for FCP, SI, TBT, and MPFID

Client-side redirects can render some content (and execute JS) before ultimately redirecting. Should that count for FCP? Do the long tasks that occur during that page load count toward TBT? Do we then reblack out longtasks for TBT once the next navigation starts? What about speed index?

This is the killer question primarily because it's not handled very explicitly in any of our audits at the moment, some metrics get it for free by looking after the last nav start but we don't filter main thread events for just those after navStart right now so user timings for example can contain negative timestamps, mainthread work breakdown will count those tasks, etc

I can see arguments for all sides but IMO, it's most straightforward and consistent if we just consider everything before the finalUrl page load as wasted time.

This PR...

  • Introduces a new timeOrigin property on trace-of-tab
  • Changes all of our references to navStart to use timeOrigin instead.
  • Uses the start time of the first document request as timeOrigin.
  • Preserves last nav start as the selector for all metrics.
  • Updates Speed Index to do max(fp, speedIndex)
  • Updates finalUrl to use final location.href of the window.
  • Adds client-side redirect smoke tests.
  • Converts the redirect some tests to use observed metrics.
  • Changes the redirects audit to display the time the URL took to redirect as wastedMs rather than the time the previous URL took to redirect
  • [TODO] Update redirects audit to surface client-side redirects

Related Issues/PRs
#8984

@patrickhulce
Copy link
Collaborator Author

Aight @paulirish lots of decisions to be made here 😬 LMK what you think

@paulirish
Copy link
Member

paulirish commented Feb 13, 2020

Patrick proposing:

  • we do the JS redirect warning in 6.0
  • Land trace-processor changes (for non-lantern runs) after 6.0's release. Make default for 7.0 after testing and evaluating in HTTPArchive.

@patrickhulce
Copy link
Collaborator Author

closing this PR as #11034 picks up the torch here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants