Table block: Improve table block typing performance#74029
Merged
Conversation
talldan
requested review from
aaronrobertshaw,
ramonjd,
t-hamano and
tellthemachines
|
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. |
talldan
commented
Dec 16, 2025
| // Cell components are memoized, so it's best to avoid | ||
| // passing in a value that will cause all cells to re-render | ||
| // whenever it changes. | ||
| selectedCell === 'text' ? onChange : undefined |
Contributor
Author
There was a problem hiding this comment.
Oh wait, selectedCell === 'text' isn't right, this still TODO 🤦
Contributor
Author
There was a problem hiding this comment.
Ok I fixed that, and then performance is back to being bad again.
The problem is rich text re-renders whenever a block updates, from what I can tell that's because it uses BlockContext (or maybe BlockEdit context), which contains the attributes.
I guess generally most block only have one or two RichText components. Lots of RichText components in one block isn't as optimized. 🤔
|
Size Change: +82 B (0%) Total Size: 2.58 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in a236970. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/20261173789
|
talldan
removed request for
aaronrobertshaw,
ramonjd,
t-hamano and
tellthemachines
talldan
changed the title
Contributor
Author
|
I think it's not worth reviewing this right now, I accidentally made a mistake in the code (mentioned in a comment above). The general premise I think is good, but it turns out RichText re-renders every cell anyway on changes even if the Table Block itself is optimized. |
… does not need it" This reverts commit 05ba032.
Contributor
Author
|
Ok, I think I've fixed all the issues now. Sorry for switching the PR back and forth between draft and ready for review a few times. The two problems causing performance issues I found:
|
talldan
changed the title
Contributor
This appears to be an intentional change to avoid rich text binding subscriptions and fix the metric inflation: #72496 However, performance testing shows that restoring subscriptions does not significantly impact performance 🤔 
|
Contributor
|
The significant part of #72496 was adding an early return on the |
Contributor
aaronrobertshaw
left a comment
aaronrobertshaw
left a comment
There was a problem hiding this comment.
Thanks for all the work here @talldan 👍
I've taken this for a test spin and it works well. Definitely reduces the rerenders as advertised.
I'm not super familiar with recent work in this area, so I'll defer to others on the implementation details, but from a testing perspective this looks good to me.
| Before | After |
|---|---|
Screen.Recording.2025-12-16.at.1.16.41.pm.mp4 |
Screen.Recording.2025-12-16.at.1.15.06.pm.mp4 |
Member
Just wanted to add the same. Cells aren't being re-rendered on every update to a single cell (like on trunk), according to my React tools. ✅ I tested the following:
LGTM I can give it a ✅ if other folks are okay with it. Thanks a lot, @talldan ! |
ramonjd
approved these changes
Dec 16, 2025
Contributor
Author
Interesting. I'll definitely keep an eye on codevitals when this is merged to see if there were any performance regressions. |
Mamaduka
reviewed
Dec 17, 2025
Comment on lines
+176
to
+187
| setAttributes( | ||
| updateSelectedCell( | ||
| attributes, | ||
| selectedCell, | ||
| ( cellAttributes ) => ( { | ||
| ...cellAttributes, | ||
| content, | ||
| } ) | ||
| ) | ||
| ); | ||
| }, | ||
| [ attributes, selectedCell, setAttributes ] |
Member
There was a problem hiding this comment.
We can use the updater function here, which eliminates the need to pass attributes as dependencies. Should make memoization more "stable". Ref: #69709.
Mamaduka
reviewed
Dec 17, 2025
Comment on lines
+168
to
+169
| bindableAttributes: | ||
| __experimentalBlockBindingsSupportedAttributes?.[ blockName ], |
Member
There was a problem hiding this comment.
I think this introduces a regression. The bindableAttributes attributes are now returned only for a selected block, but IIRC, these values are needed in other cases as well.
Contributor
Author
There was a problem hiding this comment.
Hmm, good catch, I didn't spot the early return. I guess the bindingLabel / placeholder may need it.
Contributor
Author
There was a problem hiding this comment.
PR here - Fix bindableAttributes only being returned for the selected block.
Sorry, I should've pinged more folks (block bindings contributors) on this PR.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
What?
Fixes the typing performance part of #30091. I still see quite slow performance when creating big tables, so that can be a separate perf challenge.
As noted in that issue, for large tables typing performance degrades significantly.
@t-hamano kindly diagnosed one of the problems in that issue - all cells are re-rendered whenever a change is made to a single cell, this can involve quite a large number of cells and RichText components.
After solving that, there were still some issues causing every RichText in the table to update at the same time. I narrowed this down usage of
PrivateBlockContextin Rich Text.How?
I've extracted Cell into a separate component that's memoized.
Memoization seems like a good solution to me, as the majority of the elements in the table don't change, only one cell does at a time.
For the PrivateBlockContext issue - this is an unmemoized context provider, and seems to update on any change to a block. I've removed that usage and switched to getting bindableAttributes via RichText's own useSelect call. After that change, only the RichText being typed into updates.
Testing Instructions
One way to test this is to use the React Developer Tools, from the options menu toggle on 'Highlight updates when components render'. Then compare trunk to this PR.
In this PR: Only the selected cell (and the deselected cell) re-render
In trunk: Every cell re-renders whenever changing selection or typing 😬
Screenshots or screencast
Before
Kapture.2025-12-16.at.15.50.46.mp4
After
Kapture.2025-12-16.at.15.48.39.mp4