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.

Sign up for GitHub

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

Jump to bottom

Resource hints are misprioritised in the reorderhead transformer #391

Open
schlessera opened this issue Feb 4, 2020 · 7 comments
Open

Resource hints are misprioritised in the reorderhead transformer #391

schlessera opened this issue Feb 4, 2020 · 7 comments

Comments

@schlessera
Copy link

schlessera commented Feb 4, 2020 •
edited
Loading

The reorderhead transformer puts the resource hints at the back of the head (see entry (8)):

// ReorderHead reorders the children of <head>. Specifically, it
// orders the <head> like so:
// (0) <meta charset> tag
// (1) <style amp-runtime> (inserted by ampruntimecss.go)
// (2) remaining <meta> tags (those other than <meta charset>)
// (3) AMP runtime .js <script> tag
// (4) AMP viewer runtime .js <script> tag
// (5) <script> tags that are render delaying
// (6) <script> tags for remaining extensions
// (7) <link> tag for favicons
// (8) <link> tag for resource hints
// (9) <link rel=stylesheet> tags before <style amp-custom>
// (10) <style amp-custom>
// (11) any other tags allowed in <head>
// (12) AMP boilerplate (first style amp-boilerplate, then noscript)

This means that resource hints that are provided to meet the performance guidelines in the Amp documentation will reordered to only appear after the actual download triggers they are meant to optimize.

I think that either the entry (8) should be brought up to appear after (0) in general, or resource hints should be split into two groups where the Amp-related ones are brought higher while the rest stay in (8).

cc @sebastianbenz
This was referenced Feb 4, 2020
Open
Merged
@twifkak
Copy link
Member

twifkak commented Feb 5, 2020

Good catch. I think this is not a problem for SXG, because amppkg adds preload headers, which should serve two functions:

  • hinting priority to the browser
  • hinting recursive subresource prefetch to the browser, when a site prefetches an SXG

Please correct me if that's wrong.

I guess it would a blocker for Optimizer adopting the amppkg transform library. Is that the goal? If so, I suspect that the splitting idea you suggest should solve the problem while being neutral for SXG. That said, I'd like to loop in @jridgewell for his expertise, given related work on http://b/129565737.

(A fallback solution is to add a new transformer and a new OPTIMIZER config that runs it after reorderhead.)

(Background for the SXG header decision at http://b/111845688.)

@jridgewell
Copy link
Contributor

@twifkak is right, all the resource hints are extracted into link headers.

@schlessera
Copy link
Author

If I understand correctly, the link headers would only apply in the case of an SXG prefetch?

So if the transformers are used outside of the SXG context, this would still be sub-optimal, and fixing it for the transformers would not have any negative effect on the SXG.

Are you open for a PR for splitting the resource hints into two groups with two different priorities, based on the host domain?

@twifkak
Copy link
Member

twifkak commented Feb 5, 2020

Yes, I'm open to such a PR. It shouldn't just be domain -- we'll want to limit by path, too.

Note that using the transformers for on-origin will likely require other changes that are not SXG-neutral (e.g. ability to disable subresource URL rewriting). If that's the goal, then I have more questions:

  • Should we start a separate OPTIMIZE config now, to begin encapsulating these differences?
  • Last I heard, the Go transforms can't be compiled into JS that well, and folks didn't want AMP optimizer to require Go be installed, so we'll need both ports. Is it okay if they drift a bit, or do we have a plan to maintain parity?

Just to dig into SXG a bit more, there are three cases:

  • User visits https://foo.example/ -- per our frontend recommendation and backend checks, this page should never run through the transforms or be signed -- the original AMP would be served, and this may contain the appropriate link tags or link headers, as they are considered valid AMP.
  • User visits https://foo-example.ampcache.example/wp/s/foo.example/ with an SXG-accepting browser -- it will include the Link: rel=preload headers.
  • User visits https://foo-example.ampcache.example/c/s/foo.example/ with an SXG-non-accepting browser -- the AMP cache will serve unsigned HTML (either extracted from the SXG it fetched from origin, or from a non-SXG fetch), and thus must reapply AMP transforms (in this case, adding the Link headers).

So, all of these cases involve link-rel-preloads, I think.

@sebastianbenz
Copy link

Should we start a separate OPTIMIZE config now, to begin encapsulating these differences?

+1

Last I heard, the Go transforms can't be compiled into JS that well, and folks didn't want AMP optimizer to require Go be installed, so we'll need both ports. Is it okay if they drift a bit, or do we have a plan to maintain parity?

I think we should try to maintain parity (in particular feature wise), but some drift is OK imo. For example, I've just added auto extension import and missing AMP tag injection to Optimizer which I'm not sure is something we'd need for the Go implementation (as these are mainly aimed towards easing CMS / framework integration).

@Gregable
Copy link
Member

Gregable commented Feb 13, 2020 •
edited
Loading

Correct me if I'm misunderstanding, but I think this is working as intended already, other than the amp.dev example documentation.

In the amp.dev example, we see:

<link rel="preload" as="script" href="https://cdn.ampproject.org/v0.js">
<link rel="preload" as="script" href="https://cdn.ampproject.org/v0/amp-experiment-0.1.js">
<script async src="https://cdn.ampproject.org/v0.js"></script>
<script async custom-element="amp-experiment" src="https://cdn.ampproject.org/v0/amp-experiment-0.1.js"></script>

Why is there a preload tag for a file that is then immediately loaded? What purpose does the link preload tag serve that the script tag does not? This is maybe the crux of my misunderstanding.

IIUC, the only reason we insert these preload tags for Signed Exchanges is as a hint to later code to hoist the preloads into HTTP headers on the unsigned portion of the signed exchange. They aren't for browser consumption, because once the browser is parsing the link tags, it's also parsing the script tags.

There are also potentially preloads for less critical resources, such as preloading an image or a font. Those have value even if not hoisted into headers, but they should be prioritized below the critical resources like v0.js, not ahead of them.

@twifkak
Copy link
Member

twifkak commented Feb 13, 2020

Why is there a preload tag for a file that is then immediately loaded?

It's a priority hint in the browser. See the devtools screenshots in the link that @schlessera provided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@twifkak @Gregable @schlessera @jridgewell @sebastianbenz