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

[DO NOT MERGE YET] Improve rendering performance by chunking layout #263

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

Conversation

mathiasbynens
Copy link
Member

@mathiasbynens mathiasbynens commented Oct 14, 2020

This patch adds content-visibility: auto as described here:

Demo:

Demo with deep link:

This is supposed to improve load-time performance by only requiring layout for the visible top-level emu-clauses where applicable, in browsers that support content-visibility. While recording traces to quantify the improvement I bumped into https://bugs.chromium.org/p/chromium/issues/detail?id=1138128, which we’ll probably want to have fixed upstream before landing this.

cc @una @surma @jakearchibald

@RReverser
Copy link

@mathiasbynens I see a bit of flickering when scrolling through the after.html demo, at least while it's loading - is this expected?

@RReverser
Copy link

Lol I completely broke it. Here I just scrolled around and stopped in a single position and I'm not doing anything anymore, but it just keeps flickering: https://drive.google.com/file/d/1DlZSMBaztjISBtetL3FCmj_w2IjVxrGa/view?usp=sharing

@mathiasbynens
Copy link
Member Author

@RReverser Not sure what you're referring to exactly. Some brief "white flashes" while scrolling are expected when you scroll to a new area that hasn't gone through layout yet, but "flickering" definitely sounds unexpected/bad. Could you please file a crbug? (I did file https://bugs.chromium.org/p/chromium/issues/detail?id=1138128 for the issue I described above.)

@RReverser
Copy link

Not sure what you're referring to exactly. Some brief "white flashes" while scrolling are expected when you scroll to a new area that hasn't gone through layout yet, but "flickering" definitely sounds unexpected/bad.

Yeah sorry, just updated with a video to demonstrate.

@mathiasbynens
Copy link
Member Author

@RReverser Oh wow, thanks for providing that video. I had not run into that! Please file a crbug!

@RReverser
Copy link

Done: https://bugs.chromium.org/p/chromium/issues/detail?id=1138233

@motss
Copy link

motss commented Oct 14, 2020

This greatly improves the rendering on Chrome Android on Pixel 3 XL. 👍

@mathiasbynens
Copy link
Member Author

The issue @RReverser reported is now addressed as part of this patch!

css/elements.css Outdated Show resolved Hide resolved
@bakkot
Copy link
Contributor

bakkot commented Oct 14, 2020

I notice that this makes the scrollbar jump around as you scroll, since the browser doesn't know how big the document actually is. I wonder if it would be practical to do better (not necessarily before landing this PR) - e.g. as part of the build we could render the document without this property, open it in a browser, compute the actual sizes for each section, and then write those down in the CSS. Still wouldn't be perfect, but it would be a lot better.

@ljharb
Copy link
Member

ljharb commented Oct 14, 2020

It seems bad if the scrollbar jumps around - bad enough to make the feature itself not worth it at all, imo. If it eventually (like, the 20-30s that loading would normally take) computed the correct scrollbar size, that seems reasonable, but the scrollbar is a progress bar, and a progress bar that jumps around randomly thus becomes useless.

@ljharb
Copy link
Member

ljharb commented Oct 14, 2020

In particular, if you drag the scroll thumb with the mouse, it stops being attached to the cursor as you scroll - it looks really broken, and I'm very confused how this is acceptable to Chrome itself from a UX perspective :-/

@syg
Copy link
Contributor

syg commented Oct 14, 2020

One thing we can do is to make content-visibility a mobile-only style to aid Android readers, where the scrollbar discontinuity is less jarring.

@mathiasbynens
Copy link
Member Author

Note that the scrollbar jumping around already happens on mobile platforms even without this patch. IMHO it's a small price to pay for the massive load time improvements.

e.g. as part of the build we could render the document without this property, open it in a browser, compute the actual sizes for each section, and then write those down in the CSS. Still wouldn't be perfect, but it would be a lot better.

The "actual sizes" depend on viewport width, whatever values we hardcode at build time still wouldn't be perfect indeed — we’d be optimizing for 1 particular viewport size.

css/elements.css Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Oct 15, 2020

Destroying the usefulness of the scrollbar on desktop does not seem a worthy price to pay to me. That mobile platforms already have broken scrollbars means those should be fixed - not that desktops should be treated to the same broken experience.

@bakkot
Copy link
Contributor

bakkot commented Oct 15, 2020

Note that the scrollbar jumping around already happens on mobile platforms even without this patch.

Hm, I can get that to happen occasionally on my Android phone (a Pixel 3), but it's way more dramatic after this patch. Before it's jumping by perhaps a couple percent of the height of the scrollbar; after it's jumping more like 30% - enough that I lose track of the thumb while scrolling. It makes the scrollbar pretty much unusable while scrolling on my phone, instead of being an approximation with a very small amount of jitter. I don't think the two behaviors are really comparable.

IMHO it's a small price to pay for the massive load time improvements.

I agree it might be worth it, but it does make scrolling really jarring, especially on desktop. Particularly the thing where, if you click and drag on the thumb and cross a section boundary, it jumps out from under your cursor. Breaking a part of the browser's UI seems like a pretty high cost to me.

The "actual sizes" depend on viewport width, whatever values we hardcode at build time still wouldn't be perfect indeed — we’d be optimizing for 1 particular viewport size.

True, but we could compute a few and interpolate with calc (assuming that's legal). It would be an improvement, at least.

@surma
Copy link
Member

surma commented Oct 15, 2020

(Drive by comment: You could even grab these heights via puppeteer at a bunch of different screen widths and set them accordingly with media queries)

@syg
Copy link
Contributor

syg commented Oct 15, 2020

(Drive by comment: You could even grab these heights via puppeteer at a bunch of different screen widths and set them accordingly with media queries)

I thought the problem was also that the ecma262 clauses are all very different sizes. What's the rough workaround there, bucket the clause classes by size?

@bakkot
Copy link
Contributor

bakkot commented Oct 15, 2020

I thought the problem was also that the ecma262 clauses are all very different sizes. What's the rough workaround there, bucket the clause classes by size?

If we're using automation to compute sizes, we can just assign a different intrinsic size to each individual clause.

@surma
Copy link
Member

surma commented Oct 16, 2020

I thought the problem was also that the ecma262 clauses are all very different sizes. What's the rough workaround there, bucket the clause classes by size?

I thought you’d set the intrinsic size per element via inline styles.

@bakkot
Copy link
Contributor

bakkot commented Oct 24, 2020

I talked to a few frontend devs of my acquaintance, all of whom expressed severe reservations about the damage to the scrollbar: people are used to large pages loading slowly, but pages where the scrollbar doesn't work as expected stand out as broken. As such, I'd be really hesitant to accept this as-is.

I'm still interested in exploring ways of making this work, such as those suggested above. But that said, I am not convinced this feature will see adoption in its current state: I suspect that most devs responsible for the large documents for which this is relevant will find the performance benefit not to be worth the cost of breaking part of the browser's UI. So I'm curious if Chrome or the CSS working group is looking into tweaking the design so that the benefit can be had without that cost, or if the expectation is that devs will need to try to patch over the scrollbar issues with client-side JS or other tricks.

@bakkot
Copy link
Contributor

bakkot commented Dec 5, 2020

In this post Alex Russell describes using an intersection observer to keep things marked as visible once they've ever been within the viewport, which could be an improvement - the scrollbar would still jump around while moving to new content, but at least if you scroll back to a part you've already been to it'll stay put.

Though it would come at the cost making resizing the page slow again once you'd loaded enough sections.

@mathiasbynens
Copy link
Member Author

(cc @slightlyoff given #263 (comment))

@slightlyoff
Copy link
Member

slightlyoff commented Dec 5, 2020

One option I've considered (but haven't implemented) is a placeholder element that grows in height as one accumulates visible sections. The IntersectionObserver would allow this to be modified after layout, preventing another reflow as you scroll, and letting you keep content-visibility: auto on the content itself. Window resizing is trickier, and if you were to grow the window in width, I presume you'd want to remove the scroll tentpole element and start over, but I think it's possible to make things feel stable post-resize. Maybe I'll play with this approach a little.

@slightlyoff
Copy link
Member

Ok, I've been playing with a solution for my blog that's better in the face of resizing and the like. Here's the rough code, where the document structure of the elements I'd like to make cheaply available is:

<html>
  <head>
    <!-- stuff -->
    <style>
      body > main > article {
        content-visibility: auto;
      }
    </style>
  </head>
  <body>
    <!-- stuff -->
    <main>
      <article>...</article>
      <article>...</article>
      <article>...</article>
      ...

And here's the bit of script that targets those articles for efficient painting:

<script type="module">
  let articles = Array.from(
    document.querySelectorAll("body > main > article")
  );

  let eqIsh = (a, b, fuzz=1) => {
    return (Math.abs(a - b) <= fuzz);
  };

  let rectNotEQ = (a, b, fuzz=2) => {
    return (!eqIsh(a.width, b.width, fuzz) ||
            !eqIsh(a.height, b.height, fuzz));
  };

  let added = new WeakMap();
  // Only call this when it's known cheap; post layout
  let setHeight = (el, rect=el.getBoundingClientRect()) => {
    let old = added.get(el);
    added.set(el, rect);
    // Set intrinsic size to prevent jumping on un-painting:
    //    https://drafts.csswg.org/css-sizing-4/#intrinsic-size-override
    if (!old || rectNotEQ(old, rect)) {
      el.attributeStyleMap.set(
        "contain-intrinsic-size",
        `${parseInt(rect.width)}px ${parseInt(rect.height)}px`
      );
    }
  };

  let iobs = new IntersectionObserver((entries, o) => {
      entries.forEach((entry) => {
        if (entry.intersectionRatio > 0) {
          setHeight(entry.target, entry.boundingClientRect);
        }
      });
    },
    { rootMargin: "50px 0px 100px 0px"}
  );

  let robs = new ResizeObserver((entries, o) => {
    entries.forEach((entry) => {
      if (entry.contentRect.height) {
        setHeight(entry.target, entry.contentRect);
      }
    });
  });

  articles.forEach((el) => {
    iobs.observe(el);
    robs.observe(el);
  });
</script>

Basically, this sets up intersection and resize observers to create "set aside" space on each of the elements in question. and updates that geometry on resize so that if/when they scroll offscreen, their contents won't continue to get layout/paint passes applied until they "snap back". Have verified that this works as intended in Chrome.

@ljharb
Copy link
Member

ljharb commented Dec 8, 2020

Is perhaps all this code something for which a standardized api for “efficiently paint this selector” might be warranted, since it seems the current solution in the PR requires a bit of effort to avoid footguns?

@slightlyoff
Copy link
Member

slightlyoff commented Dec 8, 2020

This is 50 non-comment lines, including the CSS, so while I think there might be space for a "please reserve the last seen space for this element" attribute value for contain-intrinsic-size, I'm sure it's something @tabatkins and @chrishtr are collecting examples of to evaluate. I anticipate many uses of these display locking features, and hopefully they'll use the popular patterns to figure out what to add next. In the mean time, this appears to work well and I'd love feedback to see if it makes things feel snappier on your document.

@bakkot
Copy link
Contributor

bakkot commented Dec 8, 2020

Thanks, @slightlyoff!

@mathiasbynens, would you be up for dropping that snippet into your demo page so we can see how it feels?

(Incidentally I suspect we'll end up wanting to put this in the ecma262-specific CSS, rather than ecmarkup itself, since it's aimed at larger documents rather than stuff like proposals, but we can keep iterating on it here for now.)

mathiasbynens added a commit to mathiasbynens/ecma262-layout-chunking that referenced this pull request Dec 8, 2020
mathiasbynens added a commit to mathiasbynens/ecma262-layout-chunking that referenced this pull request Dec 8, 2020
mathiasbynens added a commit to mathiasbynens/ecma262-layout-chunking that referenced this pull request Dec 8, 2020
@mathiasbynens
Copy link
Member Author

@slightlyoff Nice!

@mathiasbynens, would you be up for dropping that snippet into your demo page so we can see how it feels?

Done. In particular, check the deep link demo:

@syg
Copy link
Contributor

syg commented Dec 8, 2020

Hm, on Chrome 87 on Mac I don't get a scrollbar at all anymore in the after (https://mathiasbynens.github.io/ecma262-layout-chunking/after.html#sec-string.prototype.replaceall) version. Is that supposed to be the case?

@bakkot
Copy link
Contributor

bakkot commented Dec 8, 2020

On Chrome 86.0.4240.193 the infinite loading behavior still happens, and the scrollbar never shows up, and on Canary (89.0.4349.2) the page hangs outright, so I can't assess it properly. (The hangs on Canary seem to happen on the "before" page as well, so it's probably a bug in Chrome rather than your code.)

I'll wait a bit for a new version of Chrome, I guess.

@slightlyoff
Copy link
Member

slightlyoff commented Dec 8, 2020

There was at least one Chrome bug (on Android) discovered in the snippet I posted above, and so I've updated it in a blog post about this approach:

https://infrequently.org/2020/12/resize-resilient-deferred-rendering/

For Chrome bugs, tagging in @vmpstr (who I pinged out of band). On Cr87/Mac, the demo branch seems to be working as advertised for me (no hangs, and I see scrollbars). Will try other platforms.

Update: Looks good on Windows! Also confirmed that a great deal of layout work is being skipped when resizing, e.g. Trace shows original (lower left) and updated (upper right) documents being resized. Layouts go from ~360ms -> ~80ms.
chrome://tracing output of windows being resized

@mathiasbynens
Copy link
Member Author

mathiasbynens commented Dec 9, 2020

There was at least one Chrome bug (on Android) discovered in the snippet I posted above, and so I've updated it in a blog post about this approach

@slightlyoff Thanks — demo pages updated accordingly. The trace looks 🔥🔥🔥 !


On Chrome 86.0.4240.193 the infinite loading behavior still happens, and the scrollbar never shows up, and on Canary (89.0.4349.2) the page hangs outright, so I can't assess it properly. (The hangs on Canary seem to happen on the "before" page as well, so it's probably a bug in Chrome rather than your code.)

I'll wait a bit for a new version of Chrome, I guess.

@bakkot I tested on the exact same Canary version (89.0.4349.2) on macOS, with a clean browser profile, and couldn't reproduce any hangs on either page. Are you sure you're not accidentally using some browser extension that interferes with the result somehow?


Hm, on Chrome 87 on Mac I don't get a scrollbar at all anymore in the after (https://mathiasbynens.github.io/ecma262-layout-chunking/after.html#sec-string.prototype.replaceall) version. Is that supposed to be the case?

@syg That's definitely not the intention! I still get a scrollbar on 87.0.4280.88 on macOS. If you can reproduce this reliably, could you please file a crbug?

@bakkot
Copy link
Contributor

bakkot commented Dec 9, 2020

@bakkot I tested on the exact same Canary version (89.0.4349.2) on macOS, with a clean browser profile, and couldn't reproduce any hangs on either page. Are you sure you're not accidentally using some browser extension that interferes with the result somehow?

Yup, it's a completely clean profile. Opening the demo page pegs the CPU (in fact it pegs two cores) for at least a minute (at which point I just killed it).

I'm on macOS 10.14; maybe that's the diifference?

Screen Shot 2020-12-08 at 11 29 43 PM

@slightlyoff
Copy link
Member

@bakkot: can you perhaps copy/paste the full contents of chrome://version here if you're comfortable sharing that?

@bakkot
Copy link
Contributor

bakkot commented Dec 11, 2020

@slightlyoff Sure:

Google Chrome 89.0.4352.0 (Official Build) canary (x86_64)
Revision 06b48a5d4f933139737b0d981a6e34a580eaf21c-refs/branch-heads/4352@{#1}
OS macOS Version 10.14.6 (Build 18G6032)
JavaScript V8 8.9.129
User Agent Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/89.0.4352.0 Safari/537.36
Command Line /Applications/Google Chrome Canary.app/Contents/MacOS/Google Chrome Canary --flag-switches-begin --javascript-harmony --flag-switches-end
Executable Path /Applications/Google Chrome Canary.app/Contents/MacOS/Google Chrome Canary
Profile Path /Users/kevin/Library/Application Support/Google/Chrome Canary/Default
Variations 583720e7-d7c239a84085631-377be55a90a7075b-6eac6d016b16054-f23d1deab0f75187-48df420d91f8f623-f23d1dea59b6f412-cf05fe7d60d4b352-30821c4482b62ecf-e7d7ef40da89714-b419121e1d52c63e-ca7d8d8055c044d3-f23d1dea95b880ad-3f4a17df8ae424bf-62410d2b9d6a857b-af856b8d3c98d047-3f4a17df74f8fa8f-b947d8e64d2d969c-847083536025934e-3f4a17df4d936449-5419e7bd6d6d60a5-dec406248950ab95-f23d1dea1298fecf-404338159e604a08-ca7d8d8041e765a5-f23d1dea8e44abde-3f4a17dfdc742ded-dec406246f212d51-f23d1dea8f83697a-642f01308f000ce6-ca7d8d80dbf7a8af-dd20d31bf2cb61f-3f4a17df8470b833-673c39a2195288ce-f23d1deaa6baf2da-e24c5b3065570806-a70dca942cff698a-548d881a345a470a-58845760a582a1b8-ad75ce171d606bb5-3de9d90b247004c0-f23d1dea3042ad4b-a0e56f74e4a357e9-79b0fbfd10bea3af-bb72e3e93fd33f16-41ecb1b913200569-b4779eec676784ca-f66697fabebfb376-ad9079c72729b628-a048438e5252c71-ca7d8d806a116980-763f0222fab3c74d-3f4a17dff6456a4a-78f82395142e58d7-6f77aaeca168b3cd-f23d1dea3ee82fb3-3a447918fa6aa590-ca7d8d80c992f345-ca7d8d80f403643c-ca7d8d8028114f9b-629cd37d8155c77d-ca7d8d80d8692482-ca7d8d80a042f0b2-f23d1dea638e38ae-dfb2c5907a911e9f-ca7d8d8038fb2686-ca7d8d80bdbcc4a1-5f02e174f8870c0a-3f4a17dfe153f4cb-6cf79a792f990be4-10fdb82f1c1d6a98-3f4a17df3487aa71-26bbc51953c99a5a-3f4a17dfd3566fbd-eac2fe9dae85586a-3f4a17df4ea303a6-d5247c3a7048821f-ede3dfe41ec777c-ca7d8d80a5c209bc-ca7d8d805213bf1c-ca7d8d80f48aee36-3f4a17dfe4e7724d-3f4a17df2ce51440-f23d1dead6f4076c-29c53a54fff8ec0-f23d1dea58cac63b-95af196178063874-f23d1dead0ff70be-ee63ae02a375a87b-3f4a17df7760b5b2-6bb234491625732c-e0bf8cdd9ca8521a-993b9ffd39baae47-d63ec99c931c5f72-bf5a489397f5b53e-f23d1dea494d8760-52325d433ac60855-486e2a9c63dcb6a3-1185807ee706e746-4a95e4f1f296190c-f9f7acb54442aae2-4ad60575f690cf64-4ad60575ed1d377-e1cc0f1475f0f0a0-4ad60575e2b18481-3a9ae350e7e71889-e1cc0f14b1ceb06f-51f64ec2e1368496-3f4a17df27435f03-41ecb1b9a5fa7f3a-efad1e7fcddbdd4e-f23d1deadd4eb913-3f4a17dffb50494f-3d47f4f48db2e42f-d34b6455e31bb48-8924bcae3ed25c66-f23d1deadd82d379-75a79c4b1fadabdc-4a2705f0

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.

8 participants