liferay / liferay-frontend-projects

A monorepo containing assorted Frontend Infrastructure Team projects
Other
69 stars 68 forks source link

Add guidelines for when to use useMemo and useCallback #383

Closed wincent closed 1 year ago

wincent commented 3 years ago

This one might be a specific instantiation of adding something about "when/how to use React hooks" to our guidelines, but I have had a few interactions in PRs about when to use useMemo and useCallback, and as it keeps coming up that is probably a cue to put it somewhere in our docs (the other "tricky" hook that tends to come up a bit is useEffect and relatedly useLayoutEffect, but that one is even harder to talk about, so I't probably start with the easy ones first).

See:


I'd be very careful about adding useCallback or useMemo just to avoid "re-creating functions repeatedly". Note that in order to make use of either, you are instantiating a new function every time through your render anyway; eg: (function chosen at random, but it's just for illustration):

function DataSetDisplay() {
    // stuff

    function updateDataSetItems(dataSetData) {
        setTotal(dataSetData.totalCount);
        updateItems(dataSetData.items);
    }

    // etc
}

vs:

function DataSetDisplay() {
    // stuff

    const updateDataSetItems = useCallback((dataSetData) => {
        setTotal(dataSetData.totalCount);
        updateItems(dataSetData.items);
    }, [...]);

    // etc
}

The first version is re-instantiating a function object on every render, but so is the second one (the arrow function). In addition, it is paying the overhead of calling useCallback on every render, and it is allocating a dependencies array on every render. useCallback itself is going to incur the cost of comparing the items in the dependency array (using Object.is()), as well as the memory overhead of any related book-keeping details that React is maintaining internally.

Now, I'm not saying "never use useCallback or useMemo"; I'm just saying, make sure you have a good reason to use them. A non-exhaustive list of good reasons for using them is:

  • Use useMemo to avoid repeatedly recomputing an intermediate value that is expensive to evaluate (note: in order for this to pay off, the value shouldn't be changing on every render and the cost of determining equality should be cheaper than just redoing the calculation — remember that you're paying the cost of the equality comparison plus the overhead of the function call into React internals plus whatever internal book-keeping React might be doing). In many cases, it's cheaper to just re-do the computation every time.
  • Use useCallback when you want to maintain a stable identity for a callback function that you will be passing down to children; this can be important when the callback ends up being a member of a useEffect dependencies array in the child (or grandchild, or great-grandchild etc). If the callback hasn't really changed, you may find that failure to use useCallback causes children to re-run effects unnecessarily because they think something has changed when it really hasn't. This is another context-dependent thing: sometimes you know everything about your children subtree and you can know deterministically that useCallback is required; at other times you are writing "library" code and you have no idea what your children are going to be doing, so you may want to err on the side of being defensive and using useCallback "just in case".

Another angle on this: https://kentcdodds.com/blog/usememo-and-usecallback/

Performance optimizations ALWAYS come with a cost but do NOT always come with a benefit.


Well, there's no magic. The point is just that any prop you pass into a component could end up causing that component to re-render unnecessarily if that prop is not stable over time, and is used as an input into a useEffect dependencies array, or useCallback, or useMemo etc. This may be subtle too, because if the prop is passed down to a grandchild or a great-grandchild, the place where the prop is causing a re-render may be buried several levels down.

So, the basic pattern is, whenever you see <Component prop={value} />, two of the questions you should have in mind if you want to maximize interoperability are: "Can value be stable over time? If so, what do I have to do to make it stable?" If you do that as a matter of standard practice, your code is more likely to be performant now and in the future (when it gets transplanted into some unforeseen context).

In this case, () => {} is not stable over time, because every time you render, you make a new anonymous function and (() => {}) !== (() => {}). But given const noop = () => {} at the top-level of the module, noop === noop and is stable, so it's a good pattern to follow in general. (You should think about similar gotchas with default values for function parameters: if you have a param callback with a default — callback = () => {}, then that's going to be a different fallback callback every time somebody doesn't pass a default parameter.)

bryceosterhaus commented 3 years ago

Responding to your comments from other PR here.

The first version is re-instantiating a function object on every render, but so is the second one (the arrow function)

Hmm, maybe I am misunderstanding here but I believe the second one(useCallback) will only re-instantiate the function object when the dependencies change. Thats the benefit of useCallback? Example

I agree that we shouldn't be using useMemo or useCallback for everything, and we should really only be using them for expensive operations. However, I encourage the practice to evaluate function declarations for memoization in components because often as you start to evaluate the function, you realize it may not even be necessary to exist within the component lifecycle. I have seen this often with people who aren't as familiar with React and so they start to lump everything into the render lifecycle. By encouraging people to evaluate memoization I think it also encourages people to think about the component lifecycle which I don't think is thought about as often with hooks, especially with devs who use React less often.

So tldr; I would rather encourage people to evaluate with memoization in mind rather than discourage the use of it.


So, the basic pattern is, whenever you see <Component prop={value} />, two of the questions you should have in mind if you want to maximize interoperability are: "Can value be stable over time? If so, what do I have to do to make it stable?" If you do that as a matter of standard practice, your code is more likely to be performant now and in the future (when it gets transplanted into some unforeseen context).

^^ Couldn't agree with this more. The question is, how do we get devs to think in this manner? Especially people who might only touch react on occasion and not weekly or daily.

wincent commented 3 years ago

Hmm, maybe I am misunderstanding here but I believe the second one(useCallback) will only re-instantiate the function object when the dependencies change. Thats the benefit of useCallback? Example

Maybe we're not connecting on the level of terminology; when I talk about "instantiation" I am talking about a temporary function object being instantiated on the heap (along with the associated data structures in the JS VM to track what variable bindings should remain accessible via the closure) and later garbage collected. Maybe if I rewrite it like this it will be clearer what I mean:

function A() {
   function foo () {}
}

function B() {
   const memoizedFoo = useCallback(function foo() {}, [...]);
}

Obviously, the identity of memoizedFoo() is stable across calls (that's what useCallback is for), but it doesn't — as your original comment implied — "avoid re-creating these functions repeatedly" — in both cases the functions are recreated; the difference is that with useCallback, React does some additional work to keep track of whether the items in the dependencies array have changed and returns the previously-seen function instead of the new one when they haven't. So that's function call overhead, plus work internal to useCallback, and the memory overhead of maintaining a snapshot of the dependencies array between calls.

Is useCallback a terrible thing then? Nope; in my comment I listed legit reasons to pay the cost of useCallback, but it is not free. We should use it in the specific scenarios where we expect it to deliver a benefit.

So tldr; I would rather encourage people to evaluate with memoization in mind rather than discourage the use of it.

Yeah, I totally agree with that, which is why I created this issue. I want guidelines so that we can make sure that people actually understand how useCallback and useMemo work so that they can make an informed decision about what the costs and benefits are. Having people merely "evaluate" the use of something isn't helpful if the evaluation is wrong/confused.

I think the wording of your original advice is dangerous, because even if you know all the details, I expect that most other people don't, so they'll see a statement like "For the sake of performance and to avoid re-creating these functions repeatedly" and take that as an unconditional recommendation to just use useCallback and useMemo whenever they can, because more must be better. This isn't just a hypothetical problem; I've seen people make this mistake many times in Liferay, and I bet Kent C Dodds has too in other contexts, which is why he wrote that article.

bryceosterhaus commented 3 years ago

Maybe we're not connecting on the level of terminology

Yeah I think this is likely the case, and I think we likely generally agree in principle on this, however let me see if I fully understand what you are saying.

a new function foo is still created every time the function is called

Are you saying that "foo" is created every time B() is called? Or every time memoizedFoo() is called?

Using your examples above, my understanding is that

I understand that in both cases foo will be allocated memory for each render, but in A it gets garbage collected and then re-allocated to memory for each render. Where B it would be allocated to memory on the first render and then stay there until the component is disposed (assuming no dependencies). My thought process might be wrong here, but isn't it better to put it in memory once rather than remove/add on each render?

If we are saying it's better to remove/add on each render, then why would we want to move pure functions outside of a components render? It would make sense to always keep them within the component.

function sum(a, b) {
    return a + b;
}

function MyComponent(props) {
    return <div>{sum(props.first, props.second)}</div>
}

// vs....

function MyComponent(props) {
    function sum(a, b) {
        return a + b;
    }

    return <div>{sum(props.first, props.second)}</div>
}
wincent commented 3 years ago

Are you saying that "foo" is created every time B() is called? Or every time memoizedFoo() is called?

The former. Forget about useCallback() and React for a second and look at this vanilla JS:

function A() {
   const object = {};
   const foo = function () {};
   const wrappedBar = wrapper(function bar() {});

   // Some code that uses the above...
}

Every time A is called, we get new versions of object, foo, and bar. Whether or not wrappedBar is a new value or not depends on what the call to wrapper() does (ie. maybe it returns a new function, maybe it returns some memoized value, maybe it returns a 🍰 ).

All of these objects are !==. That is, object from first render doesn't === the object from any subsequent renders. Same for foo and bar. Depending on whether any references to these objects survive (ie. via a closure, or because we pass them into somewhere else, or we return them etc) they will either get garbage-collected (sooner, or later) or not.

Connecting this back to React, the whole point of useCallback() is to work around this behavior and maintain a stable identity for functions that would otherwise change on every render. So, yes, when we render B() a new function foo() is created on every render, but useCallback() gives us back a previously-seen version that is === instead.

So, what's the main reason you would care about callback stability? It's because if you are passing the callback to another component, you may not know how it is going to be used (especially if you are writing "library" code that will be used in contexts you don't know about), and that means somebody might be passing it into a useEffect dependencies array. If you fail to keep the identity stable, the useEffect may run needlessly which could be arbitrarily expensive (again, remember that you might not know what the other components are going to do in their effects) which would create performance problems, or worse than that, it might actually cause bugs (eg. it could cause an infinite loop, or trigger a mutating side-effect that is supposed to only run once etc). It doesn't need to be just useEffect either; consider any code or API that relies on identity (like addEventListener and removeEventListener, where the latter only works if you pass in the same=== — function that you passed to the former).

In general, the work of instantiating a callback isn't the expensive part. This is in contrast to useMemo() which is all about avoiding expensive-but-avoidable work. Compare these:

const value = useMemo(function crunchTheNumbers() {
  // 50 lines of code doing some expensive computation in here...
}, [...]);

const callback = useCallback(function smellTheRoses() {
  // 50 lines of code doing something that may or may not be expensive...
}, [...]);

In the example above:

Now, in practice, it is good to avoid creating a lot of short-lived garbage, to reduce pressure on the garbage collector, which takes us back to your last example:

function sum(a, b) {
    return a + b;
}

function MyComponent(props) {
    return <div>{sum(props.first, props.second)}</div>
}

// vs....

function MyComponent(props) {
    function sum(a, b) {
        return a + b;
    }

    return <div>{sum(props.first, props.second)}</div>
}

If sum is a pure function that can be extracted because it doesn't rely on any values captured via the closure, then extracting it out is indeed a small win. We'll have exactly one sum function in existence for the lifetime of the program and no short-lived garbage related to it. In addition to relieving GC pressure, it also is immune to the potential pitfalls we talked about with respect to identity; you can pass it around anywhere you want and you know that anybody who cared to check would find it === every time. I believe the infamous "exhaustive-deps" lint rule is smart enough to know about this too; if you use sum inside a useEffect, it won't require you to add it to the deps array, because it knows statically that it will never change — this is nice because it reduces noise and clutter in the code.

Of course, there are many situations where functions aren't pure; even if they don't have side-effects, they may need values that they close over. You can sometimes refactor these into pure functions that take parameters instead of implicitly accessing them via the closure, but I usually don't go too far down that path for a few reasons:

Like many things, it ends up being a judgement call; if your code is not in a frequently called "hot loop" (which hopefully, is the case for most render functions if we do our job right), then it simply doesn't matter.

bryceosterhaus commented 3 years ago

I think in essence we agree, but I think I would disagree with this statement below.

yes, when we render B() a new function foo() is created on every render, but useCallback() gives us back a previously-seen version that is === instead.

memoizedFoo is created every render, but it is referencing an existing object in memory. This is easily testable by checking how many times the console.log fires in the example below

function App() {
  const memoizedFoo = React.useMemo(() => {
    console.log('foo created');

    return function foo() {};
  }, []);

  return // ...
}

Maybe it's just a wording disagreement, but I wouldn't say that a new foo function is created on every render. Other than that though, I fully agree with it being a judgement call and we mostly need to pay attention to high-cost functions.

In regards to the main point of guidelines though, I thought Kent's summary was a good lens to view it though.

So when should I useMemo and useCallback?

There are specific reasons both of these hooks are built-into React:

  1. Referential equality
  2. Computationally expensive calculations

Expensive calcs are generally easier to spot, but his first point is probably the one with more nuance and requires more "teaching" to devs who don't use react as much.

wincent commented 3 years ago

I think in essence we agree, but I think I would disagree with this statement below.

yes, when we render B() a new function foo() is created on every render, but useCallback() gives us back a previously-seen version that is === instead.

memoizedFoo is created every render, but it is referencing an existing object in memory. This is easily testable by checking how many times the console.log fires in the example below

Those statements aren't related. My statement is a (correct) statement about foo() and your statement is a (correct) statement about memoizedFoo(). But the statements are talking about two different things: I'm talking about the value (foo) that is passed into useCallback() and you're talking about the value memoizedFoo which is returned from useCallback(). (Well, technically I used useCallback() in my example and you used useMemo() in yours but it's not an important distinction here.)

function App() {
  const memoizedFoo = React.useMemo(() => {
    console.log('foo created');

    return function foo() {};
  }, []);

  return // ...
}

What your console.log shows is how many times the arrow function body is executed; it tells you nothing at all about how many object in memory there are. Let's modify your example a bit to show that we're creating different function objects in memory every time we render App:

window.allocatedFunctions = [];
window.memoizedObjects = [];

function App() {
    console.log('rendering');

    const memoizedFoo = useMemo(function foo() {
        console.log('i am only printed once');

        return () => {};
    }, []);

    window.allocatedFunctions.push(foo);
    window.memoizedObjects.push(memoizedFoo);

    return null;
}

App();
App();

console.log(allocatedFunctions.length); // 2
console.log(allocatedFunctions[0] === allocatedFunctions[1]); // false
console.log(memoizedObjects.length); // 2
console.log(memoizedObjects[0] === memoizedObjects[1]); // true

Note I've changed the arrow function to a named function just so that I can easily get a reference to it to shove in my tracking array, but you would observe exactly the same behavior with arrow functions, if you kept a reference. You're going to see these logs:

rendering
i am only printed once
rendering
2
false
2
true

Maybe it's just a wording disagreement, but I wouldn't say that a new foo function is created on every render.

Does my example make it clearer? We are definitely getting two distinct foo functions (as shown because allocatedFunctions[0] is not === to allocatedFunctions[1]). And, of course, useMemo() is doing what it is advertized to do: we always get the same object back (as shown because memoizedObjects[0] is === to memoizedObjects[1]).

I thought Kent's summary was a good lens to view it though.**

So when should I useMemo and useCallback?

There are specific reasons both of these hooks are built-into React:

  1. Referential equality
  2. Computationally expensive calculations

Expensive calcs are generally easier to spot, but his first point is probably the one with more nuance and requires more "teaching" to devs who don't use react as much.

Yeah, his summary is good (and it matches what I said in the bullet points in the quoted comment in the issue description), but it evidently needs a lot of explanation to be actually understood, as I think we've seen in this thread.

bryceosterhaus commented 3 years ago

Ah I see, that last example helps clarify what you are talking about and I agree with that. I think I was mis-communicating what I meant by "created." My thought process was more focused on invocation of the argument to useMemo(useCallback does this internally) and the code ever reaching foo on subsequent renders.

function App() {
  const memoizedFoo = React.useMemo(function wrapper() {
    return function foo() {};
  }, []);

  return // ...
}

function App2() {
  function foo2() {};

  return // ...
}

So for this example, let me make sure I understand it correctly and maybe it's how I am viewing "created"...

App

App2

Difference being, the first example limits the number of times execution ever reaches return function foo() { //...

Is there a better word for "created" that I am missing or am I just mis-understanding what is actually happening when this code runs?

wincent commented 3 years ago

App

  • wrapper() is only invoked on the first render. (my thought here is that this function is "creating" foo)

Yep.

  • Subsequent renders, wrapper() is never invoked because no dependencies changed. (meaning foo is not "created" more than once).

Yep. The nuance is that a new instance of wrapper() is created on every render (space for new function object allocated on the heap, constructed and initialized), even though useMemo only calls it on the first time.

App2

  • foo2() is being "created" every time App2 re-renders.

Difference being, the first example limits the number of times execution ever reaches return function foo() { //...

Yep.

Is there a better word for "created" that I am missing or am I just mis-understanding what is actually happening when this code runs?

I think created is a fine word but we just need to be precise about what we are applying it to. You can stop reading here if you want, because we're now getting into the most precise little details of language and semantics...

In the App example we have three entities of interest:

Unpacking those in the context of useMemo():

So when I am thinking about what any given piece of code does, I am thinking about three things:

Those three are all just JS; finally, to bring this into the world of React, we can add a fourth:

bryceosterhaus commented 3 years ago

👍 makes sense to me, and sounds like we are on the same page in both theory and practice. Thanks for the back and forth and getting into the details, it helped give a more full picture of how it all works.

wincent commented 3 years ago

All the cool kids are writing guidelines on when to use useMemo:

(Via React Status).

bryceosterhaus commented 3 years ago

Screen Shot 2021-02-17 at 10 49 51 AM

😞

wincent commented 3 years ago

sigh-swanson

To be honest, you didn't miss much. It shares benchamarks showing the totally unsurprising result that useMemo isn't free, but if you use it to avoid expensive calculations, it is beneficial:

Screenshot 2021-02-18 -111448-o8zyKKwM@2x
matuzalemsteles commented 3 years ago

That looks interesting 🤔. Some information about the machine that was used to do these tests would be interesting to understand if this can change on different machines or browser versions. Should probably continue with some margin of error...

wincent commented 3 years ago

Should probably continue with some margin of error...

Right, instead of running for n = {1, 100, 1000, 5000} I am sure you could find equivalent-ish values of n on any given browser or machine that would be show roughly the same result.

matuzalemsteles commented 3 years ago

Right, instead of running for n = {1, 100, 1000, 5000} I am sure you could find equivalent-ish values of n on any given browser or machine that would be show roughly the same result.

Yeah! Were all these tests in Chrome? We can have the same in IE 11 🤪, just joke haha

wincent commented 3 years ago

Were all these tests in Chrome?

Doesn't actually say, but it seems a safe bet 😉

Screenshot 2021-02-22 -093416-25Nf0Ahl@2x
matuzalemsteles commented 3 years ago

Yeah, Well, anyway it helps us to offer a better guide.

jbalsas commented 3 years ago

Another one, just in: Before You memo()

bryceosterhaus commented 3 years ago

Screen Shot 2021-02-23 at 11 01 25 AM 😂