core: add largest contentful paint to lantern and default config #9905

connorjclark · 2019-10-31T01:41:23Z

(10 URLS, compared against G4s)

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>

connorjclark · 2019-11-09T21:54:40Z

@patrickhulce finished a lantern collection on GCP.

lighthouse-core/test/computed/metrics/largest-contentful-paint-test.js

paulirish · 2019-11-09T23:28:15Z

lighthouse-core/audits/predictive-perf.js

@@ -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


move comment to L17?
also can you file an issue that tracks the trace-updating/calibration bit and drop the issue # here?

paulirish · 2019-11-09T23:31:26Z

lighthouse-core/computed/metrics/largest-contentful-paint.js

 /**
 * @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');


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. :)

paulirish · 2019-11-09T23:33:10Z

lighthouse-core/test/audits/__snapshots__/metrics-test.js.snap

+  "firstCPUIdle": 10075,
+  "firstCPUIdleTs": undefined,
+  "firstContentfulPaint": 8828,
+  "firstContentfulPaintTs": undefined,


FCP, FMP, SI, and TTI flipped to undefined. i don't think that's expected is it?

we went from devtools to simulate so i think these are expected to be undefined

paulirish · 2019-11-09T23:34:40Z

lighthouse-core/test/audits/metrics-test.js

@@ -42,7 +42,7 @@ describe('Performance: metrics', () => {
      },
    };

-    const context = {settings: {throttlingMethod: 'devtools'}, computedCache: new Map()};
+    const context = {settings: {throttlingMethod: 'simulate'}, computedCache: new Map()};


i don't know the context here but this stands out.... is this deliberate?

it was devtools before because simulating wasn't an option for LCP :)

the other tests here use simulate

paulirish · 2019-11-09T23:35:45Z

lighthouse-core/test/computed/metrics/lantern-largest-contentful-paint-test.js

+Object {
+  "optimistic": 17637,
+  "pessimistic": 17637,
+  "timing": 17637,


😮 just noting that that 17s is a pretty huge LCP

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?

paulirish · 2019-11-09T23:38:16Z

types/artifacts.d.ts

@@ -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;


say whaaaaa?

paulirish · 2019-11-10T00:21:59Z

lighthouse-core/test/audits/__snapshots__/metrics-test.js.snap

+  "firstCPUIdle": 10075,
+  "firstCPUIdleTs": undefined,
+  "firstContentfulPaint": 8828,
+  "firstContentfulPaintTs": undefined,


paulirish · 2019-11-10T00:22:09Z

lighthouse-core/test/audits/metrics-test.js

@@ -42,7 +42,7 @@ describe('Performance: metrics', () => {
      },
    };

-    const context = {settings: {throttlingMethod: 'devtools'}, computedCache: new Map()};
+    const context = {settings: {throttlingMethod: 'simulate'}, computedCache: new Map()};


paulirish · 2019-11-10T00:25:32Z

lighthouse-core/test/computed/metrics/lantern-largest-contentful-paint-test.js

+Object {
+  "optimistic": 17637,
+  "pessimistic": 17637,
+  "timing": 17637,


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?

paulirish · 2019-11-10T00:28:03Z

lighthouse-core/test/computed/metrics/largest-contentful-paint-test.js

  });

  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);


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 ?

yeah observed.

I guess I had a really bad trace before

connorjclark and others added 30 commits September 19, 2019 18:20

first pass

ec75de3

update

183b080

i18n

03f8ab6

typos

52a8d28

metrics

4e2e688

ts

f1836a0

fix

449c7c5

Apply suggestions from code review

d821da7

Co-Authored-By: Paul Irish <paulirish@google.com>

todo

9cc6b8c

fix lcp

211a90b

update sample trace

ee9f34e

tweaks

24db2ed

fix metrics-test

308f0fe

rename all those pwa trace requires

33fecc9

just update all the metric snapshots

2b87da0

undo tests

c473bf5

m79 in metrics test

26bb02a

m60

b848a73

lcp metric trace test

967d630

Merge remote-tracking branch 'origin/master' into lcp

5b80688

pr

9da66f9

Update lighthouse-core/lib/tracehouse/trace-processor.js

49aa30a

Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>

Merge branch 'lcp' of github.com:GoogleChrome/lighthouse into lcp

a71ab04

Apply suggestions from code review

7fd52f4

Co-Authored-By: Brendan Kenny <bckenny@gmail.com>

update traces

f351590

minify trace

ff42528

first pass

e93e396

wip

9103431

fix print

a3fade6

improve

567efc2

lint. line length

fa7f089

vercel bot deployed to staging November 9, 2019 07:43 View deployment

paulirish added the land-when-ci-is-green label Nov 9, 2019

fix lantern test

42b7ad1

vercel bot deployed to staging November 9, 2019 08:18 View deployment

connorjclark added 2 commits November 9, 2019 14:27

Merge remote-tracking branch 'origin/master' into lcp-lantern

8b5cce0

fix tests

ce0042b

vercel bot deployed to staging November 9, 2019 22:33 View deployment

fix trace processor test

bbc5030

vercel bot temporarily deployed to staging November 9, 2019 22:40 Inactive

rm common prefix

9812701

vercel bot deployed to staging November 9, 2019 22:41 View deployment

fix cli test

e0a04b7

vercel bot deployed to staging November 9, 2019 22:45 View deployment

connorjclark commented Nov 9, 2019

View reviewed changes

lighthouse-core/test/computed/metrics/largest-contentful-paint-test.js Outdated Show resolved Hide resolved

delete old test

2d34e65

vercel bot deployed to staging November 9, 2019 23:28 View deployment

paulirish requested changes Nov 9, 2019

View reviewed changes

connorjclark added 2 commits November 9, 2019 15:43

pr

6a1fdb7

comment

1579ccb

vercel bot deployed to staging November 9, 2019 23:51 View deployment

connorjclark mentioned this pull request Nov 10, 2019

Update lantern test traces and re-calibrate lantern #9953

Closed

paulirish reviewed Nov 10, 2019

View reviewed changes

connorjclark added 2 commits November 9, 2019 16:54

Merge remote-tracking branch 'origin/master' into lcp-lantern

5fc5db0

use a better trace for lcp

98f3ea2

vercel bot deployed to staging November 10, 2019 03:50 View deployment

paulirish approved these changes Nov 10, 2019

View reviewed changes

connorjclark merged commit 7c5d86d into master Nov 10, 2019

connorjclark deleted the lcp-lantern branch November 10, 2019 04:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: add largest contentful paint to lantern and default config #9905

core: add largest contentful paint to lantern and default config #9905

connorjclark commented Oct 31, 2019 •

edited

Loading

connorjclark commented Nov 9, 2019

paulirish Nov 9, 2019

connorjclark Nov 9, 2019

paulirish Nov 9, 2019

paulirish Nov 9, 2019

connorjclark Nov 9, 2019

paulirish Nov 10, 2019

paulirish Nov 9, 2019

connorjclark Nov 9, 2019

paulirish Nov 10, 2019

paulirish Nov 9, 2019

paulirish Nov 10, 2019

paulirish Nov 9, 2019

connorjclark Nov 9, 2019

paulirish Nov 10, 2019

paulirish Nov 10, 2019

paulirish Nov 10, 2019

paulirish Nov 10, 2019

connorjclark Nov 10, 2019

core: add largest contentful paint to lantern and default config #9905

core: add largest contentful paint to lantern and default config #9905

Conversation

connorjclark commented Oct 31, 2019 • edited Loading

connorjclark commented Nov 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Oct 31, 2019 •

edited

Loading