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

Prerender report pages #893

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

Prerender report pages #893

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e7b5982
prerender report pages
rviscomi Jun 29, 2024
f0634da
eager prefetch, moderate prerender
rviscomi Jul 1, 2024
c0a0372
list
rviscomi Jul 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/js/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,12 @@ export const drawMetricSummary = (options, client, value, isMedian=true, change=
* @param {Function} callback
*/
export function callOnceWhenVisible(element, callback) {
// If the document is prerendering, don't wait until the charts are visible.
if (document.prerendering) {
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 •
edited
Loading

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 •
edited
Loading

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.

new IntersectionObserver((entries, observer) => {
if (!entries[0].isIntersecting) {
return;
Expand Down
10 changes: 10 additions & 0 deletions templates/base.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,16 @@
{% block scripts %}
<script defer src="{{ get_versioned_filename('/static/js/web-vitals.js') }}" nonce="{{ csp_nonce() }}"></script>
<script defer src="{{ get_versioned_filename('/static/js/send-web-vitals.js') }}" nonce="{{ csp_nonce() }}"></script>
<script type="speculationrules" nonce="{{ csp_nonce() }}">
{
"prerender": [{
"urls": [
"/reports/state-of-the-web",
"/reports/page-weight",
"/reports/loading-speed"
]
}
</script>
{% endblock %}
</body>
</html>
Loading