List View: improve block focus time by providing stable function references #35683
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…en during typing and focus tests
|
Size Change: -107 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
In the performance test post, focusing will re-render all ListViewBlock items. This takes around 900ms to process. This is due to the following props always having a new reference: Initial culprits in branch.js
After fixing those we see
|
… onDragEnd, onFocus
gwwar
changed the title
gwwar
changed the title
gwwar
changed the title
gwwar
added
[Feature] List View
|
Hmm looks like we'll need to continue chasing down items to memo: |
5 tasks
|
Okay looks like the full ListView re-renders:
|
|
|
Okay this makes sense. So when we click on a block, selectedIds update. We currently query this in the parent component in ListView. Since the parent (ListView) component renders all child components render too (which may or may not trigger any DOM changes). Branch currently needs selectedIds to handle things like if we should render in async mode or not, branch highlighting and if we should add an appender. I'm going to close this one out, but I'll open a smaller one that refactors path and removes terminatedLevels. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While experimenting with the windowing technique in #35230 we discovered some unexpected behavior with block focus time.
Focusing on a block in the main editor pane will re-render all ListViewBlock items. Depending on if list view is open or not, we can see some dramatic differences in how long it takes to resolve a focus event.
Proposed changes in this PR remove the unused
terminatedLevelsprop, refactors path to be a string instead of an array, and attempts to provide stable references by usinguseCallbackanduseMemoin key areas.To give us solid numbers on the proposed changes, I've also included changes in the performance tests to have list view open during
focusandtypingin addition to a new test that toggles the list view open and closed.Since this PR won't help with a first render, we may want to drop related performance test changes before landing.
Testing Instructions
CleanShot.2021-10-15.at.11.01.17.mp4
To verify that I've written the performance tests correctly we can verify visual changes with the following:
React Profiling Details
When focusing on a block we can see in the react profiler that we have some initial culprits in branch.js:
onClick,onToggleExpanded,terminatedLevelsandpath:After fixing those we see
onDragStart,onDragEnd,onFocusin ListViewBlockSelectButton