-
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(third-party-summary): add blocking time impact #9486
Changes from all commits
d70939c
e434fd4
bbd5328
f46e441
9db995b
782ad13
d55a822
676b9b0
9b36046
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 | ||||
---|---|---|---|---|---|---|
|
@@ -14,25 +14,28 @@ const NetworkRecords = require('../computed/network-records.js'); | |||||
const MainThreadTasks = require('../computed/main-thread-tasks.js'); | ||||||
|
||||||
const UIStrings = { | ||||||
/** Title of a Lighthouse audit that identifies the code on the page that the user doesn't control. This is shown in a list of audits that Lighthouse generates. */ | ||||||
title: 'Third-Party Usage', | ||||||
/** Title of a diagnostic audit that provides details about the code on a web page that the user doesn't control (referred to as "third-party code"). This descriptive title is shown to users when the amount is acceptable and no user action is required. */ | ||||||
title: 'Third-Party usage', | ||||||
/** Title of a diagnostic audit that provides details about the code on a web page that the user doesn't control (referred to as "third-party code"). This imperative title is shown to users when there is a significant amount of page execution time caused by third-party code that should be reduced. */ | ||||||
failureTitle: 'Reduce the impact of third-party code', | ||||||
/** Description of a Lighthouse audit that identifies the code on the page that the user doesn't control. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */ | ||||||
description: 'Third-party code can significantly impact load performance. ' + | ||||||
'Limit the number of redundant third-party providers and try to load third-party code after ' + | ||||||
'your page has primarily finished loading. [Learn more](https://developers.google.com/web/fundamentals/performance/optimizing-content-efficiency/loading-third-party-javascript/).', | ||||||
/** Label for a table column that displays the name of a third-party provider that potentially links to their website. */ | ||||||
columnThirdParty: 'Third-Party', | ||||||
/** Label for a table column that displays how much time each row spent executing on the main thread, entries will be the number of milliseconds spent. */ | ||||||
columnMainThreadTime: 'Main Thread Time', | ||||||
/** Summary text for the result of a Lighthouse audit that identifies the code on the page that the user doesn't control. This text summarizes the number of distinct entities that were found on the page. */ | ||||||
displayValue: `{itemCount, plural, | ||||||
=1 {1 Third-Party Found} | ||||||
other {# Third-Parties Found} | ||||||
}`, | ||||||
/** Label for a table column that displays how much time each row spent blocking other work on the main thread, entries will be the number of milliseconds spent. */ | ||||||
columnBlockingTime: 'Main-Thread Blocking Time', | ||||||
/** Summary text for the result of a Lighthouse audit that identifies the code on a web page that the user doesn't control (referred to as "third-party code"). This text summarizes the number of distinct entities that were found on the page. */ | ||||||
displayValue: 'Third-party code blocked the main thread for ' + | ||||||
`{timeInMs, number, milliseconds}\xa0ms`, | ||||||
}; | ||||||
|
||||||
const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings); | ||||||
|
||||||
// A page passes when all third-party code blocks for less than 250 ms. | ||||||
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. maybe put some version of the comment in the PR description
in here to give motivation to the particular number? 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. yeah sg
Suggested change
|
||||||
const PASS_THRESHOLD_IN_MS = 250; | ||||||
|
||||||
/** @typedef {import("third-party-web").IEntity} ThirdPartyEntity */ | ||||||
|
||||||
class ThirdPartySummary extends Audit { | ||||||
|
@@ -43,8 +46,8 @@ class ThirdPartySummary extends Audit { | |||||
return { | ||||||
id: 'third-party-summary', | ||||||
title: str_(UIStrings.title), | ||||||
failureTitle: str_(UIStrings.failureTitle), | ||||||
description: str_(UIStrings.description), | ||||||
scoreDisplayMode: Audit.SCORING_MODES.INFORMATIVE, | ||||||
requiredArtifacts: ['traces', 'devtoolsLogs'], | ||||||
}; | ||||||
} | ||||||
|
@@ -69,17 +72,18 @@ class ThirdPartySummary extends Audit { | |||||
* @param {Array<LH.Artifacts.NetworkRequest>} networkRecords | ||||||
* @param {Array<LH.Artifacts.TaskNode>} mainThreadTasks | ||||||
* @param {number} cpuMultiplier | ||||||
* @return {Map<ThirdPartyEntity, {mainThreadTime: number, transferSize: number}>} | ||||||
* @return {Map<ThirdPartyEntity, {mainThreadTime: number, transferSize: number, blockingTime: number}>} | ||||||
*/ | ||||||
static getSummaryByEntity(networkRecords, mainThreadTasks, cpuMultiplier) { | ||||||
/** @type {Map<ThirdPartyEntity, {mainThreadTime: number, transferSize: number}>} */ | ||||||
/** @type {Map<ThirdPartyEntity, {mainThreadTime: number, transferSize: number, blockingTime: number}>} */ | ||||||
const entities = new Map(); | ||||||
const defaultEntityStat = {mainThreadTime: 0, blockingTime: 0, transferSize: 0}; | ||||||
|
||||||
for (const request of networkRecords) { | ||||||
const entity = ThirdPartySummary.getEntitySafe(request.url); | ||||||
if (!entity) continue; | ||||||
|
||||||
const entityStats = entities.get(entity) || {mainThreadTime: 0, transferSize: 0}; | ||||||
const entityStats = entities.get(entity) || {...defaultEntityStat}; | ||||||
entityStats.transferSize += request.transferSize; | ||||||
entities.set(entity, entityStats); | ||||||
} | ||||||
|
@@ -91,8 +95,14 @@ class ThirdPartySummary extends Audit { | |||||
const entity = ThirdPartySummary.getEntitySafe(attributeableURL); | ||||||
if (!entity) continue; | ||||||
|
||||||
const entityStats = entities.get(entity) || {mainThreadTime: 0, transferSize: 0}; | ||||||
entityStats.mainThreadTime += task.selfTime * cpuMultiplier; | ||||||
const entityStats = entities.get(entity) || {...defaultEntityStat}; | ||||||
const taskDuration = task.selfTime * cpuMultiplier; | ||||||
// The amount of time spent on main thread is the sum of all durations. | ||||||
entityStats.mainThreadTime += taskDuration; | ||||||
// The amount of time spent *blocking* on main thread is the sum of all time longer than 50ms. | ||||||
// Note that this is not totally equivalent to the TBT definition since it fails to account for FCP, | ||||||
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. is this a TODO or worth reexamining at some point? 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. IMO not a TODO ever worth fixing since it should be rare that third party code actually blocks FCP and if it does I'm fine pointing the finger at it anyway. Our slight decoupling of the audit from TBT in external descriptions also makes this less of an issue. |
||||||
// but a majority of third-party work occurs after FCP and should yield largely similar numbers. | ||||||
entityStats.blockingTime += Math.max(taskDuration - 50, 0); | ||||||
entities.set(entity, entityStats); | ||||||
} | ||||||
|
||||||
|
@@ -117,15 +127,10 @@ class ThirdPartySummary extends Audit { | |||||
|
||||||
const summary = {wastedBytes: 0, wastedMs: 0}; | ||||||
|
||||||
// Sort by a combined measure of bytes + main thread time. | ||||||
// 1KB ~= 1 ms | ||||||
/** @param {{transferSize: number, mainThreadTime: number}} stats */ | ||||||
const computeSortValue = stats => stats.transferSize / 1024 + stats.mainThreadTime; | ||||||
|
||||||
const results = Array.from(summaryByEntity.entries()) | ||||||
.map(([entity, stats]) => { | ||||||
summary.wastedBytes += stats.transferSize; | ||||||
summary.wastedMs += stats.mainThreadTime; | ||||||
summary.wastedMs += stats.blockingTime; | ||||||
|
||||||
return { | ||||||
entity: /** @type {LH.Audit.Details.LinkValue} */ ({ | ||||||
|
@@ -135,17 +140,19 @@ class ThirdPartySummary extends Audit { | |||||
}), | ||||||
transferSize: stats.transferSize, | ||||||
mainThreadTime: stats.mainThreadTime, | ||||||
blockingTime: stats.blockingTime, | ||||||
}; | ||||||
}) | ||||||
.sort((a, b) => computeSortValue(b) - computeSortValue(a)); | ||||||
// Sort by blocking time first, then transfer size to break ties. | ||||||
.sort((a, b) => (b.blockingTime - a.blockingTime) || (b.transferSize - a.transferSize)); | ||||||
|
||||||
/** @type {LH.Audit.Details.Table['headings']} */ | ||||||
const headings = [ | ||||||
{key: 'entity', itemType: 'link', text: str_(UIStrings.columnThirdParty)}, | ||||||
{key: 'transferSize', granularity: 1, itemType: 'bytes', | ||||||
text: str_(i18n.UIStrings.columnSize)}, | ||||||
{key: 'mainThreadTime', granularity: 1, itemType: 'ms', | ||||||
text: str_(UIStrings.columnMainThreadTime)}, | ||||||
{key: 'blockingTime', granularity: 1, itemType: 'ms', | ||||||
text: str_(UIStrings.columnBlockingTime)}, | ||||||
]; | ||||||
|
||||||
if (!results.length) { | ||||||
|
@@ -156,8 +163,11 @@ class ThirdPartySummary extends Audit { | |||||
} | ||||||
|
||||||
return { | ||||||
score: Number(results.length === 0), | ||||||
displayValue: str_(UIStrings.displayValue, {itemCount: results.length}), | ||||||
score: Number(summary.wastedMs <= PASS_THRESHOLD_IN_MS), | ||||||
displayValue: str_(UIStrings.displayValue, { | ||||||
itemCount: results.length, | ||||||
timeInMs: summary.wastedMs, | ||||||
}), | ||||||
details: Audit.makeTableDetails(headings, results, summary), | ||||||
}; | ||||||
} | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,10 @@ describe('Third party summary', () => { | |
|
||
const results = await ThirdPartySummary.audit(artifacts, {computedCache: new Map()}); | ||
|
||
expect(results.displayValue).toBeDisplayString('2 Third-Parties Found'); | ||
expect(results.score).toBe(1); | ||
expect(results.displayValue).toBeDisplayString( | ||
'Third-party code blocked the main thread for 20 ms' | ||
); | ||
expect(results.details.items).toEqual([ | ||
{ | ||
entity: { | ||
|
@@ -31,6 +34,7 @@ describe('Third party summary', () => { | |
url: 'https://marketingplatform.google.com/about/tag-manager/', | ||
}, | ||
mainThreadTime: 104.70300000000002, | ||
blockingTime: 18.186999999999998, | ||
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. just interesting to see the impact here drop (104ms -> 18ms) 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. yeah I'm a bit disappointed to see the very busy third parties disappear, but if it's an incentive to script responsibly than I guess it's still a very positive outcome so I'm :) My biggest concern is that no one is going to understand what "main-thread blocking time" actually is and they'll assume it's the time spent which would be sad. FWIW this is also not as drastic in the lantern case, it goes from 419 -> 250. While still a big drop, the number is still noticeable. |
||
transferSize: 30827, | ||
}, | ||
{ | ||
|
@@ -40,6 +44,7 @@ describe('Third party summary', () => { | |
url: 'https://www.google.com/analytics/analytics/', | ||
}, | ||
mainThreadTime: 87.576, | ||
blockingTime: 0, | ||
transferSize: 20913, | ||
}, | ||
]); | ||
|
@@ -54,9 +59,12 @@ describe('Third party summary', () => { | |
const settings = {throttlingMethod: 'simulate', throttling: {cpuSlowdownMultiplier: 4}}; | ||
const results = await ThirdPartySummary.audit(artifacts, {computedCache: new Map(), settings}); | ||
|
||
expect(results.score).toBe(0); | ||
expect(results.details.items).toHaveLength(2); | ||
expect(Math.round(results.details.items[0].mainThreadTime)).toEqual(419); | ||
expect(Math.round(results.details.items[0].blockingTime)).toEqual(250); | ||
expect(Math.round(results.details.items[1].mainThreadTime)).toEqual(350); | ||
expect(Math.round(results.details.items[1].blockingTime)).toEqual(157); | ||
}); | ||
|
||
it('be not applicable when no third parties are present', async () => { | ||
|
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 think there might have been an issue with describing the "main thread" to the tc... @exterkamp? Or was it for this audit and it's already been clarified :)
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.
The problem was with "thread" being used, in regards to the description in
first-interactive
I think this is fine here. Maybe add something about threads. But I think that is unnecessary. This LGTM.