core(module-duplication): ignore smaller modules #11277
Conversation
…t of the similar name
| */ | ||
| static _normalizeAggregatedData(sourceDataAggregated) { | ||
| // Sort by resource size. | ||
| for (const sourceData of sourceDataAggregated.values()) { |
There was a problem hiding this comment.
this can move into the larger for..of entries() block. just right at the top before we start relying on the first being biggest.
| } | ||
|
|
||
| // Delete source datas with only one value (no duplicates). | ||
| for (const [key, sourceData] of sourceDataAggregated.entries()) { |
There was a problem hiding this comment.
this can move into the the above for...of entries() block.. and be done instead of the continue
| static _normalizeAggregatedData(sourceDataAggregated) { | ||
| // Sort by resource size. | ||
| for (const sourceData of sourceDataAggregated.values()) { | ||
| if (sourceData.length > 1) sourceData.sort((a, b) => b.resourceSize - a.resourceSize); |
There was a problem hiding this comment.
obv not a big deal but why the need to check length before sorting?
| if (sourceData.length > 1) sourceData.sort((a, b) => b.resourceSize - a.resourceSize); | ||
| } | ||
|
|
||
| // Remove modules smaller than 50% size of largest. |
There was a problem hiding this comment.
as discussed on voice, let's threshold on bytes instead of relative size percent.
| /** | ||
| * @param {Map<string, Array<{scriptUrl: string, resourceSize: number}>>} moduleNameToSourceData | ||
| */ | ||
| static _normalizeAggregatedData(moduleNameToSourceData) { |
There was a problem hiding this comment.
the sorting in here seems to fit semantically with a computed artifact, but the rest of it seems like opinion that we keep relegated to the audit.
so we move it here?
There was a problem hiding this comment.
@connorjclark sez he'll do this in a followup PR.
he gave his implicit word he'd doing it in a timely manner. 😆
| if (sourceData.length > 1) sourceData.sort((a, b) => b.resourceSize - a.resourceSize); | ||
| } | ||
|
|
||
| // Remove modules smaller than 90% size of largest. |
There was a problem hiding this comment.
i still dont totally understand why we want to ignore all of these. seems like a very high threshold.
There was a problem hiding this comment.
I had the comment wrong. updated.
| sourceData.sort((a, b) => b.resourceSize - a.resourceSize); | ||
| } | ||
|
|
||
| // Remove modules smaller than 90% size of largest. |
There was a problem hiding this comment.
| // Remove modules smaller than 90% size of largest. | |
| // Remove modules that are <= 10% of the size of the largest module instance. |
| /** | ||
| * @param {Map<string, Array<{scriptUrl: string, resourceSize: number}>>} moduleNameToSourceData | ||
| */ | ||
| static _normalizeAggregatedData(moduleNameToSourceData) { |
There was a problem hiding this comment.
@connorjclark sez he'll do this in a followup PR.
he gave his implicit word he'd doing it in a timely manner. 😆
| */ | ||
| static _normalizeAggregatedData(moduleNameToSourceData) { | ||
| // Sort by resource size. | ||
| for (const sourceData of moduleNameToSourceData.values()) { |
There was a problem hiding this comment.
see comments about sharing the same loop
https://github.com/GoogleChrome/lighthouse/pull/11277/files/9c781f447b29e75b04d48d8506545703af22d216#diff-d745ba9e24b3997693bca38c6f952188
avoids situations like this:
which would then be shown oddly in the treemap app by highlighting just the big module, and the other modules are way too small to even see.