Memoize pattern objects returned from getAllowedPatterns #66159
Merged
+16
−8
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
jsnajdr
added
[Type] Bug
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
youknowriad
approved these changes
Oct 16, 2024
|
Size Change: +12 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
This getter-enhancing code has been moved to the selector only very recently, in #65700. Previously it lived in the |
|
There was a conflict while trying to cherry-pick the commit to the wp/6.7 branch. Please resolve the conflict manually and create a PR to the wp/6.7 branch. PRs to wp/6.7 are similar to PRs to trunk, but you should base your PR on the wp/6.7 branch instead of trunk. |
|
@jsnajdr Thank you very much for looking into this and resolving. It Looks like this didn't merge to |
|
My patch modifies code that was merged by @youknowriad in #65611 and #65700. And these PRs weren't backported to |
|
Manual backport proposed in #66162. |
kevin940726
reviewed
Oct 16, 2024
| @@ -2385,6 +2385,21 @@ const getAllowedPatternsDependants = ( select ) => ( state, rootClientId ) => [ | |||
| ...getInsertBlockTypeDependants( state, rootClientId ), | |||
| ]; | |||
|
|
|||
| const patternsWithParsedBlocks = new WeakMap(); | |||
There was a problem hiding this comment.
Maybe it's just me, but this type of manual optimization feels fragile to me. Why do we want to use a WeakMap to cache individual patterns rather than relying on composing cached selectors? Based on the implementation in getAllPatterns, we create a new array (and new items in the array) every time getAllPatternsDependants changes, breaking the memoization here. That is, every time the patterns list changes (new patterns, removed patterns, etc), we have to recompute all these anyway. The WeakMap doesn't really have a difference from a list-level selector. I have not tested this though so I could be very wrong here 😅. It just seems like we're aggressively doing memoization with different techniques without an idiomatic mental model (or that I'm lacking it 🤦). Memoization isn't free after all. I wonder if anyone else is feeling lost sometimes too 🤔.
There was a problem hiding this comment.
I wonder if anyone else is feeling lost sometimes too
Yes, I personally also feel lost all the time. Memoization is indeed extremely hard to understand 100%. It's probably the biggest downside of the Redux architecture and React in general. They depend a lot on exact identity and equality of objects, and the underlying JS language primitives don't have a good support for that.
Based on the implementation in
getAllPatterns, we create a new array (and new items in the array) every timegetAllPatternsDependantschanges, breaking the memoization here.
There is a difference between memoizing the entire array, and memoizing the individual items. Getting allowed patterns for different block IDs legitimately produces different arrays, but our issue was that even though the individual patterns are exactly the same, the pattern objects were not the same. Because each call of the selector creates a new object for each array items: adding the get blocks enhancement.
Why do we want to use a
WeakMapto cache individual patterns rather than relying on composing cached selectors?
A cached selector must have state as the first argument, it needs to select from the Redux state. But here we're working only with the pattern object, we don't have the state.
@jsnajdr usePatternState or in the selector like here is the same thing. I'm thinking more on a single pattern component that way we can just use useMemo. |
|
@youknowriad But that can work only if Currently I'm finding at least one other usage of |
|
I don't think sharing the memoized parsed patterns is really the bottleneck here. The issue was about re-rendering the inserter component, I don't think the initial rendering itself is a performance issue. |
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.
Fixes #65920. When switching between different blocks, the
getAllowedPatternsselector calculates a list of patterns insertable into that block. And that list is further rendered byuseAsyncList. WhenuseAsyncListgets a new but identical list, it should keep the existing items rendered and incrementally add the new ones. But the pattern items are not identical, because each newgetAllowedPatternscall creates a new pattern item, enhancing it with ablocksgetter. And that makesuseAsyncListrerender the list from scratch. The solution is to memoize the function that enhances the patterns.How to test: Try to reproduce the rerendering behavior that @annezazu reports in #65920 (see the video in the initial comment). It disappears after this patch is applied.