[DRAFT] Move away from lastNavStart as timeOrigin#10325
Closed
patrickhulce wants to merge 1 commit into
Closed
Conversation
Collaborator
Author
|
Aight @paulirish lots of decisions to be made here 😬 LMK what you think |
Member
|
Patrick proposing:
|
This was referenced Jun 25, 2020
Collaborator
Author
|
closing this PR as #11034 picks up the torch here |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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?
ResourceSendRequestts (this PR)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:blankrandomly 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
timeOrigincomponent 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
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...
timeOriginproperty on trace-of-tabnavStartto usetimeOrigininstead.timeOrigin.max(fp, speedIndex)finalUrlto use finallocation.hrefof the window.redirectsaudit to display the time the URL took to redirect as wastedMs rather than the time the previous URL took to redirectredirectsaudit to surface client-side redirectsRelated Issues/PRs
#8984