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

performance-impls: LCP should use startTime #35070

Merged
merged 4 commits into from
Jun 30, 2021
Merged

Conversation

samouri
Copy link
Member

@samouri samouri commented Jun 29, 2021

summary
There isn't much utility in separately tracking LCP Load and LCP Render.

  • loadTime corresponds to when the resource associated with a node has been loaded from network. It is always available on every node, but is relatively meaningless for any non-remote resource on a page. For example, all <h1> tags have a loadTime, but that time is 0. Therefore if a text node were chosen as the LCP Element, we'd report 0 for LCP-L.
  • renderTime corresponds to when the node is painted to the screen. This has a meaningful value for text nodes, but ends up being absent for any crossorigin resource without a Timing-Allow-Origin header. In these cases it is recommended to fallback to loadTime since the numbers will be relatively close to each other. Since AMP Caches usually host all their images, this is also not a very strong factor for AMP.

cc @ampproject/wg-performance

@erwinmombay
Copy link
Member

Fixes #34383

@erwinmombay erwinmombay self-requested a review June 29, 2021 16:51
Copy link
Contributor

@rcebulko rcebulko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just commenting to flag the .only so it isn't forgotten

test/unit/test-performance.js Outdated Show resolved Hide resolved
Co-authored-by: Ryan Cebulko <ryan@cebulko.com>
@samouri samouri marked this pull request as ready for review June 29, 2021 18:36
@amp-owners-bot
Copy link

Hey @jridgewell! These files were changed:

src/core/constants/enums.js

@samouri samouri requested a review from jridgewell June 29, 2021 18:37
Copy link
Contributor

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (entry.renderTime) {
this.largestContentfulPaintRenderTime_ = entry.renderTime;
}
this.largestContentfulPaint_ = entry.startTime;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do all browsers support startTime?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is also how the web-vitals package implements it.

Copy link
Member Author

@samouri samouri Jun 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jridgewell: I've updated TICKEVENTS.md in 79da311

@samouri samouri requested a review from jridgewell June 29, 2021 20:47
@samouri samouri enabled auto-merge (squash) June 30, 2021 15:29
@samouri samouri merged commit 8662b2b into ampproject:main Jun 30, 2021
@samouri samouri deleted the lcp-fix branch June 30, 2021 18:55
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