core(lantern): fallback to FCP in 0-weight SI situations #11174
Conversation
patrickhulce
changed the title
| const totalWeightedTime = layoutWeights | ||
| .map(evt => evt.weight * Math.max(evt.time, fcpTimeInMs)) | ||
| .reduce((a, b) => a + b, 0); | ||
| const totalWeight = layoutWeights.map(evt => evt.weight).reduce((a, b) => a + b, 0); | ||
|
|
||
| if (!totalWeight) return fcpTimeInMs; |
There was a problem hiding this comment.
if layoutWeights.length === 0 then totalWeight will still be 0
| @@ -23,7 +25,46 @@ describe('Metrics: Speed Index', () => { | |||
| timing: Math.round(result.timing), | |||
| optimistic: Math.round(result.optimisticEstimate.timeInMs), | |||
| pessimistic: Math.round(result.pessimisticEstimate.timeInMs), | |||
| }).toMatchSnapshot(); | |||
| }).toMatchInlineSnapshot(` | |||
| Object { | |||
There was a problem hiding this comment.
we initially committed this snapshot before jest supported inline snapshots, which are greatly preferred for objects of this size to make the expectations more obvious and easier to notice
There was a problem hiding this comment.
aside: did jest mess with the format of the rest of the file? If so, could you try updating to the latest jest and see if they've fixed it? I know it was on their radar.
There was a problem hiding this comment.
prettier's API doesn't support it yet prettier/prettier#5807
it only affected the 1 line that would be updated on any future -u so didn't seem like a big deal 🤷♂️
There was a problem hiding this comment.
darn, still not done.
it's a much larger annoyance for other files, requiring you to either change dozens of other lines to prettier's standards, or copy and pasting the result -u, revert everything, and individually applying the new snapshot.
There was a problem hiding this comment.
It is, and I'd obvs love to adopt prettier even if it's only in our test files (looks around to see if @brendankenny is hiding in the tall grass), but some git checkout -p to clean it up definitely beats manually updating all the snapshot expectations :)
There was a problem hiding this comment.
nice, didn't know checkout could do patches.
There was a problem hiding this comment.
looks around to see if @brendankenny is hiding in the tall grass
🌾🌿 ⚔️ 🌿🌾
| @@ -10,6 +10,8 @@ const assert = require('assert').strict; | |||
| const SpeedIndex = require('../../../computed/metrics/speed-index.js'); | |||
| const trace = require('../../fixtures/traces/progressive-app-m60.json'); | |||
| const devtoolsLog = require('../../fixtures/traces/progressive-app-m60.devtools.log.json'); | |||
| const traceNaN = require('../../fixtures/traces/speedindex-nan-m84.trace.json'); | |||
| const devtoolsLogNaN = require('../../fixtures/traces/speedindex-nan-m84.devtoolslog.json'); | |||
There was a problem hiding this comment.
is there an alternate name for the trace/devtoolslog that says what the nan refers to? zero-ms-layouts or something?
There was a problem hiding this comment.
speedindex-1ms-layout sgtm 👍
| Object { | ||
| "optimistic": 575, | ||
| "pessimistic": 563, | ||
| "timing": 599, |
There was a problem hiding this comment.
is it worth asserting these are equal to the fcp estimates or should that be a non-guaranteed implementation detail and/or not worth adding any extra piping to be able to assert it?
There was a problem hiding this comment.
IMO, not worth it. conceptually speed index could be lower than FCP due to background color painting so if we ever decided to try to match that it wouldn't be an assertion that holds up.
| const totalWeightedTime = layoutWeights | ||
| .map(evt => evt.weight * Math.max(evt.time, fcpTimeInMs)) | ||
| .reduce((a, b) => a + b, 0); | ||
| const totalWeight = layoutWeights.map(evt => evt.weight).reduce((a, b) => a + b, 0); | ||
|
|
||
| if (!totalWeight) return fcpTimeInMs; | ||
| return totalWeightedTime / totalWeight; |
There was a problem hiding this comment.
it's hard to get my brain around possible values here without looking at a bunch of examples, but is it possible to get a weird discontinuity where adding a single layout task could give a result lower than fcpTimeInMs?
IOW, should this line be return Math.max(totalWeightedTime / totalWeight, fcpTimeInMs); or is that implicitly handled?
(this isn't something new, handling the division by zero case is 👍 )
There was a problem hiding this comment.
is it possible to get a weird discontinuity where adding a single layout task could give a result lower than fcpTimeInMs
no the .map(evt => evt.weight * Math.max(evt.time, fcpTimeInMs)) above should prevent any time earlier than fcpTimeInMs from being used in the weight
Summary
I was finally able to reproduce #11154 and found the issue. In some circumstances in DevTools only under desktop throttling (i.e. no CPU throttling), we end up with no consequential layout events (this trace has a single layout event that lasted 1ms). Because we use the log magnitude, log 1 is 0 which can't be a denominator :)
Related Issues/PRs
fixes #11154