core(opportunities): take max of savings on TTI, load #5084
Conversation
| @@ -314,6 +314,7 @@ declare global { | |||
|
|
|||
| export interface LanternMetric { | |||
| timing: number; | |||
| timestamp?: never; | |||
There was a problem hiding this comment.
allows you to check existence of timestamp to see if it's Metric or LanternMetric
| @@ -122,7 +122,7 @@ class Node { | |||
| * Clones the entire graph connected to this node filtered by the optional predicate. If a node is | |||
| * included by the predicate, all nodes along the paths between the two will be included. If the | |||
| * node that was called clone is not included in the resulting filtered graph, the method will throw. | |||
| * @param {function(Node):boolean=} predicate | |||
| * @param {function(Node):boolean} [predicate] | |||
There was a problem hiding this comment.
typechecker was always expecting at least one argument otherwise
| @@ -26,12 +26,12 @@ class OffscreenImages extends ByteEfficiencyAudit { | |||
| static get meta() { | |||
| return { | |||
| name: 'offscreen-images', | |||
| description: 'Offscreen images', | |||
| description: 'Defer offscreen images', | |||
| Interactive.getLastLongTaskEndTime(simulationAfterChanges.nodeTimings), | ||
| 0 | ||
| ); | ||
| const savingsOnLoad = options.includeLoad ? |
There was a problem hiding this comment.
how about we just default to this? no option needed.
There was a problem hiding this comment.
well that's effectively what's happening with the default options above
There was a problem hiding this comment.
sure, just pitching the idea of removing this condition entirely. always use savingsOnload.
There was a problem hiding this comment.
maybe agree with the above. It's not clear why offscreen-images is different and would be the only one to opt out
There was a problem hiding this comment.
because this function takes how many bytes that can be removed from each resource and simulates the graph if they were removed.
in offscreen images, we can't actually remove the bytes from being loaded at all. i.e. the result of this function with includeLoad is no longer "defer offscreen images", but "delete all offscreen images". restricting the computation to savings on TTI preserves the contract of "defer offscreen images" because it's saying how much earlier could you push TTI if you loaded all this junk after TTI
There was a problem hiding this comment.
ok, makes sense. Maybe savingsOnOverallLoad or something for the variable name to make it clear it's our handwavey overall load numbers and not onload?
| @@ -117,11 +132,13 @@ class OffscreenImages extends ByteEfficiencyAudit { | |||
| return results; | |||
| }, new Map()); | |||
|
|
|||
| // TODO(phulce): move this to always use lantern | |||
There was a problem hiding this comment.
why not move this to lantern like the other ones?
There was a problem hiding this comment.
oh this is moving it to be lantern-based 👍
the extra check and comment below is clarifying that the timestamp dance is just an extra check in the non-lantern case, the graph should cover us for the most part
There was a problem hiding this comment.
got it. Comment + code (lantern artifact doesn't have timestamp) was a little confusing, I guess.
Maybe add something about timestamp like // If we're in the Lantern case and `timestamp` isn't available, we just have to rely...
| Interactive.getLastLongTaskEndTime(simulationAfterChanges.nodeTimings), | ||
| 0 | ||
| ); | ||
| const savingsOnLoad = options.includeLoad ? |
There was a problem hiding this comment.
maybe agree with the above. It's not clear why offscreen-images is different and would be the only one to opt out
| Interactive.getLastLongTaskEndTime(simulationAfterChanges.nodeTimings), | ||
| 0 | ||
| ); | ||
| const savingsOnLoad = options.includeLoad ? |
There was a problem hiding this comment.
ok, makes sense. Maybe savingsOnOverallLoad or something for the variable name to make it clear it's our handwavey overall load numbers and not onload?
| simulationBeforeChanges.timeInMs - simulationAfterChanges.timeInMs : | ||
| Number.MIN_VALUE; | ||
| const savingsOnTTI = Interactive.getLastLongTaskEndTime(simulationBeforeChanges.nodeTimings) - | ||
| Interactive.getLastLongTaskEndTime(simulationAfterChanges.nodeTimings); |
There was a problem hiding this comment.
rather than this Number.MIN_VALUE thing, what about
const savingsOnOverallLoad = simulationBeforeChanges.timeInMs - simulationAfterChanges.timeInMs;
const savingsOnTTI = ...;
const savings = options.includeLoad ? Math.max(savingsOnOverallLoad, savingsOnTTI) : savingsOnTTI;
return Math.round(savings / 10) * 10;?
| @@ -117,11 +132,13 @@ class OffscreenImages extends ByteEfficiencyAudit { | |||
| return results; | |||
| }, new Map()); | |||
|
|
|||
| // TODO(phulce): move this to always use lantern | |||
There was a problem hiding this comment.
got it. Comment + code (lantern artifact doesn't have timestamp) was a little confusing, I guess.
Maybe add something about timestamp like // If we're in the Lantern case and `timestamp` isn't available, we just have to rely...
| * @return {number} | ||
| */ | ||
| static computeWasteWithTTIGraph(results, graph, simulator) { | ||
| static computeWasteWithTTIGraph(results, graph, simulator, options) { |
There was a problem hiding this comment.
add docstring for includeLoad option :)
closes #5017
byte efficiency audits will now take max savings on TTI/load
I had to address offscreen images at the same time otherwise they'd start way over-reporting
also contains some drive-by consistency text changes I noticed while testing, opportunity audits are actions for the user to take