core(font-size): speed up gatherer #6391
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,12 +17,10 @@ | |
| */ | ||
|
|
||
| const Gatherer = require('../gatherer'); | ||
| const FONT_SIZE_PROPERTY_NAME = 'font-size'; | ||
| const TEXT_NODE_BLOCK_LIST = new Set(['SCRIPT', 'STYLE', 'NOSCRIPT']); | ||
| const MINIMAL_LEGIBLE_FONT_SIZE_PX = 12; | ||
| // limit number of protocol calls to make sure that gatherer doesn't take more than 1-2s | ||
| const MAX_NODES_SOURCE_RULE_FETCHED = 50; // number of nodes to fetch the source font-size rule | ||
|
|
||
| /** DevTools uses a numeric enum for nodeType */ | ||
|
|
@@ -31,6 +29,7 @@ const TEXT_NODE_TYPE = 3; | |
| /** @typedef {import('../../driver.js')} Driver */ | ||
| /** @typedef {LH.Artifacts.FontSize['analyzedFailingNodesData'][0]} NodeFontData */ | ||
| /** @typedef {LH.Artifacts.FontSize.DomNodeMaybeWithParent} DomNodeMaybeWithParent*/ | ||
| /** @typedef {Map<number, {fontSize: number, textLength: number}>} BackendIdsToFontData */ | ||
|
|
||
| /** | ||
| * @param {LH.Artifacts.FontSize.DomNodeMaybeWithParent=} node | ||
|
|
@@ -178,11 +177,11 @@ function getEffectiveFontRule({inlineStyle, matchedCSSRules, inherited}) { | |
| } | ||
|
|
||
| /** | ||
| * @param {string} nodeValue | ||
| * @returns {number} | ||
| */ | ||
| function getNodeTextLength(nodeValue) { | ||
| return nodeValue.trim().length; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -209,78 +208,14 @@ async function fetchSourceRule(driver, node) { | |
| } | ||
|
|
||
| /** | ||
| * @param {{nodeType: number, parentNodeName: string}} node | ||
| * @returns {boolean} | ||
| */ | ||
| function isTextNode(node) { | ||
| return node.nodeType === TEXT_NODE_TYPE && !TEXT_NODE_BLOCK_LIST.has(node.parentNodeName); | ||
| } | ||
|
|
||
| class FontSize extends Gatherer { | ||
| /** | ||
| * @param {LH.Gatherer.PassContext['driver']} driver | ||
| * @param {Array<NodeFontData>} failingNodes | ||
|
|
@@ -304,6 +239,99 @@ class FontSize extends Gatherer { | |
| return {analyzedFailingNodesData, analyzedFailingTextLength}; | ||
| } | ||
|
|
||
| /** | ||
| * @param {LH.Crdp.DOMSnapshot.CaptureSnapshotResponse} snapshot | ||
| * @return {BackendIdsToFontData} | ||
| */ | ||
| calculateBackendIdsToFontData(snapshot) { | ||
| const strings = snapshot.strings; | ||
|
|
||
| // The document under analysis is the root document. | ||
| const doc = snapshot.documents[0]; | ||
|
|
||
| // doc is a flattened property list describing all the Nodes in a document, with all string values | ||
| // deduped in a strings array. | ||
| // Implementation: | ||
| // https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/inspector/inspector_dom_snapshot_agent.cc?sq=package:chromium&g=0&l=534 | ||
|
|
||
| // satisfy the type checker that all expected values exist | ||
| if (!doc || !doc.nodes.nodeType || !doc.nodes.nodeName || !doc.nodes.backendNodeId | ||
| || !doc.nodes.nodeValue || !doc.nodes.parentIndex) { | ||
| throw new Error('Unexpected response from DOMSnapshot.captureSnapshot.'); | ||
| } | ||
|
There was a problem hiding this comment. it looks like these are meant to satisfy the type checker? Maybe that's worth commenting, e.g. |
||
|
|
||
| // Not all nodes have computed styles (ex: TextNodes), so doc.layout.* is smaller than doc.nodes.* | ||
| // doc.layout.nodeIndex maps the index into doc.layout.styles to an index into doc.nodes.* | ||
| // nodeIndexToStyleIndex inverses that mapping. | ||
| /** @type {Map<number, number>} */ | ||
| const nodeIndexToStyleIndex = new Map(); | ||
| doc.layout.nodeIndex.forEach((nodeIndex, styleIndex) => { | ||
| nodeIndexToStyleIndex.set(nodeIndex, styleIndex); | ||
| }); | ||
|
|
||
| /** @type {BackendIdsToFontData} */ | ||
| const backendIdsToFontData = new Map(); | ||
| for (let i = 0; i < doc.nodes.nodeType.length; i++) { | ||
| const nodeType = doc.nodes.nodeType[i]; | ||
| const nodeValue = strings[doc.nodes.nodeValue[i]]; | ||
| const parentIndex = doc.nodes.parentIndex[i]; | ||
| const parentNodeName = strings[doc.nodes.nodeName[parentIndex]]; | ||
| if (!isTextNode({ | ||
| nodeType, | ||
| parentNodeName, | ||
| })) continue; | ||
|
|
||
| const styleIndex = nodeIndexToStyleIndex.get(parentIndex); | ||
| if (!styleIndex) continue; | ||
| const textLength = getNodeTextLength(nodeValue); | ||
| if (!textLength) continue; // ignore empty TextNodes | ||
| const parentStyles = doc.layout.styles[styleIndex]; | ||
| // each styles is an array of string indices, one for each property given | ||
|
There was a problem hiding this comment. each styles? There was a problem hiding this comment.
|
||
| // to DOMSnapshot.captureSnapshot. We only requested font-size, so there's just | ||
| // the one string index here. | ||
| const [fontSizeStringId] = parentStyles; | ||
|
There was a problem hiding this comment. this is arbitrary enough that this probably deserves a comment. Is it the only one in that array because There was a problem hiding this comment. a comment here would still be good too. The type is just There was a problem hiding this comment. Added a comment explaining why there's just the one value here. I'm unsure about adding an assertion for .length === 1 here. This property is inherent to the API for this protocol method - do we assert post conditions like this often for other protocol methods? |
||
| // expects values like '11.5px' here | ||
| const fontSize = parseFloat(strings[fontSizeStringId]); | ||
| backendIdsToFontData.set(doc.nodes.backendNodeId[i], { | ||
| fontSize, | ||
| textLength, | ||
| }); | ||
| } | ||
|
|
||
| return backendIdsToFontData; | ||
| } | ||
|
|
||
| /** | ||
| * The only connection between a snapshot Node and an actual Protocol Node is backendId, | ||
| * so that is used to join the two data structures. DOMSnapshot.captureSnapshot doesn't | ||
| * give the entire Node object, so DOM.getFlattenedDocument is used. | ||
| * @param {BackendIdsToFontData} backendIdsToFontData | ||
| * @param {LH.Artifacts.FontSize.DomNodeWithParent[]} crdpNodes | ||
| */ | ||
| findFailingNodes(backendIdsToFontData, crdpNodes) { | ||
| /** @type {NodeFontData[]} */ | ||
| const failingNodes = []; | ||
|
There was a problem hiding this comment. do you think we could move all this into some functions for the different steps you're adding so it looks like there's roughly the following stages
is that roughly accurate? There was a problem hiding this comment. Yeah that sounds good. I mostly wanted to wait for PR feedback before chopping up afterPass, just incase the whole thing was moot or something :P |
||
| let totalTextLength = 0; | ||
| let failingTextLength = 0; | ||
| for (const crdpNode of crdpNodes) { | ||
| const partialFontData = backendIdsToFontData.get(crdpNode.backendNodeId); | ||
| if (!partialFontData) continue; // wasn't a non-empty TextNode | ||
| const {fontSize, textLength} = partialFontData; | ||
| totalTextLength += textLength; | ||
| if (fontSize < MINIMAL_LEGIBLE_FONT_SIZE_PX) { | ||
| // Once a bad TextNode is identified, its parent Node is needed. | ||
| failingTextLength += textLength; | ||
| failingNodes.push({ | ||
| node: crdpNode.parentNode, | ||
| textLength, | ||
| fontSize, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| return {totalTextLength, failingTextLength, failingNodes}; | ||
| } | ||
|
|
||
| /** | ||
| * @param {LH.Gatherer.PassContext} passContext | ||
| * @return {Promise<LH.Artifacts.FontSize>} font-size analysis | ||
|
|
@@ -316,17 +344,24 @@ class FontSize extends Gatherer { | |
| passContext.driver.on('CSS.styleSheetAdded', onStylesheetAdd); | ||
|
|
||
| await Promise.all([ | ||
| passContext.driver.sendCommand('DOMSnapshot.enable'), | ||
| passContext.driver.sendCommand('DOM.enable'), | ||
| passContext.driver.sendCommand('CSS.enable'), | ||
| ]); | ||
|
|
||
| // Use DOMSnapshot.captureSnapshot to get the computed font-size style of every node. | ||
| const snapshot = await passContext.driver.sendCommand('DOMSnapshot.captureSnapshot', { | ||
|
There was a problem hiding this comment. very small speedup if you run these two in parallel: lighthouse-core/gather/gatherers/seo/font-size.js | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js
index e6d0dbe0..3ceac9bd 100644
--- a/lighthouse-core/gather/gatherers/seo/font-size.js
+++ b/lighthouse-core/gather/gatherers/seo/font-size.js
@@ -350,13 +350,15 @@ class FontSize extends Gatherer {
]);
// Use DOMSnapshot.captureSnapshot to get the computed font-size style of every node.
- const snapshot = await passContext.driver.sendCommand('DOMSnapshot.captureSnapshot', {
+ const snapshotPromise = passContext.driver.sendCommand('DOMSnapshot.captureSnapshot', {
computedStyles: ['font-size'],
});
+ // but nodes will only contain the Body node and its descendants.
+ const allNodesPromise = getAllNodesFromBody(passContext.driver);
+ const [snapshot, crdpNodes] = await Promise.all([snapshotPromise, allNodesPromise]);
+
// backendIdsToFontData will include all non-empty TextNodes,
const backendIdsToFontData = this.calculateBackendIdsToFontData(snapshot);
- // but nodes will only contain the Body node and its descendants.
- const crdpNodes = await getAllNodesFromBody(passContext.driver);
const {
totalTextLength,
failingTextLength, |
||
| computedStyles: ['font-size'], | ||
| }); | ||
| // backendIdsToFontData will include all non-empty TextNodes, | ||
| const backendIdsToFontData = this.calculateBackendIdsToFontData(snapshot); | ||
| // but nodes will only contain the Body node and its descendants. | ||
| const crdpNodes = await getAllNodesFromBody(passContext.driver); | ||
|
There was a problem hiding this comment. getAllNodesFromBody relies on Using #6410 it looks like getFlattenedDocument is slower, so it seems nice to drop it.
There was a problem hiding this comment. The FontSize artifact contains entire CRDP Nodes. The snapshot data doesn't have all the data to reconstruct a CRDP Node. There was a problem hiding this comment. I think we don't need everything that's in CRDP Node. There was a problem hiding this comment.
we need There was a problem hiding this comment. @paulirish , given the above (we need the snapshot + the flattened document protocol calls, to correlate failing nodes with their matched styles), should we still reduce the scope of artifact? There was a problem hiding this comment. Yeah looks like the snapshot doesn't have K. We'll keep the current protocol calls. :) |
||
| const { | ||
| totalTextLength, | ||
| failingTextLength, | ||
| failingNodes, | ||
| } = this.findFailingNodes(backendIdsToFontData, crdpNodes); | ||
| const { | ||
| analyzedFailingNodesData, | ||
| analyzedFailingTextLength, | ||
|
|
@@ -340,6 +375,7 @@ class FontSize extends Gatherer { | |
| .forEach(data => (data.cssRule.stylesheet = stylesheets.get(data.cssRule.styleSheetId))); | ||
|
|
||
| await Promise.all([ | ||
| passContext.driver.sendCommand('DOMSnapshot.disable'), | ||
| passContext.driver.sendCommand('DOM.disable'), | ||
| passContext.driver.sendCommand('CSS.disable'), | ||
| ]); | ||
|
|
@@ -348,7 +384,6 @@ class FontSize extends Gatherer { | |
| analyzedFailingNodesData, | ||
| analyzedFailingTextLength, | ||
| failingTextLength, | ||
| totalTextLength, | ||
| }; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed
visitedTextLengthbecause we can now easily visit every node (it's one protocol call).