Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preload suggestions are too aggressive #11960

Open
patrickhulce opened this issue Jan 15, 2021 · 14 comments
Open

Preload suggestions are too aggressive #11960

patrickhulce opened this issue Jan 15, 2021 · 14 comments
Assignees
Labels
breaking needs-complete-audit-proposal https://github.com/GoogleChrome/lighthouse/blob/master/docs/new-audits.md needs-investigation P1.5

Comments

@patrickhulce
Copy link
Collaborator

patrickhulce commented Jan 15, 2021

I recently came across several partners who had applied preload too aggressively in ways that were making every performance metric worse, and when asked why, they shared Lighthouse reports that told them to do so. I think there are a couple culprits in Lighthouse where the preload advice is too aggressive and we should scale back our preload recommendations.

Examples:

  • Recommending to preload an optional font so that it's used is bad advice for mobile connections that won't end up using it anyway. Given how contentious this audit was in the first place, it seems clear to me that this is more of a case-by-case basis recommendation and shouldn't be a blanket recommendation on mobile. Proposed action: Mark this audit not applicable on mobile (I might even want an audit that says "DONT'T preload an optional font on mobile")
  • Preload of swap fonts. A particular site was preloading five different webfonts because Lighthouse told them to which resulted is a massive ugly tradeoff in 2s LCP delay (due to contention with the render blocking stylesheet) in exchange for like .01 CLS improvement. Proposed action: don't suggest preloading fonts with a non-block font display when throttling method is simulate (in applied throttling Chrome will have already rendered), tbd language changes that the preload audit isn't meant to be followed blindly, it's a "pick the important 1 or 2" situation
  • Maybe more...
@connorjclark
Copy link
Collaborator

Yes, preload is often a footgun. Should we do something like #9903 ?

@patrickhulce
Copy link
Collaborator Author

Oh I remember my third now :) I want something even stronger than that. I want an audit that fails too many preloads at once on mobile.

@paulirish
Copy link
Member

paulirish commented Feb 8, 2021

Other ideas from discussion:

  • We should be more conservative in adding URLs to our results as many users have a tendency to just mass-add preloads.
  • We can also establish a max # of recommended preloads
  • Don't suggest preload if there's already a lot of early network contention.
  • We have 3 preload oriented audits. They can share some audit logic so we have aligned recommendations.

Later: we can add an audit around reduce-noncritical-preloads because folks have already over-preloaded.

Much later: going the opposite direction: are we missing any good preload candidates? "critical" XHRs?


Also some discussion on preconnect: perhaps we show all the origins that could be preconnected and tell you to preconnect 2 of them, leaving the onus of choice on them. filed as #12066

@dvelasquez
Copy link
Contributor

@patrickhulce I can think of a 4th case:

  • Don't recommend use preload to scripts loaded with async.

I first noticed this on a NextJS application. NextJS by default adds a <link rel="preload"> to all NextJS chunks in the head of the page. Then it adds all the <script src="..." async> at the bottom of the body.

If the page is big enough (a lot of html, a lot of css or a combination of both), several of the preloaded scripts are already downloaded and loaded when the browser reach the bottom of the page, then triggering the execution of those scripts right away, deferring the FCP and LCP.

There is this misconception of that script loaded with async are in effect asynchronous, so people think that they are not render blocking, but at least in Chrome, async means that the script will be downloaded in parallel, and once is available, it will block the main thread and load the script.

This is a graph showing FCP before/after we removed the <link rel="preload> from the head:
image

We fixed this by tweaking the code for NextJS, you can see the discussion here

After that I've found several sites that has been seen their performance decrease by preloading too much (almost all) of their content. And if you preload everything, you are kind of not preloading anything.

@patrickhulce
Copy link
Collaborator Author

Thanks for the extra case @dvelasquez! I'm pretty sure Lighthouse would never suggest the developer to preload that case today because it's already discoverable from the main document. A separate audit telling devs not to preload something they're already preloading is definitely up for discussion though!

After that I've found several sites that has been seen their performance decrease by preloading too much (almost all) of their content. And if you preload everything, you are kind of not preloading anything.

Yes exactly! That's the situation we're trying to discourage here 👍 Thanks for linking in that next.js discussion too we'll have to sync up with the outcome there

@dvelasquez
Copy link
Contributor

@patrickhulce Would make sense an audit like:

Avoid preload scripts that are intended to load as asynchronous

  • The script asdf.js is being preloaded with <link rel="preload"> but loaded with <script async>
  • The script qwerty.js is being preloaded with <link rel="preload"> but loaded with <script async>

@patrickhulce
Copy link
Collaborator Author

I'm not sure that advice can be so broad as to always apply, which is exactly what we're trying to back away from here, even if it's absolutely true the situation you ran into. AFAIK, some of these preload issues with FCP are actually bugs in preload that are being addressed on the browser-side (see https://bugs.chromium.org/p/chromium/issues/detail?id=788757)

If for example there's an app shell with a single async script that renders the primary content and the FCP issue is resolved browser side, I could potentially see preload being used to increase the priority of that script compared to others.

@dvelasquez
Copy link
Contributor

@patrickhulce Thanks for sharing this. I didn't knew it was a bug, I will follow this closely :)

@midzer
Copy link
Contributor

midzer commented May 16, 2021

Using LH 7.4.0, in one of my sites "Fonts with font-display: optional are not preloaded" complains about the woff although I already preload the woff2 correctly.

Maybe we can consider this as well: if the right/best font is already preloaded, LH neglects the others in the same @font-face.

@patrickhulce
Copy link
Collaborator Author

More discussion offline, the current plan here is:

  • Move all preload audits to not applicable until the preload bug fix is validated. Once validated...
  • Introduce an audit that flags any use of more than N fonts. (N=4?)
  • Flag late discovered, render-blocking fonts only when there are 2 or fewer (otherwise this is not applicable and only other audit will show), recommend either preload or inline of discovery in docs but emphasis is on moving discovery earlier, not preload specifically.
  • Remove the old preload advice in v9.

@patrickhulce
Copy link
Collaborator Author

patrickhulce commented Sep 28, 2021

Update here after syncing up with @pmeenan

  1. Preload is safe to use after Chrome 95 (will not skip queue), other browsers are already good here
  2. Order still matters a lot, don't put lower-pri preloads in headers or first in the head compared to other critical resources (we should update docs here to make this clear)
  3. General consensus that using preload for important resources that are not discoverable from the parser (i.e. our current audits) is a good thing
  4. Still need to adjust our approach in existing audits before enabling to not blanketly recommend preloading all of them
  5. Possibly consider adding an audit that says do NOT use headers or preload too many

@connorjclark
Copy link
Collaborator

Coming under the wire for 9.0, for now let's just mark this audit as group: 'hidden' (or just do nothing, depends on how #13241 / a followup).

@adamraine
Copy link
Member

TODO: Discuss reenabling preload-lcp-image

@paulirish
Copy link
Member

paulirish commented Jan 31, 2022

5. Possibly consider adding an audit that says do NOT use headers

I asked pmeenan about this and he said...

The html tag lets you control when it is discovered and fetched. If you put it in the headers it will still jump ahead of css, js, etc just by virtue of being discovered first

and also

Not sure I'd preload the LCP image though unless it is a background image. Priority Hints are a better fit for regular <img> tags


issue status:

uses-rel-preload and preload-fonts are still disabled. we think they're too aggressive to just reenable. and we need to do some work to determine a new policy. and as pat indicates, we may want to adjust our preload-lcp-image audit to suggest priority hints above a preload


edit: WPT tweaked some unused-preload reporting stuff here: Add support for reporting unused preloads by pmeenan · #490 · wptagent

edit May 23 2022: We previously decided the next action here is for someone to author a design doc to capture the problem space and potential solution space. (There are several preload issues open right now). Phil also has some ideas that would be good to align on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking needs-complete-audit-proposal https://github.com/GoogleChrome/lighthouse/blob/master/docs/new-audits.md needs-investigation P1.5
Projects
None yet
Development

No branches or pull requests

9 participants