Lazy render block types in the inserter #33749
Conversation
youknowriad
added
[Feature] Inserter
|
Size Change: +566 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
youknowriad
force-pushed
the
perf/more-responsive-inserter
branch
from
229f166 to
b970a7c
Compare
|
@youknowriad Which inserter were you testing with? Opening the global inserter from the top toolbar, I see:
|
|
@gwwar the global one, I didn't see that personally but I might still have some issues there, I'll look tomorrow. |
Ideas
ProfilingSo to verify that this is a hotspot in trunk, I did see ~300-400ms for the click handler. Our tasks should ideally be split into <16ms chunks, so we don't drop frames. We can confirm this by running the profiler and toggling open the global inserter. If we dig into the call stack, we see that we go into React internals suggesting that there's too many nodes to render. This is confirmed when we profile in the react profiler. Some larger chunks I spotted in the react profiler look like they're coming from icons and enabling drag.
Also of note is that there's a layout shift for the skeleton + 3 animations that fire here. These won't be the cause of poor performance here, but we're adding some extra calculations on top of a slow spot. Might end up being a no-op, but these stood out.
To see what animations are fired, we can use the animations tab: animations.on.open.mp4 |
Yes, this is kind of similar to what this PR does but with some subtle differences:
|
Yep, It does seem that there are components that are a bit slow to render maybe, the reason wasn't obvious to me while tracking but it something we can explore further. |
|
@gwwar I'm not able to reproduce your error above btw, do you think you can try to debug, or is there any instructions for me to try? |
|
It seems there's a browser specific issue, everything is working for me in safari but not in chrome 👀 |
|
I think the Chrome rendering issues are now fixed with the last commit. |
|
The inserter opening metric is three times quicker in this branch
|
|
I'm thinking of doing the same for search results on a separate PR. Right now, sometimes when typing the first characters in the search input, you do notice a small lag. |
There was a problem hiding this comment.
@youknowriad thanks for adding in the Chrome fix!
Do you mind checking the following in the profiler? I think I'm seeing the useAsyncList continue to queue up work after a single inserter open. In the videos below I left the react profiler highlighting on to demonstrate the issue.
| trunk - see how it idles after open | branch - why are we still doing work? |
|---|---|
 |
 |
How I'm profiling in trunk:
trunk.profiler.mp4
How I'm profiling in the branch:
branch.continues.to.do.work.mp4
The current PR lazy renders the list categories, one by one, without requiring scrolling
Right, I have no problems with breaking up the work. That makes a lot of sense when we have too much of it, I'm more curious about why it is as slow as it is with not a lot of items. It feels like it shouldn't be this slow for what looks like an icon and text. I'd be curious to 🔍 further investigate in follow up PRs:
- Why does it take so long to render a block type? Can we speed this up? (It looks like drag support + svg icons adds around 100ms, but there might be more to find for example, maybe we're using bad index keys for the list).
- Break up work in a sensible way. (this PR is one way of splitting this up and I think it's a good strategy to start with)
- With plugins do the number of items we need to render here grow unbounded? No matter how fast we can make a block type item render, if the number of types can grow to arbitrary sizes we'll eventually need/want to limit the number of items we should render. (windowing, just showing the top X, etc.)
| // Ideally, we shouldn't use a timeout and instead check the browser is idle for | ||
| // a specific duration, but didn't manage to find a simple way to do that. | ||
| // eslint-disable-next-line no-restricted-syntax | ||
| await page.waitFor( 500 ); |
There was a problem hiding this comment.
This is likely to be fragile. Perhaps split up the helper to fetch tiles for a specific section, or wait for all expected categories to appear instead of a wait.
There was a problem hiding this comment.
wait for all expected categories to appear instead of a wait.
This is exactly what I want to do here, but I didn't find a way to do it without introducing test specific code in the production code which I'm not a fan of.
Yes, that's intended, the browser stays responsive while the rendering of the remaining categories continues after the inserter is opened.
I can't tell but it's not 100ms for a single icon and drag support, it's for all the block types and there's a big number of them. |
I'm still seeing the browser do work after the inserter is fully rendered. |
oh got it, that's not correct indeed, I'll check. |
|
It turns out the |
There was a problem hiding this comment.
Giving tentative approval, let's wait for E2Es to be green. Some E2E specs may become more fragile from this change, but I think that's fine to address in follow ups provided that there aren't too many that are failing randomly.
This is a nice responsive speedup for the user, ~300ms -> 100 ms chunks on my machine. I think there's likely some more gains in shaving down the block type render, and splitting up work further so we can make sure tasks are closer to <16ms, but we can 🔍 in follow ups.
I'd also be curious to see if we have some research already with providing some a11y friendly ways of limiting what we render (like a simple load more button, or traditional pager).
✅ I also verified that I'm no longer seeing misc work firing after an inserter open.
Initial render now looks like 100-150ms chunks on my machine, with re-renders more around 90ms-100ms.
yes, I just saw one instance of this which I fixed. I'm happy to monitor and fix these.
We didn't do extensive research which is worth it. My previous limited research saw that infinite scrolling and "more button" is potentially possible but would rarely be an ideal solution https://www.digitala11y.com/infinite-scroll-accessibility-is-it-any-good/ (initially we were thinking about virtual scrolling too for block content) |
Opening the inserter is very a slow operation, it takes approximatively 360ms according to our metrics https://codehealth-riad.vercel.app and the size of the post don't have an impact there meaning on slow machines, folks could potentially notice a small lag when clicking the "open inserter" button.
I tried to debug this using the Chrome tools, and I noticed that the biggest part of that time is spent on rendering the block types, there's just a lot of block types and the number grows with third-party blocks.
This PR uses a technique we previously used for the patterns list to make the inserter feel more responsive: render the inserter categories one by one, and then let the browser do more priority things (like actually opening the inserter) before continuing
Trace before this PR:
Trace after the PR
Notice that now the trace is split into smaller chunks which allows the browser to feel more responsive.