core: add largest contentful paint to lantern and default config #9905
Conversation
Co-Authored-By: Paul Irish <paulirish@google.com>
Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>
Co-Authored-By: Brendan Kenny <bckenny@gmail.com>
|
@patrickhulce finished a lantern collection on GCP. |
lighthouse-core/test/computed/metrics/largest-contentful-paint-test.js
Outdated
Show resolved
Hide resolved
| @@ -53,6 +54,8 @@ class PredictivePerf extends Audit { | |||
| const ttfcpui = await LanternFirstCPUIdle.request({trace, devtoolsLog, settings}, context); | |||
| const si = await LanternSpeedIndex.request({trace, devtoolsLog, settings}, context); | |||
| const eil = await LanternEil.request({trace, devtoolsLog, settings}, context); | |||
| // TODO: we don't have LCP in the lantern test yet | |||
There was a problem hiding this comment.
move comment to L17?
also can you file an issue that tracks the trace-updating/calibration bit and drop the issue # here?
| /** | ||
| * @fileoverview Computed Largest Contentful Paint (LCP), the paint time of the largest in-viewport contentful element | ||
| * COMPAT: LCP's trace event was first introduced in m78. We can't surface an LCP for older Chrome versions | ||
| * @see https://github.com/WICG/largest-contentful-paint | ||
| * @see https://wicg.github.io/largest-contentful-paint/ | ||
| * @see https://web.dev/largest-contentful-paint | ||
| */ | ||
|
|
||
| const makeComputedArtifact = require('../computed-artifact.js'); |
There was a problem hiding this comment.
we're inconsistent about this already. it looks 50/50 in a brief survey.
being inconsistent would suggest that we leave existing ones as they lie until we fix all of them at once. (i think that's what we've done before in similar situations... require('xx.js').. right?)
that said i dont think its important this time. so lets ignore this one.. just this one time. :)
| "firstCPUIdle": 10075, | ||
| "firstCPUIdleTs": undefined, | ||
| "firstContentfulPaint": 8828, | ||
| "firstContentfulPaintTs": undefined, |
There was a problem hiding this comment.
FCP, FMP, SI, and TTI flipped to undefined. i don't think that's expected is it?
There was a problem hiding this comment.
we went from devtools to simulate so i think these are expected to be undefined
| @@ -42,7 +42,7 @@ describe('Performance: metrics', () => { | |||
| }, | |||
| }; | |||
|
|
|||
| const context = {settings: {throttlingMethod: 'devtools'}, computedCache: new Map()}; | |||
| const context = {settings: {throttlingMethod: 'simulate'}, computedCache: new Map()}; | |||
There was a problem hiding this comment.
i don't know the context here but this stands out.... is this deliberate?
There was a problem hiding this comment.
it was devtools before because simulating wasn't an option for LCP :)
the other tests here use simulate
| Object { | ||
| "optimistic": 17637, | ||
| "pessimistic": 17637, | ||
| "timing": 17637, |
There was a problem hiding this comment.
😮 just noting that that 17s is a pretty huge LCP
There was a problem hiding this comment.
in the trace this is at 1750ish ms
obviously this lantern lcp isn't perfect but i wouldnt expect that we'd have a 10x diff between observed and simulated.
would you mind looking into why it's so high?
types/artifacts.d.ts
Outdated
| @@ -527,6 +527,8 @@ declare global { | |||
| firstContentfulPaintEvt: TraceEvent; | |||
| /** The trace event marking firstMeaningfulPaint, if it was found. */ | |||
| firstMeaningfulPaintEvt?: TraceEvent; | |||
| /** The trace event marking largestMeaningfulPaint, if it was found. */ | |||
| largestMeaningfulPaintEvt?: TraceEvent; | |||
| "firstCPUIdle": 10075, | ||
| "firstCPUIdleTs": undefined, | ||
| "firstContentfulPaint": 8828, | ||
| "firstContentfulPaintTs": undefined, |
| @@ -42,7 +42,7 @@ describe('Performance: metrics', () => { | |||
| }, | |||
| }; | |||
|
|
|||
| const context = {settings: {throttlingMethod: 'devtools'}, computedCache: new Map()}; | |||
| const context = {settings: {throttlingMethod: 'simulate'}, computedCache: new Map()}; | |||
| Object { | ||
| "optimistic": 17637, | ||
| "pessimistic": 17637, | ||
| "timing": 17637, |
There was a problem hiding this comment.
in the trace this is at 1750ish ms
obviously this lantern lcp isn't perfect but i wouldnt expect that we'd have a 10x diff between observed and simulated.
would you mind looking into why it's so high?
| }); | ||
|
|
||
| it('should compute an observed value', async () => { | ||
| const settings = {throttlingMethod: 'provided'}; | ||
| const context = {settings, computedCache: new Map()}; | ||
| const result = await LargestContentfulPaint.request({trace, devtoolsLog, settings}, context); | ||
|
|
||
| assert.equal(Math.round(result.timing), 15024); | ||
| assert.equal(result.timestamp, 1671236939268); | ||
| assert.equal(Math.round(result.timing), 1744); |
There was a problem hiding this comment.
and here's the observed numbers right?
this would only change with the trace changing.. i do see the trace changed but viewing the diff definitely isn't straightforward.
you had a trace before where the observed LCP was 15s ?
There was a problem hiding this comment.
yeah observed.
I guess I had a really bad trace before
(10 URLS, compared against G4s)
