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

Prerender report pages #893

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Prerender report pages #893

wants to merge 3 commits into from

Conversation

rviscomi
Copy link
Member

Uses Speculation Rules to eagerly prerender /reports/* pages

Bypasses lazy rendering of charts during prerendering

@rviscomi rviscomi requested a review from tunetheweb June 29, 2024 00:39
@tunetheweb
Copy link
Member

This seems way too heavy. We’re prerendering all 10 reports (which is the absolute max allowed btw!) even though the user is at best case going to get value out of only one of those.

AND you’re removing your lazy loading option to give them the FULL load cost. For the CrUX report alone that’s 1.3MB.

This is likely to cause a big performance regression as the current page load and INP competes for network and CPU resources—particularly on slow devices.

I also think it sets a really bad example. I’m telling people not to overdo this, to think of their users, and that the 10 limit is a guard against sites going way too far, and not a target.

WDYT about instead prefetching the reports eagerly? And then prerendering all internal links less eagerly (moderate eagerness)? With the prefetch that should give it enough head start to improve navigations without making them download (and render charts for) ALL the data we show. We can also <link rel=prefetch> the highcharts libraries to further reduce delays so the moderate eagerness gets best change to complete even in that short window.

@rviscomi
Copy link
Member Author

rviscomi commented Jul 1, 2024

Yeah I think you're right. I'll update.

callback();
return;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should remove this. Especially as not eager now so good change the document will not have fully loaded (especially on mobile) so can’t assume this is beneficial and will probably regress INP again (which is why you put this delay in IIRC).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow yeah, I just tested this and it does not at all work the way I expected: https://trace.cafe/t/mU5ULRNT6F (6x slowdown)

image image

I waited about 1500ms before clicking on the prerendered link, and I just happened to catch it when the charts started prerendering. I expected document.prerendering to be immediately set to false, so even if a few charts had started prerendering, the last one in-flight might block for only a short time and the rest of them would be rendered lazily as usual. What happened instead is all of the charts continued prerendering in a long task for more than 3 seconds, which completely blocked the report page from appearing. So the TTFB was instant, CLS was 0, but LCP was extremely slow. To make things worse, my first interaction on the page coincided with Wappalyzer starting up, causing slow INP too.

What's also weird is if I break up each chart prerender into its own distinct task, it behaves exactly the same way: https://trace.cafe/t/Itf3hC9zHo

image

It seems like document.prerendering only flips after the page is shown, but the page might be using that as a signal that more prerendering work can be done, which ironically ends up blocking the page anyway. Is that WAI from your perspective?

In any case, this seems like something worth warning developers about! Somewhere in the vicinity of this bit:

While this may make sense for analytics and similar use cases, in other cases you may want more content loaded for those cases, and so may want to use document.prerendering and prerenderingchange to specifically target prerendering pages.

Copy link
Member Author

@rviscomi rviscomi Jul 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, document.prerendering seems kinda like isInputPending in that it's an unreliable signal for "the user is not currently blocked". So does it ever make sense to do additional work during prerendering if there's no way to be sure that it's not blocking?

FWIW I also tried to yield with rAF and setTimeout values over 20 ms, but in each case the callback never fired by the time I clicked the link:

image

I know timeouts are throttled as an optimization for backgrounded tabs, but it's counter-productive if you're trying to do more work in a yieldy way during prerendering.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you break each one into it's own task? What code did you add, where?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  getFlagSeries()
    .then(flagSeries => series.push(flagSeries))
    // If the getFlagSeries request fails (503), catch so we can still draw the chart
    .catch(console.error)
    .then(_ => {
      const chart = document.getElementById(options.chartId);
      setTimeout(() => {
        callOnceWhenVisible(chart, () => drawChart(options, series));
      }, 0);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmm… not sure to be honest. The first one kinda makes sense if document.prendering was checked before activation (since all the network requests finished before then, and it also looks like Highcharts bundles multiple chart requests into one task) but the second one I can’t explain.

I wonder if activations tries to drain the task queue before scheduling the initial render? But that seems less than optimal!

It would be worth trying to produced a more minimal reproduction without highcharts in glitch or something and then raising a bug with the preloading team.

However, even without that, I think not using lazy loading if prerendering is still too much anyway due to the small lead time from moderate eagerness so unlikely to benefit and more likely to lead to race conditions (even if it shouldn’t be as bad as it appears to be from your testing). So I still say remove this code anyway regardless of whether this is WAI or not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I definitely agree that given the current implementation it doesn't make sense to eagerly prerender the charts. But I'm not sure I understand your reasoning to avoid making use of any idle time before the prerendered page is visible, assuming there was a reliable way to check if it's blocking. Is it that we might start prerendering a chart immediately before the link is clicked, so the user might be blocked for that ~300 ms? TBH that seems like a decent tradeoff, as that time would be attributed to the near-zero LCP anyway, not the INP of the click, and subsequent interactions wouldn't have to pay the costs to render the already prerendered charts.

I'll look into creating a repro for a bug report.

Copy link
Member

@tunetheweb tunetheweb Jul 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you could if there was a reliable way to detect this.

I initially didn’t understand the intent was to check this for each chart, and that each would be rendered serially, rather than as a once off. So would be good to make that clearer in the comments if keeping this.

I just think, particularly on mobile, where there is no hover event, the chances are slim of this allowing more charts to render and, on the downside, if a chart takes 500ms to render on a slow device for example and you click just after it’s started then you get a 499ms slower LCP than necessary. Now I guess you’re saying you have such a head start on LCP you think that’s a worthwhile trade off? Maybe, but I’d rather take the LCP gains. It’s not like it’s guaranteed to be 0ms LCP so that 500ms is the worst case. It could be the difference between 1second and 1.5seconds. Or worse—maybe between 2.1 seconds and 2.6 seconds and suddenly we’re failing LCP.

And even if it did render first (as I presumed it would to be honest) and then draw the chart, you’ve an INP risk instead.

And I argue if it’s good enough to lazy load for regular page loads, then it’s good enough for prerendered pages too.

So probably not something I’d have bothered implementing. I wouldn’t be totally against it though if it was reliable to check between each graph if you felt strongly about it. Maybe we should A/B test to see if net gain or not?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summarizing what we chatted about yesterday:

Eagerly prerendering a small number of report pages would help to minimize overhead and be much more likely to be impactful for mobile users. Here are the top 3 most popular reports, which account for about two thirds of report traffic:

  1. /reports/state-of-the-web
  2. /reports/page-weight
  3. /reports/loading-speed

We also talked about keeping the logic to skip lazily rendering the charts during preloading given that there will be a lot more idle time the sooner (more eager) the pages are prerendered.

Does that sound right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's give it a go.

Personally, I think prefetching highcharts js, plus prerendering on hover, and keeping the lazy loading, is maybe better overall, but agree it might not be impactful for mobile.

I do see two downsides of skipping lazy rendering during prerender:

  1. Could impact the current page more (especially on low-end devices).
  2. If prerendering the other 7 reports on hover, then they will also skip lazy loading, despite having the less lead time. Not clear from your comment whether you plan to skip that option for the other 7 reports?

But let's give your proposal a go and see if any impact on our CWVs. If we don't see a decent improvement on INP (or worse—a regression!) then can try something else.

<script type="speculationrules" nonce="{{ csp_nonce() }}">
{
"prefetch": [{
"source": "document",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically this is not needed as the where clause implies this, and it may be removed in future. There was one version of Chrome (121) where document rules was supported but needs this explicitly set, before we made it optional (in Chrome 122):

https://developer.chrome.com/blog/speculation-rules-improvements?hl=en#optional_source

Suggested change
"source": "document",

}],

"prerender": [{
"source": "document",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

Suggested change
"source": "document",

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

Successfully merging this pull request may close these issues.

2 participants