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.
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
core(lantern): improve LCP tail error #10529
core(lantern): improve LCP tail error #10529
Changes from 9 commits
a57f352
e76c0a8
d79e310
4bca67b
2551c56
ac5f1f5
9f58163
65fe3c8
48e2db7
27885d4
269a9b5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
wait, this seems different than just a tie breaker. It can actually change the order of nodes that don't have the same start time?
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.
"tiebreaker" if it were only the exact same nanosecond timestamp is useless. The lower the priority the wider the berth that's given to consider it a "tie" and defer to the higher priority request.
Requests started within the same second that all have their dependencies finished do not really need to be started in the exact same observed order that Chrome happened to request them.
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.
sure, but usually "tie breaker" is for exactly that sort of thing (e.g. sorting comparators). This is more of a time spreading heuristic :)
You didn't post much motivation for the exact numbers used in the PR, and it's not clear to me that reordering like that would work out correctly, but I guess the proof is the comparison errors :)
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.
bingo :) haha
and that's fair, so here's my caveat to anyone who comes back trying to find where they came from
Dear reader from the future, I do not have supreme confidence that the exact numbers used in the PR are optimal nor were they divined after some extensive research process. They "felt right" and improved accuracy so if you need to change them for similar reasons, feel free :)
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.
lmao i am future reader, thx for context
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.
also that
|| 0
frightens me :) When does that come into play?startTime
andpriority
should always be defined?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.
it shouldn't (it ran successfully on our entire lantern corpus and passed all our tests without this change) but in the event that we get something unplanned for priority the failure mode without it is no longer sorting the nodes at all, failing the entire performance category also seems like a pretty bad experience so 0 fallback makes the most sense.