fix: report proper first paint delay for blocking tags audits #1555
Conversation
|
|
||
| return { | ||
| url: URL.getDisplayName(item.tag.url), | ||
| totalKb: `${Math.round(item.transferSize / 1024)} KB`, |
There was a problem hiding this comment.
should we include the unit alongside the table value (what you have) or just in the header?
There was a problem hiding this comment.
we've been including it in each cell and header so far and I think it's fine to be a bit redundant for the sake of clarity
| }); | ||
| let startTime = Number.MAX_VALUE; | ||
| let endTime = 0; | ||
| startTime = filtered.reduce((t, item) => Math.min(t, item.startTime), startTime); |
There was a problem hiding this comment.
would this work:
let startTime = filtered.reduce((t, item) => Math.min(t, item.startTime), Number.MAX_VALUE);
There was a problem hiding this comment.
sure will, just looked weird above endTime but current looks weird too, I'll switch back
| label: `delayed first paint by ${item.spendTime}ms` | ||
| }; | ||
| }); | ||
| const startTime = filtered.reduce((t, item) => Math.min(t, item.startTime), Number.MAX_VALUE); |
There was a problem hiding this comment.
are these strictly overlapping or is it possible there are breaks between them?
There was a problem hiding this comment.
it's possible there are breaks, what does it impact if there are breaks?
There was a problem hiding this comment.
it's possible there are breaks, what does it impact if there are breaks?
If there are link elements blocking first paint, then the browser is off doing something else, then it gets back to parsing and finds more link elements blocking first paint, don't we only want to report the time due to the link elements?
There was a problem hiding this comment.
in other words, is it possible for there to be a large difference between the union of time spent with these blocking first paint vs the first startTime subtracted from the last endTime
There was a problem hiding this comment.
It's possible for that difference to be large (though in practice I suspect it to be small unless people love putting huge script in the head or using document.write all over the place in which case there are other audits for them and I'm happy to punish them a little more than necessary here :) ) given our numbers because these are all purely network times not parse or anything else that would have generated these large gaps, but the goal here is for there to be 0 items blocking first paint and as far as I'm concerned it makes more sense to report that the violations pushed out first paint to at least X ms than to report the share of network time they occupied.
There was a problem hiding this comment.
yeah, document.write is definitely one that throws a wrench in the works. Unfortunately we don't have any audits that try to quantify the downsides of it, just true/false is it used.
There was a problem hiding this comment.
FWIW, I wrote some quick code to calculate the blocking length from the union of these requests and I could find no page where the two numbers differed (no site had a break between overlapping requests). It's possible I didn't test enough bad sites, though :)
There was a problem hiding this comment.
phew! I can achieve it using a hypothetical combo of stylesheets returning a 500 (which Chrome will re-request!?) and HTML imports injected later with a delayed script just as the first blocking stylesheet comes in :) but glad that situation isn't common in practice haha
| return { | ||
| url: URL.getDisplayName(item.tag.url), | ||
| totalKb: `${Math.round(item.transferSize / 1024)} KB`, | ||
| totalMs: `${Math.round((item.endTime - startTime) * 1000)}ms` |
There was a problem hiding this comment.
should this be item.startTime for just this item?
There was a problem hiding this comment.
overall startTime IMO (really it should be just when the original request finished but first possible request seems good enough). Using the overall start then highlights the biggest offenders the right way, since these block first paint, we want developers to target the ones that pushed it out the farthest regardless of when the request was started by Chrome.
There was a problem hiding this comment.
Using the overall start then highlights the biggest offenders the right way, since these block first paint, we want developers to target the ones that pushed it out the farthest regardless of when the request was started by Chrome
Ah, I see now. So the idea is that the parser has seen these by the time the first request is sent, so even if it doesn't get around to requesting this item right away, it makes sense to punish for that full time? That makes sense for many cases it's of course not just that item's contribution to blocking time...e.g. if the browser is throttled on the number of connections to make and so a request gets pushed further out, eliminating that particular request won't speed up page by item.endTime - startTime.
OTOH, maybe with the change you're making to how the total blocked time is calculated, it's clear that while this individual request blocked first paint for this long, eliminating this request won't speed up first paint by that much (i.e. the individual delays are overlapping).
I might be out in the weeds here, though, so I'll defer to you all :) @paulirish for his opinion
|
@ebidel PTAL :) |
|
@brendankenny any more objections to approval or just waiting on @paulirish 's opinion? |
Switches the blocking tag audits to report the maximum delay of first paint rather than a sum of the individual delays. Moves us more in alignment with the actual impact these violations have on the UX.
Also removes duplicate URLs from showing up twice to be inline with our use of "resources" and switches to table formatter.
Before:
After:
