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.

Sign up for GitHub

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

Jump to bottom

core(byte-efficiency): compute FCP & LCP savings #15104

Merged
merged 21 commits into from
Jul 5, 2023
Merged

core(byte-efficiency): compute FCP & LCP savings #15104

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
086cd00
WIP: compute FCP & LCP savings for byte efficiency audits
adamraine May 22, 2023
049f130
ope
adamraine May 22, 2023
c23c193
graphs
adamraine May 23, 2023
e452b93
console
adamraine May 23, 2023
f73e001
Merge branch 'main' into byte-efficiency-savings
adamraine Jun 8, 2023
f1fdca3
reorg
adamraine Jun 8, 2023
369df9b
include lcp record savings
adamraine Jun 8, 2023
f489a86
comment
adamraine Jun 8, 2023
32d8dd3
Merge branch 'main' into byte-efficiency-savings
adamraine Jun 9, 2023
caa183f
comment
adamraine Jun 9, 2023
7556484
unit test fix
adamraine Jun 9, 2023
92213ac
Merge branch 'main' into byte-efficiency-savings
adamraine Jun 9, 2023
7be2067
unit tests
adamraine Jun 9, 2023
e058cf1
smokes
adamraine Jun 12, 2023
cc55816
no smoke expectations
adamraine Jun 12, 2023
f894073
only review LCP download time
adamraine Jun 15, 2023
5cdc409
Merge branch 'main' into byte-efficiency-savings
adamraine Jun 15, 2023
08e1ba7
sample
adamraine Jun 15, 2023
c2bd4fe
precision
adamraine Jun 16, 2023
f97bf07
test
adamraine Jun 16, 2023
2551675
Merge branch 'main' into byte-efficiency-savings
adamraine Jul 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions core/audits/audit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,18 @@ class Audit {
details: product.details,
};
}

/**
* @param {LH.Artifacts} artifacts
* @param {LH.Audit.Context} context
* @returns {LH.Artifacts.MetricComputationDataInput}
*/
static makeMetricComputationDataInput(artifacts, context) {
const trace = artifacts.traces[Audit.DEFAULT_PASS];
const devtoolsLog = artifacts.devtoolsLogs[Audit.DEFAULT_PASS];
const gatherContext = artifacts.GatherContext;
return {trace, devtoolsLog, gatherContext, settings: context.settings, URL: artifacts.URL};
}
}

export {Audit};
125 changes: 100 additions & 25 deletions core/audits/byte-efficiency/byte-efficiency-audit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ import * as i18n from '../../lib/i18n/i18n.js';
import {NetworkRecords} from '../../computed/network-records.js';
import {LoadSimulator} from '../../computed/load-simulator.js';
import {PageDependencyGraph} from '../../computed/page-dependency-graph.js';
import {LanternLargestContentfulPaint} from '../../computed/metrics/lantern-largest-contentful-paint.js';
import {LanternFirstContentfulPaint} from '../../computed/metrics/lantern-first-contentful-paint.js';
import {LCPImageRecord} from '../../computed/lcp-image-record.js';
import {LCPBreakdown} from '../../computed/metrics/lcp-breakdown.js';

const str_ = i18n.createIcuMessageFn(import.meta.url, {});

Expand Down Expand Up @@ -104,9 +108,7 @@ class ByteEfficiencyAudit extends Audit {
*/
static async audit(artifacts, context) {
const gatherContext = artifacts.GatherContext;
const trace = artifacts.traces[Audit.DEFAULT_PASS];
const devtoolsLog = artifacts.devtoolsLogs[Audit.DEFAULT_PASS];
const URL = artifacts.URL;
const settings = context?.settings || {};
const simulatorOptions = {
devtoolsLog,
Expand All @@ -125,34 +127,29 @@ class ByteEfficiencyAudit extends Audit {
};
}

const [result, graph, simulator] = await Promise.all([
const metricComputationInput = Audit.makeMetricComputationDataInput(artifacts, context);

const [result, simulator] = await Promise.all([
this.audit_(artifacts, networkRecords, context),
// Page dependency graph is only used in navigation mode.
gatherContext.gatherMode === 'navigation' ?
PageDependencyGraph.request({trace, devtoolsLog, URL}, context) :
null,
LoadSimulator.request(simulatorOptions, context),
]);

return this.createAuditProduct(result, graph, simulator, gatherContext);
return this.createAuditProduct(result, simulator, metricComputationInput, context);
}

/**
* Computes the estimated effect of all the byte savings on the maximum of the following:
*
* - end time of the last long task in the provided graph
* - (if includeLoad is true or not provided) end time of the last node in the graph
* Computes the estimated effect of all the byte savings on the provided graph.
*
* @param {Array<LH.Audit.ByteEfficiencyItem>} results The array of byte savings results per resource
* @param {Node} graph
* @param {Simulator} simulator
* @param {{includeLoad?: boolean, label?: string, providedWastedBytesByUrl?: Map<string, number>}=} options
* @return {number}
* @param {{label?: string, providedWastedBytesByUrl?: Map<string, number>}=} options
* @return {{savings: number, simulationBeforeChanges: LH.Gatherer.Simulation.Result, simulationAfterChanges: LH.Gatherer.Simulation.Result}}
*/
static computeWasteWithTTIGraph(results, graph, simulator, options) {
options = Object.assign({includeLoad: true, label: this.meta.id}, options);
const beforeLabel = `${options.label}-before`;
const afterLabel = `${options.label}-after`;
static computeWasteWithGraph(results, graph, simulator, options) {
options = Object.assign({label: ''}, options);
const beforeLabel = `${this.meta.id}-${options.label}-before`;
const afterLabel = `${this.meta.id}-${options.label}-after`;

const simulationBeforeChanges = simulator.simulate(graph, {label: beforeLabel});

Expand Down Expand Up @@ -187,7 +184,36 @@ class ByteEfficiencyAudit extends Audit {
node.record.transferSize = originalTransferSize;
});

const savingsOnOverallLoad = simulationBeforeChanges.timeInMs - simulationAfterChanges.timeInMs;
const savings = simulationBeforeChanges.timeInMs - simulationAfterChanges.timeInMs;

return {
// Round waste to nearest 10ms
savings: Math.round(Math.max(savings, 0) / 10) * 10,
simulationBeforeChanges,
simulationAfterChanges,
};
}

/**
* Computes the estimated effect of all the byte savings on the maximum of the following:
*
* - end time of the last long task in the provided graph
* - (if includeLoad is true or not provided) end time of the last node in the graph
*
* @param {Array<LH.Audit.ByteEfficiencyItem>} results The array of byte savings results per resource
* @param {Node} graph
* @param {Simulator} simulator
* @param {{includeLoad?: boolean, providedWastedBytesByUrl?: Map<string, number>}=} options
* @return {number}
*/
static computeWasteWithTTIGraph(results, graph, simulator, options) {
options = Object.assign({includeLoad: true}, options);
const {savings: savingsOnOverallLoad, simulationBeforeChanges, simulationAfterChanges} =
this.computeWasteWithGraph(results, graph, simulator, {
...options,
label: 'overallLoad',
});

const savingsOnTTI =
LanternInteractive.getLastLongTaskEndTime(simulationBeforeChanges.nodeTimings) -
LanternInteractive.getLastLongTaskEndTime(simulationAfterChanges.nodeTimings);
Expand All @@ -201,24 +227,65 @@ class ByteEfficiencyAudit extends Audit {

/**
* @param {ByteEfficiencyProduct} result
* @param {Node|null} graph
* @param {Simulator} simulator
* @param {LH.Artifacts['GatherContext']} gatherContext
* @return {LH.Audit.Product}
* @param {LH.Artifacts.MetricComputationDataInput} metricComputationInput
* @param {LH.Audit.Context} context
* @return {Promise<LH.Audit.Product>}
*/
static createAuditProduct(result, graph, simulator, gatherContext) {
static async createAuditProduct(result, simulator, metricComputationInput, context) {
const results = result.items.sort((itemA, itemB) => itemB.wastedBytes - itemA.wastedBytes);

const wastedBytes = results.reduce((sum, item) => sum + item.wastedBytes, 0);

/** @type {LH.Audit.MetricSavings} */
const metricSavings = {
FCP: 0,
LCP: 0,
};

// `wastedMs` may be negative, if making the opportunity change could be detrimental.
// This is useful information in the LHR and should be preserved.
let wastedMs;
if (gatherContext.gatherMode === 'navigation') {
if (!graph) throw Error('Page dependency graph should always be computed in navigation mode');
if (metricComputationInput.gatherContext.gatherMode === 'navigation') {
const graph = await PageDependencyGraph.request(metricComputationInput, context);
const {
pessimisticGraph: pessimisticFCPGraph,
} = await LanternFirstContentfulPaint.request(metricComputationInput, context);
const {
pessimisticGraph: pessimisticLCPGraph,
} = await LanternLargestContentfulPaint.request(metricComputationInput, context);

wastedMs = this.computeWasteWithTTIGraph(results, graph, simulator, {
providedWastedBytesByUrl: result.wastedBytesByUrl,
});

const {savings: fcpSavings} = this.computeWasteWithGraph(
results,
pessimisticFCPGraph,
simulator,
{providedWastedBytesByUrl: result.wastedBytesByUrl, label: 'fcp'}
);
const {savings: lcpGraphSavings} = this.computeWasteWithGraph(
results,
pessimisticLCPGraph,
simulator,
{providedWastedBytesByUrl: result.wastedBytesByUrl, label: 'lcp'}
);

// The LCP graph can underestimate the LCP savings if there is potential savings on the LCP record itself.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm having trouble understanding this. How is this the case? If the LCP node download time is reduced, why isn't the simulation taking that into account when returning LCP savings?

Copy link
Member Author

Choose a reason for hiding this comment

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

Graph timings are computed using the end time of the entire graph, the LCP request node is not necessarily the final node in the graph. Presumably, it's possible for the node that marks the end of the entire graph to have it's timing unaffected by changes to the LCP request node.

Copy link
Collaborator

@connorjclark connorjclark Jun 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can you provide a breakdown for a specific example where this is the case? I'd expect there to be some dependency between the final node (is it the CPU node that has the layout?) and the network request node whose bytes are being reduced.

Copy link
Member Author

Choose a reason for hiding this comment

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

The case in the PR comment is an example of this happening. LCP image is listed modern-image-formats but the LCP graph savings are 0. I did not save any artifacts other than the trace, so I'll try and get another example of such a case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is an example I captured with DT throttling. modern-image-formats shows the LCP record savings as larger than the LCP graph savings:

artifacts.json.zip
traceAndDtLog.zip
(had to split it up because it was too big for GH)

let lcpRecordSavings = 0;
const lcpRecord = await LCPImageRecord.request(metricComputationInput, context);
const lcpBreakdown = await LCPBreakdown.request(metricComputationInput, context);
if (lcpRecord && lcpBreakdown.loadStart && lcpBreakdown.loadEnd) {
const lcpResult = results.find(result => result.url === lcpRecord.url);
if (lcpResult) {
const lcpLoadTime = lcpBreakdown.loadEnd - lcpBreakdown.loadStart;
Copy link
Member

Choose a reason for hiding this comment

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

the benefit is only to download time.. but this loadTime duration includes ttfb time

tbh looking at your original results I'd say that the image audits were not underestimating LCP savings

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean "includes ttfb time"? lcpLoadTime is supposed to represent the "Resource load time" phase of LCP so it's just the download time.

Copy link
Member Author

Choose a reason for hiding this comment

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

tbh looking at your original results I'd say that the image audits were not underestimating LCP savings

In the first set of results, modern-image-formats reports 0 savings on LCP when calculated using the LCP graph even though the LCP image is listed in that audit. This is definitely an underestimate IMO.

lcpRecordSavings = Math.round(lcpLoadTime * lcpResult.wastedBytes / lcpResult.totalBytes);
}
}

metricSavings.FCP = fcpSavings;
metricSavings.LCP = Math.max(lcpGraphSavings, lcpRecordSavings);
adamraine marked this conversation as resolved.
Show resolved Hide resolved
} else {
wastedMs = simulator.computeWastedMsFromWastedBytes(wastedBytes);
}
Expand All @@ -232,6 +299,13 @@ class ByteEfficiencyAudit extends Audit {
const details = Audit.makeOpportunityDetails(result.headings, results,
{overallSavingsMs: wastedMs, overallSavingsBytes: wastedBytes, sortedBy});

// TODO: Remove from debug data once `metricSavings` is added to the LHR.
// For now, add it to debug data for visibility.
details.debugData = {
type: 'debugdata',
metricSavings,
};

return {
explanation: result.explanation,
warnings: result.warnings,
Expand All @@ -240,6 +314,7 @@ class ByteEfficiencyAudit extends Audit {
numericUnit: 'millisecond',
score: ByteEfficiencyAudit.scoreForWastedMs(wastedMs),
details,
metricSavings,
};
}

Expand Down
Loading