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

perf: memoize rendering of library #6622

Merged

Conversation

arnostpleskot
Copy link
Contributor

@arnostpleskot arnostpleskot commented May 25, 2023

The Target of this PR is to minimize the number of unnecessary renders of the items in the library and the library itself.

There is an issue, when the user clicks on the canvas, a random item is added into appState.selectedElementIds. This will cause a new render of the personal part of the library (but items are memorized) and the first item with selected content (if there is any).

This may cause some conflicts when #6621 is merged.

@arnostpleskot arnostpleskot requested a review from dwelle May 25, 2023 10:20
@arnostpleskot arnostpleskot self-assigned this May 25, 2023
@vercel
Copy link

vercel bot commented May 25, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
excalidraw ✅ Ready (Inspect) Visit Preview May 31, 2023 1:25pm
excalidraw-package-example ✅ Ready (Inspect) Visit Preview May 31, 2023 1:25pm
1 Ignored Deployment
Name Status Preview Updated (UTC)
docs ⬜️ Ignored (Inspect) May 31, 2023 1:25pm

@dwelle
Copy link
Member

dwelle commented May 28, 2023

It's getting pretty complicated, and I wonder if all the memoization will not make it actually slower.

@arnostpleskot did you profile by any chance with a 100+ items, between this PR and master? Is there any significant difference in either direction?

Also, as per our previous discussion about the pending elements toggling changing the row count which results in rerender of the whole personal library section. I'm wondering, do you think we can leave the rows wrapping on the browser, while also keeping the sequential rows rendering optimization? E.g., make the <LibraryRow> not render a <Stack.*> components, but just the <LibraryUnit> directly, which would sit in a single section container?

@arnostpleskot
Copy link
Contributor Author

Yeah, there are a lot of things that are happening, and part of it depends on the app state. One of the problems is that LibraryUnits uses different props based on their location in the library. And it is not feasible to pass all possible variations and memoize simultaneously, so it is quite a mess regarding where the callbacks and memos are defined. I'm open to any suggestions on how to clean it up...

Regarding the performance of rendering on the first render is, the version in this branch slightly faster - probably thanks to limiting logic in the LibraryUnit itself and passing memoized callbacks from top. But it is in units of ms on my machine. The main impact is noticeable perf improvement when working on Canvas while the library is open.

And removing <Stack.*> is definitively possible by using either CSS grid or Flex.

# Conflicts:
#	src/components/LibraryMenuItems.tsx
#	src/components/LibraryMenuSection.tsx
#	src/components/LibraryUnit.tsx
and make size an odd number which feels a better UX when loading
@dwelle
Copy link
Member

dwelle commented May 31, 2023

LGTM, thanks! 🚀

based on cache vs items size, so that installing new libs will use smaller batch size
@arnostpleskot arnostpleskot merged commit 253c5c7 into excalidraw:master May 31, 2023
7 checks passed
@arnostpleskot arnostpleskot deleted the library-memoization-perf branch May 31, 2023 13:37
alswl pushed a commit to alswl/excalidraw that referenced this pull request Nov 15, 2023
Co-authored-by: dwelle <luzar.david@gmail.com>
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.

None yet

2 participants