-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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(cumulative-layout-shift): introduce CDT's trace processor for CLS #15225
Changes from all commits
af4be66
eed1013
58d2016
f8dc2a7
417f613
315a9f7
e52106a
4ee6efb
5543429
678c5ac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,10 +25,12 @@ import {LH_ROOT} from '../root.js'; | |
|
||
const outDir = `${LH_ROOT}/core/lib/cdt/generated`; | ||
|
||
const frontEndPath = 'node_modules/chrome-devtools-frontend/front_end'; | ||
|
||
/** @type {Modification[]} */ | ||
const modifications = [ | ||
{ | ||
input: 'node_modules/chrome-devtools-frontend/front_end/core/sdk/SourceMap.ts', | ||
input: `${frontEndPath}/core/sdk/SourceMap.ts`, | ||
output: `${outDir}/SourceMap.js`, | ||
template: [ | ||
'const Common = require(\'../Common.js\');', | ||
|
@@ -62,7 +64,7 @@ const modifications = [ | |
], | ||
}, | ||
{ | ||
input: 'node_modules/chrome-devtools-frontend/front_end/core/common/ParsedURL.ts', | ||
input: `${frontEndPath}/core/common/ParsedURL.ts`, | ||
output: `${outDir}/ParsedURL.js`, | ||
template: '%sourceFilePrinted%', | ||
rawCodeToReplace: {}, | ||
|
@@ -113,6 +115,122 @@ const modifications = [ | |
'Platform', | ||
], | ||
}, | ||
{ | ||
input: `${frontEndPath}/models/trace/types/types.ts`, | ||
output: `${outDir}/models/trace/types/types.js`, | ||
template: '%sourceFilePrinted%', | ||
rawCodeToReplace: {}, | ||
classesToRemove: [], | ||
methodsToRemove: [], | ||
variablesToRemove: [], | ||
}, | ||
{ | ||
input: `${frontEndPath}/models/trace/types/TraceEvents.ts`, | ||
output: `${outDir}/models/trace/types/TraceEvents.js`, | ||
template: '%sourceFilePrinted%', | ||
rawCodeToReplace: {}, | ||
classesToRemove: [], | ||
methodsToRemove: [], | ||
variablesToRemove: [], | ||
}, | ||
{ | ||
input: `${frontEndPath}/models/trace/types/Timing.ts`, | ||
output: `${outDir}/models/trace/types/Timing.js`, | ||
template: '%sourceFilePrinted%', | ||
rawCodeToReplace: {}, | ||
classesToRemove: [], | ||
methodsToRemove: [], | ||
variablesToRemove: [], | ||
}, | ||
{ | ||
input: `${frontEndPath}/models/trace/helpers/helpers.ts`, | ||
output: `${outDir}/models/trace/helpers/helpers.js`, | ||
template: '%sourceFilePrinted%', | ||
rawCodeToReplace: {}, | ||
classesToRemove: [], | ||
methodsToRemove: [], | ||
variablesToRemove: [], | ||
}, | ||
{ | ||
input: `${frontEndPath}/models/trace/helpers/Timing.ts`, | ||
output: `${outDir}/models/trace/helpers/Timing.js`, | ||
template: [ | ||
'const Platform = require(\'../../../../Platform.js\');', | ||
'%sourceFilePrinted%', | ||
].join('\n'), | ||
rawCodeToReplace: { | ||
'navigator.language': `'en'`, | ||
}, | ||
classesToRemove: [], | ||
methodsToRemove: [], | ||
variablesToRemove: [ | ||
'Platform', | ||
], | ||
}, | ||
{ | ||
input: `${frontEndPath}/models/trace/helpers/Trace.ts`, | ||
output: `${outDir}/models/trace/helpers/Trace.js`, | ||
template: [ | ||
'const Platform = require(\'../../../../Platform.js\');', | ||
'const Common = require(\'../../../../Common.js\');', | ||
'%sourceFilePrinted%', | ||
].join('\n'), | ||
rawCodeToReplace: {}, | ||
classesToRemove: [], | ||
methodsToRemove: [], | ||
variablesToRemove: [ | ||
'Platform', | ||
'Common', | ||
], | ||
}, | ||
{ | ||
input: `${frontEndPath}/models/trace/handlers/types.ts`, | ||
output: `${outDir}/models/trace/handlers/types.js`, | ||
template: '%sourceFilePrinted%', | ||
rawCodeToReplace: {}, | ||
classesToRemove: [], | ||
methodsToRemove: [], | ||
variablesToRemove: [], | ||
}, | ||
{ | ||
input: `${frontEndPath}/models/trace/handlers/MetaHandler.ts`, | ||
output: `${outDir}/models/trace/handlers/MetaHandler.js`, | ||
template: [ | ||
'const Platform = require(\'../../../../Platform.js\');', | ||
'%sourceFilePrinted%', | ||
].join('\n'), | ||
rawCodeToReplace: { | ||
'new DOMRect(viewportX, viewportY, viewportWidth, viewportHeight)': 'null', | ||
}, | ||
classesToRemove: [], | ||
methodsToRemove: [], | ||
variablesToRemove: [ | ||
'Platform', | ||
], | ||
}, | ||
{ | ||
input: `${frontEndPath}/models/trace/handlers/LayoutShiftsHandler.ts`, | ||
output: `${outDir}/models/trace/handlers/LayoutShiftsHandler.js`, | ||
template: [ | ||
'const Platform = require(\'../../../../Platform.js\');', | ||
'%sourceFilePrinted%', | ||
].join('\n'), | ||
rawCodeToReplace: { | ||
'findNextScreenshotSource(event.ts)': 'null', | ||
[`['Screenshots', 'Meta']`]: `['Meta']`, | ||
'!event.args.data?.had_recent_input': 'true', | ||
}, | ||
classesToRemove: [], | ||
methodsToRemove: [ | ||
'findNextScreenshotSource', | ||
'stateForLayoutShiftScore', | ||
], | ||
variablesToRemove: [ | ||
'Platform', | ||
'PageLoadMetricsHandler_js_1', | ||
'ScreenshotsHandler_js_1', | ||
], | ||
}, | ||
]; | ||
|
||
/** | ||
|
@@ -220,3 +338,9 @@ function writeGeneratedFile(outputPath, contents) { | |
} | ||
|
||
modifications.forEach(doModification); | ||
writeGeneratedFile(`${outDir}/models/trace/handlers/handlers.js`, ` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was it just easier to rewrite this file? Can you add a comment about this? |
||
module.exports.ModelHandlers = { | ||
Meta: require('./MetaHandler.js'), | ||
LayoutShiftsHandler: require('./LayoutShiftsHandler.js'), | ||
}; | ||
`); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,10 +4,13 @@ | |
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. | ||
*/ | ||
|
||
import log from 'lighthouse-logger'; | ||
|
||
import SDK from '../../lib/cdt/SDK.js'; | ||
import {makeComputedArtifact} from '../computed-artifact.js'; | ||
import {ProcessedTrace} from '../processed-trace.js'; | ||
|
||
/** @typedef {{ts: number, isMainFrame: boolean, weightedScore: number, impactedNodes?: LH.Artifacts.TraceImpactedNode[]}} LayoutShiftEvent */ | ||
/** @typedef {{ts: number, isMainFrame: boolean, weightedScore: number, impactedNodes?: LH.Artifacts.TraceImpactedNode[], event: LH.TraceEvent}} LayoutShiftEvent */ | ||
|
||
const RECENT_INPUT_WINDOW = 500; | ||
|
||
|
@@ -66,6 +69,7 @@ | |
isMainFrame: event.args.data.is_main_frame, | ||
weightedScore: event.args.data.weighted_score_delta, | ||
impactedNodes: event.args.data.impacted_nodes, | ||
event, | ||
}); | ||
} | ||
|
||
|
@@ -108,13 +112,38 @@ | |
*/ | ||
static async compute_(trace, context) { | ||
const processedTrace = await ProcessedTrace.request(trace, context); | ||
|
||
const allFrameShiftEvents = | ||
CumulativeLayoutShift.getLayoutShiftEvents(processedTrace); | ||
const mainFrameShiftEvents = allFrameShiftEvents.filter(e => e.isMainFrame); | ||
|
||
let cumulativeLayoutShift; | ||
try { | ||
// Experimental usage of CDT's trace processor. | ||
const processor = new SDK.TraceProcessor.TraceProcessor({ | ||
LayoutShifts: SDK.TraceHandlers.LayoutShiftsHandler, | ||
}); | ||
// We really want the logic that getLayoutShiftEvents provides for filtering out | ||
// some events based on recent input. | ||
// Would it make sense to upstream this to CDT? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would say yes. If we are going to unify our trace processors then we should avoid modifying the results. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jackfranklin does filtering out user-initiated layout shifts from perf panel sound good? (we don't need to block this initial integration on this) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the perf panel we have historically shown shifts even if they were user initiated, but no reason that the handler couldn't expose the set of user initiated shifts and set of non-user initiated shifts, or something like that, so that any consumer can use whichever set they want. |
||
const filteredTrace = processedTrace.frameTreeEvents.filter(event => { | ||
if (event.name !== 'LayoutShift') { | ||
return true; | ||
} | ||
|
||
return allFrameShiftEvents.some(lse => lse.event === event); | ||
}); | ||
await processor.parse(filteredTrace); | ||
const data = /** @type {import('../../lib/cdt/SDK.js').TraceProcessorResult} */ ( | ||
processor.data); | ||
cumulativeLayoutShift = data.LayoutShifts.sessionMaxScore; | ||
} catch (e) { | ||
// Something failed, so fallback to our own implementation. | ||
log.error('Error running SDK.TraceProcessor', e); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this deserves to be captured in sentry. If there are situations where the DT trace processor breaks then we should investigate and fix them. |
||
cumulativeLayoutShift = CumulativeLayoutShift.calculate(allFrameShiftEvents); | ||
} | ||
|
||
return { | ||
cumulativeLayoutShift: CumulativeLayoutShift.calculate(allFrameShiftEvents), | ||
cumulativeLayoutShift, | ||
cumulativeLayoutShiftMainFrame: CumulativeLayoutShift.calculate(mainFrameShiftEvents), | ||
}; | ||
} | ||
|
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.
All this complexity melts away once we validate this shared trace processor approach, and extract to a shared library.
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.
Is this the optimal order of operations? I haven't seen the design discussions so far, but this requires a bunch of generation/generated code to do a task we already know both code bases can do (CLS is straightforward and they both have extensive CLS tests).
The shared library is what historically no one had a unified vision of and is going to require work that will need to stick around (e.g. making code living in DevTools importable). Does it make more sense to start there so the infrastructure won't have to melt away, it won't be needed from the start?