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

core(lantern): FCP improvements #7513

Merged
merged 10 commits into from
Apr 9, 2019
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
162 changes: 132 additions & 30 deletions lighthouse-core/computed/metrics/lantern-first-contentful-paint.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ const LanternMetric = require('./lantern-metric.js');
const BaseNode = require('../../lib/dependency-graph/base-node.js');

/** @typedef {BaseNode.Node} Node */
/** @typedef {import('../../lib/dependency-graph/cpu-node')} CPUNode */
/** @typedef {import('../../lib/dependency-graph/network-node')} NetworkNode */

class LanternFirstContentfulPaint extends LanternMetric {
/**
Expand All @@ -24,52 +26,152 @@ class LanternFirstContentfulPaint extends LanternMetric {
}

/**
* @param {Node} dependencyGraph
* @param {LH.Artifacts.TraceOfTab} traceOfTab
* @return {Node}
* This function computes the set of URLs that *appeared* to be render-blocking based on our filter,
* *but definitely were not* render-blocking based on the timing of their EvaluateScript task.
* It also computes the set of corresponding CPU node ids that were needed for the paint at the
* given timestamp.
*
* @param {Node} graph
* @param {number} filterTimestamp The timestamp used to filter out tasks that occured after our
* paint of interest. Typically this is First Contentful Paint or First Meaningful Paint.
* @param {function(NetworkNode):boolean} blockingScriptFilter The function that determines which scripts
* should be considered *possibly* render-blocking.
* @param {(function(CPUNode):boolean)=} extraBlockingCpuNodesToIncludeFilter The function that determines which CPU nodes
* should also be included in our blocking node IDs set.
* @return {{definitelyNotRenderBlockingScriptUrls: Set<string>, blockingCpuNodeIds: Set<string>}}
*/
static getOptimisticGraph(dependencyGraph, traceOfTab) {
const fcp = traceOfTab.timestamps.firstContentfulPaint;
const blockingScriptUrls = LanternMetric.getScriptUrls(dependencyGraph, node => {
return (
node.endTime <= fcp && node.hasRenderBlockingPriority() && node.initiatorType !== 'script'
);
static getBlockingCpuData(
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
graph,
filterTimestamp,
blockingScriptFilter,
extraBlockingCpuNodesToIncludeFilter
) {
/** @type {Array<CPUNode>} */
const cpuNodes = [];
graph.traverse(node => {
if (node.type === BaseNode.TYPES.CPU && node.startTime <= filterTimestamp) {
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
cpuNodes.push(node);
}
});

return dependencyGraph.cloneWithRelationships(node => {
if (node.endTime > fcp && !node.isMainDocument()) return false;
// Include EvaluateScript tasks for blocking scripts
if (node.type === BaseNode.TYPES.CPU) {
return node.isEvaluateScriptFor(blockingScriptUrls);
}
cpuNodes.sort((a, b) => a.startTime - b.startTime);

// Include non-script-initiated network requests with a render-blocking priority
return node.hasRenderBlockingPriority() && node.initiatorType !== 'script';
// A script is *possibly* render blocking if it finished loading before FCP
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
const possiblyRenderBlockingScriptUrls = LanternMetric.getScriptUrls(graph, node => {
return node.endTime <= filterTimestamp && blockingScriptFilter(node);
});

// A script is *definitely not* render blocking if its EvaluateScript task finished after FCP.
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
/** @type {Set<string>} */
const definitelyNotRenderBlockingScriptUrls = new Set();
/** @type {Set<string>} */
const blockingCpuNodeIds = new Set();
for (const url of possiblyRenderBlockingScriptUrls) {
let hadEvaluateScript = false;

for (const cpuNode of cpuNodes) {
if (cpuNode.isEvaluateScriptFor(new Set([url]))) {
hadEvaluateScript = true;
blockingCpuNodeIds.add(cpuNode.id);
break;
}
}

// We couldn't find the evaluate script in the set of CPU nodes that ran before our paint, so
// it must not have been necessary for the paint.
if (!hadEvaluateScript) definitelyNotRenderBlockingScriptUrls.add(url);
}

// The first layout, first paint, and first ParseHTML are almost always necessary for first paint,
// so we always include those CPU nodes.
const firstLayout = cpuNodes.find(node => node.didPerformLayout());
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
if (firstLayout) blockingCpuNodeIds.add(firstLayout.id);
const firstPaint = cpuNodes.find(node => node.childEvents.some(e => e.name === 'Paint'));
if (firstPaint) blockingCpuNodeIds.add(firstPaint.id);
const firstParse = cpuNodes.find(node => node.childEvents.some(e => e.name === 'ParseHTML'));
if (firstParse) blockingCpuNodeIds.add(firstParse.id);
Copy link
Member

Choose a reason for hiding this comment

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

are the FCP/FMP results weird (and not worth keeping) if any of these are missing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

theoretically yes, but in practice I've seen some of these be shifted slightly so I wouldn't want to throw it all away

maybe another candidate for #7041 ?

Copy link
Member

Choose a reason for hiding this comment

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

maybe another candidate for #7041 ?

yeah, probably the best thing before we start throwing on it


// If a CPU filter was passed in, we also want to include those extra nodes.
if (extraBlockingCpuNodesToIncludeFilter) {
cpuNodes
.filter(extraBlockingCpuNodesToIncludeFilter)
.forEach(node => blockingCpuNodeIds.add(node.id));
}

return {
definitelyNotRenderBlockingScriptUrls,
blockingCpuNodeIds,
};
}

/**
* This function computes the graph required for the first paint of interest.
*
* @param {Node} dependencyGraph
* @param {LH.Artifacts.TraceOfTab} traceOfTab
* @param {number} paintTs
* @param {function(NetworkNode):boolean} blockingScriptFilter
* @param {function(CPUNode):boolean=} extraBlockingCpuNodesToIncludeFilter
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
* @return {Node}
*/
static getPessimisticGraph(dependencyGraph, traceOfTab) {
const fcp = traceOfTab.timestamps.firstContentfulPaint;
const blockingScriptUrls = LanternMetric.getScriptUrls(dependencyGraph, node => {
return node.endTime <= fcp && node.hasRenderBlockingPriority();
});
static getFirstPaintBasedGraph(
dependencyGraph,
paintTs,
blockingScriptFilter,
extraBlockingCpuNodesToIncludeFilter
) {
const {
definitelyNotRenderBlockingScriptUrls,
blockingCpuNodeIds,
} = this.getBlockingCpuData(
dependencyGraph,
paintTs,
blockingScriptFilter,
extraBlockingCpuNodesToIncludeFilter
);

return dependencyGraph.cloneWithRelationships(node => {
if (node.endTime > fcp && !node.isMainDocument()) return false;
// Include EvaluateScript tasks for blocking scripts
if (node.type === BaseNode.TYPES.CPU) {
return node.isEvaluateScriptFor(blockingScriptUrls);
}
if (node.type === BaseNode.TYPES.NETWORK) {
// Exclude all nodes that ended after FCP (except for the main document which we always consider necessary)
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
if (node.endTime > paintTs && !node.isMainDocument()) return false;

// Include non-script-initiated network requests with a render-blocking priority
return node.hasRenderBlockingPriority();
const url = node.record.url;
// If the URL definitely wasn't render-blocking then we filter it out.
if (definitelyNotRenderBlockingScriptUrls.has(url)) {
Copy link
Member

Choose a reason for hiding this comment

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

definitelyNotRenderBlockingScriptUrls is a little confusing inside of getBlockingCpuData, but really nice here!

return false;
}
return node.hasRenderBlockingPriority();
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
} else {
// If it's a CPU node, just check if it was blocking.
return blockingCpuNodeIds.has(node.id);
}
});
}

/**
* @param {Node} dependencyGraph
* @param {LH.Artifacts.TraceOfTab} traceOfTab
* @return {Node}
*/
static getOptimisticGraph(dependencyGraph, traceOfTab) {
return this.getFirstPaintBasedGraph(
dependencyGraph,
traceOfTab.timestamps.firstContentfulPaint,
node => node.hasRenderBlockingPriority() && node.initiatorType !== 'script'
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
);
}

/**
* @param {Node} dependencyGraph
* @param {LH.Artifacts.TraceOfTab} traceOfTab
* @return {Node}
*/
static getPessimisticGraph(dependencyGraph, traceOfTab) {
return this.getFirstPaintBasedGraph(
dependencyGraph,
traceOfTab.timestamps.firstContentfulPaint,
node => node.hasRenderBlockingPriority()
);
}
}

module.exports = makeComputedArtifact(LanternFirstContentfulPaint);
46 changes: 13 additions & 33 deletions lighthouse-core/computed/metrics/lantern-first-meaningful-paint.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,10 @@

const makeComputedArtifact = require('../computed-artifact.js');
const LanternMetric = require('./lantern-metric.js');
const BaseNode = require('../../lib/dependency-graph/base-node.js');
const LHError = require('../../lib/lh-error.js');
const LanternFirstContentfulPaint = require('./lantern-first-contentful-paint.js');

/** @typedef {BaseNode.Node} Node */
/** @typedef {import('../../lib/dependency-graph/base-node.js').Node} Node */

class LanternFirstMeaningfulPaint extends LanternMetric {
/**
Expand All @@ -36,22 +35,11 @@ class LanternFirstMeaningfulPaint extends LanternMetric {
throw new LHError(LHError.errors.NO_FMP);
}

const blockingScriptUrls = LanternMetric.getScriptUrls(dependencyGraph, node => {
return (
node.endTime <= fmp && node.hasRenderBlockingPriority() && node.initiatorType !== 'script'
);
});

return dependencyGraph.cloneWithRelationships(node => {
if (node.endTime > fmp && !node.isMainDocument()) return false;
// Include EvaluateScript tasks for blocking scripts
if (node.type === BaseNode.TYPES.CPU) {
return node.isEvaluateScriptFor(blockingScriptUrls);
}

// Include non-script-initiated network requests with a render-blocking priority
return node.hasRenderBlockingPriority() && node.initiatorType !== 'script';
});
return LanternFirstContentfulPaint.getFirstPaintBasedGraph(
dependencyGraph,
fmp,
node => node.hasRenderBlockingPriority() && node.initiatorType !== 'script'
);
}

/**
Expand All @@ -65,21 +53,13 @@ class LanternFirstMeaningfulPaint extends LanternMetric {
throw new LHError(LHError.errors.NO_FMP);
}

const requiredScriptUrls = LanternMetric.getScriptUrls(dependencyGraph, node => {
return node.endTime <= fmp && node.hasRenderBlockingPriority();
});

return dependencyGraph.cloneWithRelationships(node => {
if (node.endTime > fmp && !node.isMainDocument()) return false;

// Include CPU tasks that performed a layout or were evaluations of required scripts
if (node.type === BaseNode.TYPES.CPU) {
return node.didPerformLayout() || node.isEvaluateScriptFor(requiredScriptUrls);
}

// Include all network requests that had render-blocking priority (even script-initiated)
return node.hasRenderBlockingPriority();
});
return LanternFirstContentfulPaint.getFirstPaintBasedGraph(
dependencyGraph,
fmp,
node => node.hasRenderBlockingPriority(),
// For pessimistic FMP we'll include *all* layout nodes
node => node.didPerformLayout()
);
}

/**
Expand Down
11 changes: 11 additions & 0 deletions lighthouse-core/lib/dependency-graph/cpu-node.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,17 @@ class CPUNode extends BaseNode {
return this._childEvents.some(evt => evt.name === 'Layout');
}

/**
* Returns true if this node contains any EvaluateScript task with a URL.
* @return {boolean}
*/
isEvaluateScript() {
return this._childEvents.some(evt => {
return evt.name === 'EvaluateScript' &&
!!evt.args.data && !!evt.args.data.url;
});
}

/**
* Returns true if this node contains the EvaluateScript task for a URL in the given set.
* @param {Set<string>} urls
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@

exports[`Performance: metrics evaluates valid input correctly 1`] = `
Object {
"estimatedInputLatency": 100,
"estimatedInputLatency": 78,
"estimatedInputLatencyTs": undefined,
"firstCPUIdle": 3351,
"firstCPUIdleTs": undefined,
"firstContentfulPaint": 911,
"firstContentfulPaint": 1307,
"firstContentfulPaintTs": undefined,
"firstMeaningfulPaint": 1355,
"firstMeaningfulPaint": 1511,
"firstMeaningfulPaintTs": undefined,
"interactive": 3427,
"interactiveTs": undefined,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,20 @@
exports[`Performance: predictive performance audit should compute the predicted values 1`] = `
Object {
"optimisticEIL": 93,
"optimisticFCP": 911,
"optimisticFMP": 1211,
"optimisticFCP": 1307,
"optimisticFMP": 1439,
"optimisticSI": 605,
"optimisticTTFCPUI": 3351,
"optimisticTTI": 3351,
"pessimisticEIL": 158,
"pessimisticFCP": 911,
"pessimisticFMP": 1498,
"pessimisticEIL": 101,
"pessimisticFCP": 1307,
"pessimisticFMP": 1583,
"pessimisticSI": 1631,
"pessimisticTTFCPUI": 3502,
"pessimisticTTI": 3502,
"roughEstimateOfEIL": 100,
"roughEstimateOfFCP": 911,
"roughEstimateOfFMP": 1355,
"roughEstimateOfEIL": 78,
"roughEstimateOfFCP": 1307,
"roughEstimateOfFMP": 1511,
"roughEstimateOfSI": 1657,
"roughEstimateOfTTFCPUI": 3351,
"roughEstimateOfTTI": 3427,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

exports[`Performance: first-meaningful-paint audit computes FMP correctly for simulated 1`] = `
Object {
"rawValue": 1354.506000038469,
"score": 1,
"rawValue": 1510.6100000208241,
"score": 0.99,
}
`;
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ describe('Performance: first-contentful-paint-3g audit', () => {
// Use InlineSnapshot here so changes to Lantern coefficients can be easily updated en masse
expect({score: result.score, value: Math.round(result.rawValue)}).toMatchInlineSnapshot(`
Object {
"score": 1,
"value": 1661,
"score": 0.99,
"value": 2057,
}
`);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
exports[`Metrics: EIL should compute a simulated value 1`] = `
Object {
"optimistic": 93,
"pessimistic": 158,
"timing": 100,
"pessimistic": 101,
"timing": 78,
}
`;
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

exports[`Metrics: FCP should compute a simulated value 1`] = `
Object {
"optimistic": 911,
"pessimistic": 911,
"timing": 911,
"optimistic": 1307,
"pessimistic": 1307,
"timing": 1307,
}
`;
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

exports[`Metrics: FMP should compute a simulated value 1`] = `
Object {
"optimistic": 1211,
"pessimistic": 1498,
"timing": 1355,
"optimistic": 1439,
"pessimistic": 1583,
"timing": 1511,
}
`;
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
exports[`Metrics: Lantern EIL should compute a simulated value 1`] = `
Object {
"optimistic": 93,
"pessimistic": 158,
"timing": 100,
"pessimistic": 101,
"timing": 78,
}
`;
Loading