Data: Use deep equal instead of shallow equal for useSelect
#47419
Conversation
tyxla
added
[Type] Enhancement
tyxla
requested review from
adamziel,
Mamaduka,
youknowriad,
jsnajdr,
gziolo,
ellatrix,
flootr and
ntsekouras
|
@tyxla AFAIK shallow comparison was a deliberate choice. What is the performance impact of this change on the editor by and large? I understand it does save some re-rendering in certain cases, but surely it adds overhead when dealing with deeply nestes objects many times over. I’d love to learn more about the big picture effect of this change. Also cc @dmsnell who’s been working with performance tests lately and @youknowriad |
|
Size Change: +280 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
+1 to what @adamziel said. It's a recommended/good practice to return referentially stable objects inside hooks like
Would you happen to have a list of culprits? Let's fix those selectors where we can. |
|
Flaky tests detected in b75285c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4005157779
|
That generally makes sense to me, but For the protocol, I did remove the shallow comparison check in my testing, and that revealed a bunch of cases that are caught by that condition, effectively reducing the number of unnecessary updates. My suggestion only catches additional cases that are unnecessary updates, making the approach work on all levels and not just on the first one, resulting in less updates, and fewer unnecessary rerenders, which I expect to affect runtime performance positively, especially on slower machines.
I'd love to hear more about that. Anyone who can share more about it?
I've run the performance tests and didn't see any negative impact. Some of the stats actually appeared to improve, but I guess this can be statistical noise.
I'm happy to work on such a list if we end up going that route. But my quick testing revealed a bunch of them, indicating that there's likely a bigger issue to be solved at the framework level. |
|
Yeah, I'm a bit concerned about this change as well. I know that this was a deliberate choice indeed and that the initial design was both for performance reasons and to match redux (connect initially). Why it's only one level because this it's common practice that we return an object with multiple selectors in fact it was just a coincidence that we noticed that we could call it with the selector directly In that sense, in this second example, even shallow comparison could be removed but it's just not easy to separate the two cases as selectors can also return objects. And actually, this PR breaks |
|
useSelect( ( s ) => ( {
a: select( 'a' ).get(),
b: select( 'b' ).get(),
} );Here the shallow equal check never compares the actual data, just the If we returned the data directly: useSelect( ( s ) => select( 'a' ).get() );then Comparing the actual data should never be needed. There's no universal method to compare anyway. For example, const eq = require('fast-deep-equal');
function createTree() {
const el = { children: [ {} ] };
el.children[0].parent = el;
return el;
}
const a = createTree();
const b = createTree();
eq(a,b);This will crash with "Maximum call stack size exceeded" Redux |
|
Quick note that this can have dramatic worst-case performance implications.
There's a small misunderstanding here on what equality means in JavaScript. Objects and array will compare equal if they are equal, but the example demonstrated two unequal objects. Every object literal will create a new distinct object. If you change it to comparing two objects that are equal then of course, they will evaluate as equal. const a = { foo: { bar: 1 } };
const b = a;If a shallow comparison ever shows equality for unequal objects it implies we have a defect in our state system somewhere, because of the lack of value objects in JavaScript or a way to identify value equality. That is, every update should return a new value, which if it's an object, will not be equal to the previous one. The other way around we might be identifying unoptimized state or selectors, whereas if we find no changes for a given action in the data system, we should return the previous state, which will always evaluate as value equal. If we return a tree of data though, as @jsnajdr pointed out, each time we create that tree it's a new object, thus not equal. His proposed alternative of returning the data directly is the proper remedy. Deep equality must examine all the data and must convert any data it can't natively compare, and then still we cannot compare everything. If, for example, we had loaded in 45 MB of WP pages, as I have seen done when populating the parent page selector, then deep equality must traverse and compare all that information on every change, which could quickly lead to an application freeze. So more or less if we find extraneous renders I think it will be more profitable to inspect those components and see why they are depending on coarser data than they use to render, why they are pulling in more dependencies than they actually have. We can prune the data fetching where it's happening and cut out the extra renders, then not have to introduce a potential performance time bomb into the code. |
|
Thank you everyone for the great, thorough feedback! I couldn't have asked for better context and explanation of prior decisions. 🙌 Given all the context, I agree with the sentiment and will be closing this PR, and I'll be looking for more specific solutions where applicable. |
@youknowriad I'd love to learn more about this selector because I noticed |
|
"edits" are stored in separate places (entities) if I'm not wrong and we just want to be notified when there are "new edits" basically and right now it relies on a change to the "undo" stack. |
|
Gotcha, thank you! |
What?
This PR updates
useSelect()to perform deep equality comparison instead of shallow equality comparison when updating values.Why?
While evaluating performance in the editor, I noticed that there are a bunch of rerenders for the same data. On a deeper inspection, I observed that some of them are with the exact same data. Then I noticed that when updating the value in
useSelect(), we'll do a shallow equality comparison when attempting to preserve old values that are equal to the new ones, attempting to not trigger unwanted updates. This works fine, but only when comparing primitive values. When comparing objects that contain objects, even if they are equal,isShallowEqual()will intentionally returnfalse:That makes
useSelect()inefficient when it retrieves more complex values that contain nested objects - it will always return new values, even when they are equal to the old ones. That causes unnecessary updates and re-renders as a consequence.How?
We're updating
useSelect()to usefastDeepEqualinstead of@wordpress/is-shallow-equal.fastDeepEqualis already widely used as a substitute for Lodash's_.isEqual(), so it's a safe bet.We're introducing a new unit test and fixing an existing test to not expect a rerender for equal values.
Feel free to try the new unit test against the
useSelect()intrunkto see it failing.Testing Instructions
Testing Instructions for Keyboard
None
Screenshots or screencast
None