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

Suggest <link rel="preload"> #931

Closed
ebidel opened this issue Nov 12, 2016 · 20 comments · Fixed by #3450
Closed

Suggest <link rel="preload"> #931

ebidel opened this issue Nov 12, 2016 · 20 comments · Fixed by #3450

Comments

@ebidel
Copy link
Contributor

ebidel commented Nov 12, 2016

critical-ish assets which are requested later by the page

@samccone interested?

@samccone
Copy link
Contributor

Kk

@addyosmani
Copy link
Member

addyosmani commented Aug 23, 2017

Going to bucket this one under the new loading audits milestone if that's cool? :)

Feel free to riff on these.

Description: Critical resources that are discovered late can be preloaded to fetch them earlier.
Failure description: bundle.js was discovered late but fetching it earlier may have improved how quickly the page was interactive (1.5s)
Help text: Consider using <link rel=preload> to prioritize fetching late-discovered resources sooner

@wardpeet
Copy link
Collaborator

I guess critical-ish assets are assets that are not preloaded and are not requested by the document itself? So all critical assets that are not picked up by the preload scanner?

Should we consider H2 push as well? If so can devtools detect that it's h2 push?

@paulirish
Copy link
Member

I guess critical-ish assets are assets that are not preloaded and are not requested by the document itself? So all critical assets that are not picked up by the preload scanner?

yah this seems like a good place to start.
We'd have to see on a few sites what requests this would recommend preload for. To be more conservative we could start with only CRC depth = 2 requests.. (rather than depth > 2).

Should we consider H2 push as well? If so can devtools detect that it's h2 push?

No. h2 push requires a bunch of conversation that we don't want to mix up on this one. :)

@wardpeet
Copy link
Collaborator

Ok I've setup basic audit code for this issue. Don't focus on the report just on the urls I captured.
image

WIth the CRC approach I don't get all the requests that are necessary until TTI, it's only the critical ones but critical requests don't mean much as it discards low priority requests. The problem is that many of these async js files have a low priority.

What do we think of getting al scripts that are requested before TTI and see if they are preloaded and not requested by the document?

cc @addyosmani @paulirish

@paulirish
Copy link
Member

WIth the CRC approach I don't get all the requests that are necessary until TTI, it's only the critical ones but critical requests don't mean much as it discards low priority requests. The problem is that many of these async js files have a low priority.

What do we think of getting al scripts that are requested before TTI and see if they are preloaded and not requested by the document?

I think we should start more conservatively. Async JS files that are preloaded will introduce network contention against actual critical JS files. There is probably some opportunity in here, but it's gonna be too difficult to generalize and be correct.

Let's start with: Only consider critical requests. Only requests of CRC depth = 2.

This will deliver webfonts pretty often. It will also include scripts that other scripts request (like on my site). Seems good.

@wardpeet
Copy link
Collaborator

@paulirish so this would be ok?
image

I need to write unit tests and smoke tests and than it's good to go 😄

@paulirish
Copy link
Member

  • audit title: Preload key requests
  • Don't really need that debugString
  • On my site the one result should be the youtube iframe. So that's either CRC depth = 2 or depth = 1 depending on how you count. :)

@addyosmani
Copy link
Member

Focusing on at least CRC depth =2 sounds good. Are we initially limiting the preload audit to scripts? In case useful here are (ranked) how preload is used in the wild today:

  • Preloading self-hosted Web Fonts
  • Preloading late discovered scripts
  • Preloading image headers or brand logos
  • Preloading stylesheets

Thus is based on the last look I had through HA. For PWAs, preloading JS is a pattern becoming more common and I could see value in starting there initially.

@samccone
Copy link
Contributor

There is a lot of nuance when using link rel preload, as it is trivial to preload "incorrectly" thus causing double requests, and therefore resulting in worse performance.

I am wondering if link rel preload is mature enough to point people down this path at the current date.

@wardpeet
Copy link
Collaborator

wardpeet commented Oct 1, 2017

I doesn't have to be scripts only. I've fixed the audit now on paulirish.com it has nothing to preload as the youtube video is a preload

@patrickhulce
Copy link
Collaborator

I somewhat agree with @samccone here. This and preconnect audit is the first area where following our performance opportunity recommendations may actually hurt another aspect of performance.

Is there an even more limited subset than depth 2 that'd be bulletproof? CRC sweeps all the way down to Medium priority here and is a rather wide net.

@paulirish
Copy link
Member

My proposal:

  • Included: non-async scripts, stylesheets, webfonts, html imports, in-viewport image, XHR/fetch
  • Excluded: async scripts, iframes, other images, video/audio, favicons

And because CRC depth=2, we'd only be recommending preload for the above included resourceTypes if their request was initiated by way of a primary script/stylesheet. (but not the HTML).

That feels fairly conservative to me already.

  • On theverge, it will recommend 7 webfont requests. Good.
  • On paulirish.com, it'll find nothing to recommend. Good.
  • on washingtonpost, it'll mark 1 stylesheet and 1 script, both pulled in for a media player. Seems OK.
  • on nytimes, it'll mark 1 css file requested by head.js that seems used for a modal dialog. Seems OK.

@patrickhulce
Copy link
Collaborator

That all sounds good.

I guess my concern is muddying the "Opportunities" section as just pure wins when the benefit is missing/negative for some situations (i.e. reporting that preloading a css file for hidden UI would save you X seconds on your load feels wrong). How do folks feel about surfacing this info in Diagnostics instead?

IMO, this changes the tone from "Loading these late was bad. Move them earlier for better performance!" to "These were loaded late and looked important. Do you want to preload them?"

@paulirish
Copy link
Member

Aye. It's reasonable for this to start in Diagnostics rather than Opportunties.

Generally, we won't be able to have the certainty to confirm a preload/preconnect is definitely a win, so it seems good to offer our knowledge to the dev and let them make a call. (Again, communicating the scoring is based on the metrics only).

There are some cases where preload is a sure win. (If a site loads gpt.js then pubads_impl.js will definitely be loaded.) But we'll probably have to hardcode those cases.

@addyosmani
Copy link
Member

Included: non-async scripts, stylesheets, webfonts, html imports, in-viewport image, XHR/fetch
Excluded: async scripts, iframes, other images, video/audio, favicons

This is a pretty reasonable list of includes imo.

I guess my concern is muddying the "Opportunities" section as just pure wins when the benefit is missing/negative for some situations

I'm pretty favorable towards including preload recommendations in Diagnostics to start off with. Reading back through the concerns @patrickhulce and @samccone had, I agree there's definitely potential for us to make a suggestion that isn't guaranteed to actually improve perf.

Presenting preload as an optional addition you might want to apply vs something you 100% should apply makes sense as we're unable to determine that it would definitely improve things every time.

@wardpeet
Copy link
Collaborator

wardpeet commented Oct 3, 2017

🤔 thought the preload didn't have many drawbacks 😛

I'll start implement what @paulirish suggested.
I don't think it's a bad idea to suggest fixed solutions like gpt to help optimize

@paulirish
Copy link
Member

Key for this one and the preconnect:

  • Clarify that this is not scored. This is just diagnostic info about the page.
  • Please don't preload all 10+ items listed in this table. only the ones that make sense

@CodeZeno
Copy link

CodeZeno commented Oct 6, 2019

I get the message "Consider using to prioritize fetching resources that are currently requested later in page load." for a resource that is already using this.
<link rel="preload" href="https://mywebsite.com/wp-content/plugins/wonderplugin-carousel/engine/icons/css/fontello.css" as="style">

@patrickhulce
Copy link
Collaborator

@CodeZeno please file a separate issue with repro steps if you believe you've found a bug you'd like addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants