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

[3895] Memoize the explorer #3896

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

mcharfadi
Copy link
Contributor

No description provided.

@mcharfadi mcharfadi added this to the 2024.9.0 milestone Aug 28, 2024
@mcharfadi mcharfadi linked an issue Aug 28, 2024 that may be closed by this pull request
@mcharfadi mcharfadi force-pushed the mch/enh/memo_explorer branch 3 times, most recently from 68545b0 to 9f825dd Compare August 28, 2024 08:07
@mcharfadi
Copy link
Contributor Author

mcharfadi commented Aug 28, 2024

I had issues typing useRef since it's using scrollIntoViewIfNeeded
I removed scrollIntoViewIfNeeded (it's deprecated) since scrollIntoViewseems to be well supported now

https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView

@mcharfadi mcharfadi changed the title Memoize the explorer [3895] Memoize the explore Aug 28, 2024
@sbegaudeau sbegaudeau changed the title [3895] Memoize the explore [3895] Memoize the explorer Aug 28, 2024
@mcharfadi mcharfadi modified the milestones: 2024.9.0, 2024.11.0 Oct 4, 2024
@sbegaudeau
Copy link
Member

I'm not sure why but with this PR, the explorer feels way more slow, everything seems to be blinking.

@mcharfadi
Copy link
Contributor Author

mcharfadi commented Oct 15, 2024

The refDom.current.scrollIntoView behave differently.
But I also found that with the rebase, it seems the expand now has an issue. I don't know if you're referring to this.
Otherwise selecting from the diagram is noticeably faster on my end with the performance papaya.

@mcharfadi
Copy link
Contributor Author

So for the expand, it's missing some dependencies in the useCallBack.
For the scrollIntoView , I'm not sure, it seems to me that it was not working properly before and fixing that makes the interaction less smooth.

@mcharfadi
Copy link
Contributor Author

I confirm that with the PR #3875 , most optimizations I made in this PR will not work anymore.
For example the method onExpand now uses state.expanded and as such we can't memoize onExpand correctly now.
I'm not sure how to handle these newly added states while only rerendering the treeItems impacted by a change.

@mcharfadi
Copy link
Contributor Author

mcharfadi commented Oct 18, 2024

I pushed a prototype version that would restore the performance gains.
Please note that I have used Zustand as a quick way of prototyping a solution but on a concrete implementation we might either :

  1. useExternalStore to replicate the behaviour of Zustand without adding the library
  2. adopt our current partern with a context and a customHook (but in this case I'm not sure we could have the same behavior easily)

The main goal is to avoid having onExpand as a prop, but I also wrapped several component in memo
Here are the results when expanding a node on papaya with only the explorer opened.

image
This is on master, we can see that around 15 rerender took each around 100ms totaling more than 1.5s

image
This is with the updated PR, we have as much "big" rerender as we expect. The first one is due to the selection change context (and has been reduced from 150ms to 30ms due to the different memo). The second one is due to receiving the TreeEventPayload and is taking 200ms.

Also note, that I have not tried with tree item action contributions, and I have not tried with several Tree (this might require an instance of a store for each Tree)

I put the PR on draft, we could probably find another way to get back the performance gains, but since it was relatively quick to prototype, I wanted to share this possible solution.

Bug: #3895
Signed-off-by: Michaël Charfadi <[email protected]>
@mcharfadi mcharfadi marked this pull request as draft October 18, 2024 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memoizing the explorer
2 participants