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.

Sign up for GitHub

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

Jump to bottom

Block Editor: use shallow memo for prioritized inserter blocks #65737

Merged
merged 2 commits into from
Oct 7, 2024
Merged

Block Editor: use shallow memo for prioritized inserter blocks #65737

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4958b9b
Block Editor: use shallow memo for prioritized inserter blocks
tyxla Sep 30, 2024
6c85abb
Add single-pass implementation of useShallowMemo
jsnajdr Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 16 additions & 10 deletions packages/block-editor/src/components/inner-blocks/use-nested-settings-update.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* WordPress dependencies
*/
import { useLayoutEffect, useMemo, useState } from '@wordpress/element';
import { useLayoutEffect, useState } from '@wordpress/element';
import { useRegistry } from '@wordpress/data';
import deprecated from '@wordpress/deprecated';
import isShallowEqual from '@wordpress/is-shallow-equal';
Expand All @@ -16,12 +16,21 @@ import { getLayoutType } from '../../layouts';

const pendingSettingsUpdates = new WeakMap();

// Creates a memoizing caching function that remembers the last value and keeps returning it
// as long as the new values are shallowly equal. Helps keep dependencies stable.
function createShallowMemo() {
let value;
return ( newValue ) => {
if ( value === undefined || ! isShallowEqual( value, newValue ) ) {
value = newValue;
}
return value;
};
}

function useShallowMemo( value ) {
const [ prevValue, setPrevValue ] = useState( value );
if ( ! isShallowEqual( prevValue, value ) ) {
setPrevValue( value );
}
return prevValue;
const [ memo ] = useState( createShallowMemo );
return memo( value );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a side effect here at render time, comparable to reading/writing a ref, which makes this unsafe for concurrency and a future source of tearing.

The only reason this doesn't trigger a React compiler error is that their linter is not accounting for this more obscure way of setting and reading non-stateful values, but it is no different than a ref-based solution and the error applies all the same.

}

/**
Expand Down Expand Up @@ -77,10 +86,7 @@ export default function useNestedSettingsUpdate(
// otherwise if the arrays change length but the first elements are equal the comparison,
// does not works as expected.
const _allowedBlocks = useShallowMemo( allowedBlocks );

const _prioritizedInserterBlocks = useMemo(
() => prioritizedInserterBlocks,
// eslint-disable-next-line react-hooks/exhaustive-deps
const _prioritizedInserterBlocks = useShallowMemo(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good idea to use useShallowMemo here but I think the useShallowMemo utility is not implemented very well. It will do a cascade of setState calls and rerenders. It should be storing the memoized value in a ref instead. Something like:

function useMemoEx( callback, compare ) {
  const memoRef = useRef( undefined );
  const newValue = callback();
  if ( memoRef.current === undefined || ! compare( memoRef.current, newValue ) ) {
    memoRef.current = newValue;
  }
  return memoRef.current;
}

I'm pretty sure we already have such a hook (memo with custom compare) defined somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point 👍

I've seen you working on it in another repository.

Feel free to open a PR, I think it can be improved separately.

Copy link
Contributor

@DaniGuardiola DaniGuardiola Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsnajdr the implementation you're proposing is unsafe because you're reading/writing refs in render, which is forbidden by the React Gods (can mess things up in concurrent rendering trees). When doing render-time computations, you simply can't use refs.

However, many use cases for render-time refs can be replaced by derived state, which is a technique consisting of calling a setState at render time, which causes the render function to instantly re-run synchronously, skipping all effects and other work of the kind. This is fine, as long as the state is guaranteed to settle, otherwise it can result in an endless loop. Can't find it right now, but the technique is officially listed somewhere in the official, updated docs.

Running a component's render function is typically pretty much free, so it'd be rare if this technique is a problem for performance. Re-rendering is a tool, not a danger to avoid.

It'd be another story imo if you found a specific perf problem somewhere that is caused by excessive re-rendering, that can happen but anything earlier than that feels like premature optimization (or even worsening, as sometimes re-rendering is the better option).

That's my 2cents anyway :)

Copy link
Member

@Mamaduka Mamaduka Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some context for the original useShallowMemo - #53943 (comment). We can adopt the usePrevious hook to achieve the same behavior by adding the optional compare as the second argument. But duplication is okay, as usual.

Aside: The usehooks.com has also updated its implementation of usePrevious to use the new pattern.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this about 3 years ago: https://codesandbox.io/p/sandbox/previous-vs-previous-value-t7t3y?file=%2Fsrc%2FApp.tsx

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't agree that breaking the rules to prevent renders without a clear performance issue is the pragmatic and less ideological way, I'd argue it's the opposite. That said, we can agree to disagree on that too :P

Just hope I don't have a "told you so" moment a few months down the line. No hard feelings either way!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the general case, I do agree that we shouldn't be disobeying the React Gods without a substantial upside, and without a mechanism to catch problems when things start to break. Especially because this seems to be the type of thing where the kinds of bugs that could possibly occur in the future are very subtle.

I'm not privy enough of React internals to assess how much of a special exception this is to the general case. In the general case, just because an official hook adopts an implementation internally right now doesn't necessarily make it safe to imitate in userland. Could they rewrite the internal useMemo implementation without us noticing? Could they add a special safeguard for useMemo that doesn't apply to userland imitations? If there really is no magic and we can confidently answer No to those questions, then sure, maybe this is a special exception. (Unfortunately I cannot confidently answer those questions 😅)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's helpful to try to really understand why exactly reading/writing refs during render can be dangerous. The metaphors about rules written by gods are counterproductive, and also not completely innocent, because they suggest that we're dealing with some revealed truths that are inaccessible to reason. That's not true.

One example of a bad component is this:

function Foo( { value } ) {
  const valueRef = useRef();
  valueRef.current = value;
  return <button onClick={ () => { console.log( valueRef.current ); } } />
}

Here, if you render <Foo value="hello"/>, clicking on the button will print hello. That's good. Now React can start rerendering the tree that contains <Foo />, with another value of the prop, but that render is aborted. It suspends or is interrupted, and the results are thrown away. DOM is not changed and effects are not run. But the valueRef.current = value modification was done and it stays. It's an assignment very similar to calling this.value = ... on a component instance, there is nothing sophisticated about it.

This means that an aborted render of <Foo value="world" /> will actually modify valueRef and clicking on the button will log world. The modification that was not supposed to happen has a material effect on the rendered UI. We can construct many other examples like this where aborted, tentative renders can leak into the active UI. That's why the ref access should be moved into effects, because effects run only when the result of the render was actually commited. Or we should use state, because the state of a committed component is isolated from the state of the tentative render, and the updates are not done directly, but go though a managed queue. Refs are not isolated or managed like this, they are "naked" values.

But not all ref reads/writes are dangerous like this. The React docs contain this example that gives us a first piece of evidence that the rules are not absolute.

function Video() {
  const playerRef = useRef(null);
  if (playerRef.current === null) {
    playerRef.current = new VideoPlayer();
  }
  // ...

Here we read and write a ref during render, but the docs say this is fine!

Normally, writing or reading ref.current during render is not allowed. However, it’s fine in this case because the result is always the same, and the condition only executes during initialization so it’s fully predictable.

Indeed, if a tentative render that's going to be aborted initializes the VideoPlayer, we don't mind, because we would create it the same way anyway. And most of the time the tentative render will find out the VideoPlayer already exists and will do nothing. Nothing bad can happen.

Now let's check if useMemo could also possibly be safe like this. Let's implement useMemo in user space:

function useMemo( create, deps ) {
  const prevState = useRef();
  if ( prevState.current === undefined || ! areDepsEqual( prevState.current[ 1 ], deps ) ) {
    prevState.current = [ create(), deps ];
  }
  return prevState.current[ 0 ];
}

Two things to note here:

  1. The hook's create parameter is always capable to produce a valid return value. And it's constructed from "pure" React values: the props, the state, the context. Also the deps are derived from these "pure" values.
  2. The hook is very skeptical about the value in the prevState ref. Before using it, it carefully verifies that it's valid, i.e., whether it was constructed with the same deps. If some bad value leaked into the prevState ref from an aborted render, this will be detected, the value will be ignored, and will be recalculated from the "pure" create function.

That means that the useMemo hook is resilient against bad values that leak into it from aborted renders. The only thing that the aborted renders can ever achieve is that they invalidate your memoization and force you to recreate it, but the hook will never return a bad value.

That's why the native useMemo hook is safe, and also why a user-space useMemo or useShallowMemo implementation is also safe if done well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsnajdr Thanks for the explanation!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks y'all for the feedback!

I agree with @jsnajdr and will move to ship this, and will keep an eye in the future.

prioritizedInserterBlocks
);

Expand Down
Loading