lantern: keep first layout/paint/parse events regardless of duration #9922
Conversation
There was a problem hiding this comment.
I think this is exactly what we need :) 👍
unfortunately the exact code we use to filter this is going to completely change if we decide to move forward with #9910 (comment) :/
maybe we hold off on hardening this but we keep it open in draft to ensure the numbers don't change on us too much?
|
Is this still relevant? |
| "optimistic": 1307, | ||
| "pessimistic": 1307, | ||
| "timing": 1307, | ||
| "optimistic": 1337, |
|
meta note: it'd be nice if we saved a summary of |
There was a problem hiding this comment.
a bummer that this accidentally hurts the basket sites slightly and doesn't fix any of those cases, but it's definitely more true to life and should fix #9627
LGTM % the weird node count thing being nothing
| // Don't prune this node. The task is long so it will impact simulation. | ||
| // Don't prune if event is the first ParseHTML/Layout/Paint. | ||
| // See https://github.com/GoogleChrome/lighthouse/issues/9627#issuecomment-526699524 for more. | ||
| let isFirst = false; |
There was a problem hiding this comment.
| let isFirst = false; | |
| let isFirstOfImportantEventType = false; |
maybe? :)
There was a problem hiding this comment.
does this really help with understanding? kinda a mouth full (mind full?) to read
There was a problem hiding this comment.
helps IMO, but I don't feel strongly can skip if you think it makes things worse
| @@ -66,7 +66,7 @@ describe('FirstInteractive computed artifact:', () => { | |||
| pessimistic: Math.round(result.pessimisticEstimate.timeInMs), | |||
| }).toMatchSnapshot(); | |||
| assert.equal(result.optimisticEstimate.nodeTimings.size, 19); | |||
| assert.equal(result.pessimisticEstimate.nodeTimings.size, 79); | |||
| assert.equal(result.pessimisticEstimate.nodeTimings.size, 80); | |||
There was a problem hiding this comment.
curious and very unexpected that the optimistic estimate already included this somehow but the pessimistic didn't...
can we double check this? might point to a bug we're both missing
There was a problem hiding this comment.
is it b/c cpu idle uses fmp like this?
static getEstimateFromSimulation(simulation, extras) {
const fmpTimeInMs = extras.optimistic
? extras.fmpResult.optimisticEstimate.timeInMs
: extras.fmpResult.pessimisticEstimate.timeInMs;
return {
timeInMs: LanternFirstCPUIdle.getFirstCPUIdleWindowStart(simulation.nodeTimings, fmpTimeInMs),
nodeTimings: simulation.nodeTimings,
};
}
There was a problem hiding this comment.
That doesn't change the size of the nodeTimings map though, so I don't think it could affect this.
There was a problem hiding this comment.
No idea how to debug this.
There was a problem hiding this comment.
Ah it's not that the optimistic estimate already had it, it's just that it still gets filtered out by the optimistic estimate.
lighthouse/lighthouse-core/computed/metrics/lantern-interactive.js
Lines 40 to 43 in 4367845
This is slightly suboptimal but OKish from the simulation perspective because we're flooring on the first paint metrics if we end up too early and if it's that short it's unlikely to have a large impact to the last long task.
There was a problem hiding this comment.
cool, thanks for looking!
Co-Authored-By: Patrick Hulce <[email protected]>
Fixes #9627
Lantern
master
pr