facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
229.08k stars 46.86k forks source link

useMemo / useCallback cache busting opt out #15278

Open alexreardon opened 5 years ago

alexreardon commented 5 years ago

According to the React docs, useMemo and useCallback are subject to cache purging:

You may rely on useMemo as a performance optimization, not as a semantic guarantee. In the future, React may choose to “forget” some previously memoized values and recalculate them on next render, e.g. to free memory for offscreen components. Write your code so that it still works without useMemo — and then add it to optimize performance. source

I am working on moving react-beautiful-dnd over to using hooks https://github.com/atlassian/react-beautiful-dnd/issues/871. I have the whole thing working and tested 👍

It leans quite heavily on useMemo and useCallback right now. If the memoization cache for is cleared for a dragging item, the result will be a cancelled drag. This is not good.

My understanding is that useMemo and useCallback are currently not subject to cache purging based on this language:

In the future, React may choose to “forget”

Request 1: Is it possible to opt-out of this cache purging? Perhaps a third options argument to useMemo and useCallback:

const value = useMemo(() => ({ hello: 'world' }), [], { usePersistantCache: true });

(Naming up for grabs, but this is just the big idea)

A work around is to use a custom memoization toolset such as a useMemoOne which reimplements useMemo and useCallback just using refs see example

I am keen to avoid the work around if possible.

Request 2: While request 1 is favourable, it would be good to know the exact conditions in which the memoization caches are purged

gaearon commented 5 years ago

If you want to rely on it as a semantic guarantee, using a ref sounds like the way to go. Why is that not working out for you?

alexreardon commented 5 years ago

Using a ref is working for me, but I thought it would be nice to not need to use an alternative. I suspect other people might also want to opt out of cache purging if I do. Also, moving away from useMemo and useCallback currently means losing some of the value provided by eslint-plugin-react-hooks

alexreardon commented 5 years ago

Even if useMemo and useCallback cache behaviour doesn't change it would be nice to know under what conditions the caches are purged

ntucker commented 5 years ago

@alexreardon How are you computing the value before useEffect runs? Or are you doing the should-it-update checks manually?

alexreardon commented 5 years ago

My current approach is fairly naive and needs to be thought through:

It currently does not use useEffect or useLayoutEffect. It does a side effect during the render

ntucker commented 5 years ago

Ah ya the manual approach. Probably uses more memory than if it were baked into React. Definitely adds more bundle bloat. Thanks @alexreardon !

aweary commented 5 years ago

I think this is an uncommon enough case that using useRef to implement your own stable references is the right approach.

While request 1 is favourable, it would be good to know the exact conditions in which the memoization caches are purged

I don't think it's valuable to document the internal caching strategy beyond "React might clear the cache when it needs to" since it's likely to be dynamic and difficult to predict. No single component could predict cache hits or misses at runtime with any certainty.

It does a side effect during the render

Just a heads up that this is likely to be problematic in concurrent mode, since the function might be called many times with different props.

alexreardon commented 5 years ago

Just a heads up that this is likely to be problematic in concurrent mode, since the function might be called many times with different props.

What is the recommendation then for this behaviour? useEffect? When does useMemo update?

ntucker commented 5 years ago

@aweary Interesting, so you don't think caching the promise thrown for Suspense will be common? I hear if you return a different promise everything breaks.

alexreardon commented 5 years ago

Here is an alternative useMemoOne that seems to be concurrent mode safe 🤞

// @flow
import { useRef, useState, useEffect } from 'react';
import areInputsEqual from './are-inputs-equal';

type Result<T> = {|
  inputs: mixed[],
  result: T,
|};

export default function useMemoOne<T>(
  // getResult changes on every call,
  getResult: () => T,
  // the inputs array changes on every call
  inputs?: mixed[] = [],
): T {
  // using useState to generate initial value as it is lazy
  const initial: Result<T> = useState(() => ({
    inputs,
    result: getResult(),
  }))[0];

  const uncommitted = useRef<Result<T>>(initial);
  const committed = useRef<Result<T>>(initial);

  // persist any uncommitted changes
  useEffect(() => {
    committed.current = uncommitted.current;
  });

  if (areInputsEqual(inputs, committed.current.inputs)) {
    return committed.current.result;
  }

  uncommitted.current = {
    inputs,
    result: getResult(),
  };

  return uncommitted.current.result;
}
urugator commented 5 years ago

If there are no deps I think that useState is appropriate: const [value] = useState(() => ({ hello: 'hello' })); useRef is viable, but doesn't offer lazy init and aRef.current is annoying. If there are deps and the memoization isn't just an optimization, but a semantic guarantee, then you're dealing with derived state. Here is a hook based on react's useMemo sources: https://gist.github.com/urugator/5c78da03a7b1a7682919cc1cf68ff8e9 Usage const value = useDerivedState(() => ({ hello: aProp }), [aProp]); Conceptually I think it's similar to getDerivedStateFromProps

alexreardon commented 5 years ago

I have shipped useMemoOne which is a drop-in replacement for useMemo and useCallback that has a stable cache - for those who need it

otakustay commented 5 years ago

We also have cases where useMemo is required for semantic guarantee:

const {keyword} = props;
const keywordList = useMemo(
    () => keyword.split(' '),
    [keyword]
);
const flipList = useCallback(
    () => {
        // something about keywordList
    },
    [keywordList]
);
useEffect(
    () => {
        someSideEffectWithKeywordList(keywordList);
    },
    [keywordList]
);

useCallback is not sensitive to cache purging, it only provides performance improvements, however useEffect can results in unexpected behaviors when keywordList is busted from its cache.

Currently we try to get rid of this risk by computing keyword.split(' ') inside both useCallback and useEffect, this is not what we want actually.

skyboyer commented 5 years ago

@otakustay can you rely on keyword data instead of keywordList callback for your useEffect?

otakustay commented 5 years ago

can you rely on keyword data instead of keywordList callback for your useEffect?

I tried this, then I encountered 2 issues:

  1. exhaustive-deps eslint rule complains about it, I'm required to disable lint rule every time, which is not a happy experience
  2. More often I need to pass keywordList down to child components, I can't pass keyword instead of it because there could be a large amount of child components receiving this prop, making component only receiving primitive type props are not my choice
bhovhannes commented 5 years ago

@alexreardon useCallback docs do not say that it is subject to cache purging. Docs are not clear enough here though.

Docs only say that:

useCallback(fn, deps) is equivalent to useMemo(() => fn, deps)

If useCallback is equivalent to useMemo, why we have useCallback at all? I assume if there is a separate useCallback hook shipped with React there is a reason for that and that reason is not documented well.

jedwards1211 commented 5 years ago

@bhovhannes useCallback is just a convenient shorthand that makes the code more legible. Writing

useMemo(() => e => e.stopPropagation())

isn't as pleasant to read or write as

useCallback(e => e.stopPropagation())

and the double => probably confuses novice JS devs as well.

jedwards1211 commented 5 years ago

@urugator Yup aRef.current is only good for element refs, for any other persistent values it's super annoying. So I abuse it like this instead, especially when I would need multiple useRef calls:

const stash = useRef({}).current
if (!stash.foo) stash.foo = ...
stash.onChange = props.onChange
...

useEffect(() => {
  const {onChange} = stash
  onChange(value)
}, [value])
jedwards1211 commented 5 years ago

@alexreardon @urugator I haven't read much into concurrent mode but would the following work?

function useMemoOne(compute, deps) {
  const stash = useRef({}).current
  if (!stash.initialized) stash.value = compute()
  useEffect(() => {
    if (!stash.initialized) stash.initialized = true
    else stash.value = compute()
  }, deps)
  return stash.value
}

function useCallbackOne(callback, deps) {
  const ref = useRef(callback)
  useEffect(() => {
    ref.current = callback
  }, deps)
  return ref.current
}
otakustay commented 5 years ago

@jedwards1211 No, useEffect is too late, the following one may be better:

const useMemoOne = (compute, deps) => {
    const value = useRef(compute);
    const previousDeps = useRef(deps);

    if (!shallowEquals(previousDeps, deps)) {
        previousDeps.current = deps;
        value.current = compute();
    }

    return value.current;
};
jedwards1211 commented 5 years ago

I was about to delete my comment, I wasn't thinking about how it would be too late. I don't really understand why this side effect during render would be more problematic for concurrent mode than anything else, as @aweary implied. Will useMemo do something special in concurrent mode that's impossible to implement with useRef and render side effects?

eps1lon commented 5 years ago

I don't really understand why this side effect during render would be more problematic for concurrent mode than anything else

if (!stash.initialized) stash.value = compute() can potentially be called multiple times before the effect runs. If this is not a problem I don't see one either.

urugator commented 5 years ago

@jedwards1211

I don't really understand why this side effect during render would be more problematic for concurrent mode

Concurrent mode is not an issue. Problem is that effect runs after render, meaning that when deps are changed, there is one render pass during which memoized value is out of sync with the rest of the props. This can lead to bugs in render logic. Speaking of which I don't think your solution works since you update the value in effect, but you don't force the component to re-render with this new value. You should use useState as cache storage.

jedwards1211 commented 5 years ago

Yeah I realized the useEffect render sync issue. I was asking why a hard side effect like in @otakustay's example, instead of useEffect, would be any more of a problem for concurrent mode.

Or put another way, if all the complexity in @alexreardon's useMemoOne is necessary, or if @otakustay's implementation would suffice.

urugator commented 5 years ago

if all the complexity in @alexreardon's useMemoOne is necessary

I don't think so. The only problem with @otakustay solution is that useRef doesn't accept init function, so const value = useRef(compute); won't do the trick. Also the deps are unnecessarily compared on initial render. If you take a look at the solution I posted (gist) it's basically the same, but deals with this initial render init (most of the code is validation, actual logic is lines 70-81).

TimothyJones commented 5 years ago

I can't edit the issue, but I think the title is meant to be "cache purging" not "cache busting".

bertho-zero commented 4 years ago

@alexreardon What is the difference between the package use-memo-one and this code (by @otakustay):

const useMemoOne = (compute, deps, equalityFn = shallowEqual) => {
  const value = useRef(compute);
  const previousDeps = useRef(deps);

  if (!equalityFn(previousDeps, deps)) {
    previousDeps.current = deps;
    value.current = compute();
  }

  return value.current;
};

const useCallbackOne = (compute, deps, equalityFn) => useMemoOne(() => compute, deps, equalityFn);
steve-taylor commented 4 years ago

@bertho-zero compute is a function, but unlike useState, useRef doesn't call provided value, if it's a function, to compute a value. It uses the value as is, even if it's a function.

That means useRef(compute) should be useRef(compute()).

Now, the problem is that compute will be called on every render. This defeats the purpose of memoization, so you'll need a flag to stop this happening.

Also, I wouldn't change the semantics of hooks inputs, so equalityFn shouldn't be a thing here.

Also, there's another bug: You're comparing previousDeps instead of previousDept.current to deps.

Try this:

function useMemoOne(compute, deps) {
    const isNew = useRef(true);
    const value = useRef(isNew.current ? compute() : null);
    const previousDeps = useRef(deps);

    isNew.current = false;

    if (!(
        Array.isArray(deps)
        && Array.isArray(previousDeps.current)
        && deps.length === previousDeps.current.length
        && deps.every((dep, index) => dep === previousDeps.current[index]
    )) {
        previousDeps.current = deps;
        value.current = compute();
    }

    return value.current;
}
fabb commented 4 years ago

@steve-taylor isNew should not be necessary at all since useRef only executes its parameter once initially, so const value = useRef(isNew.current ? compute() : null); is equal to const value = useRef(compute());. EDIT: I was wrong on this one, see comments below.

function useMemoOne(compute, deps) {
    const value = useRef(compute());
    const previousDeps = useRef(deps);

    if (!(
        Array.isArray(deps)
        && Array.isArray(previousDeps.current)
        && deps.length === previousDeps.current.length
        && deps.every((dep, index) => dep === previousDeps.current[index]
    )) {
        previousDeps.current = deps;
        value.current = compute();
    }

    return value.current;
}

Additionally the deps comparison can be avoided on the initial render like @urugator mentioned:

function useMemoOne(compute, deps) {
    const value = useRef(null);
    const previousDeps = useRef(null);

    if (!(
        Array.isArray(deps)
        && Array.isArray(previousDeps.current)
        && deps.length === previousDeps.current.length
        && deps.every((dep, index) => dep === previousDeps.current[index]
    )) {
        previousDeps.current = deps;
        value.current = compute();
    }

    return value.current;
}

The only issue left is that when deps is undefined or null, the value would be computed every render. So after all maybe an extra flag is necessary indeed, because compute() might also return a falsy value and therefore the current value cannot be used to skip the if condition.

steve-taylor commented 4 years ago

@fabb there’s nothing magical about useRef that changes the way JavaScript works. For a function to be called, all its actual parameters have to be evaluated. This holds true whether the function chooses to use or ignore the parameters passed to it.

The change you made causes compute to be called on every render.

Edit: Your second version looks like it might work, because now you’re not evaluating compute every render.

fabb commented 4 years ago

@steve-taylor you are completely right, thanks.

I've written some unit tests to see what really happens:

import React, { useRef, DependencyList, FunctionComponent } from 'react'
import { render, wait, cleanup } from '@testing-library/react'
import '@testing-library/jest-dom/extend-expect'

afterEach(cleanup)

function useMemoOne<T>(compute: () => T, deps: DependencyList | undefined) {
    const value = useRef<T | null>(null)
    const previousDeps = useRef<DependencyList | undefined | null>(null)

    if (
        !(
            Array.isArray(deps) &&
            Array.isArray(previousDeps.current) &&
            deps.length === previousDeps.current!.length &&
            deps.every((dep, index) => dep === previousDeps.current![index])
        )
    ) {
        previousDeps.current = deps
        value.current = compute()
    }

    return value.current
}

const UseMemoTestComponent: FunctionComponent<{
    compute: () => { text: string }
    deps: DependencyList | undefined
}> = props => {
    const value = useMemoOne(props.compute, props.deps)

    return <div>{value?.text}</div>
}

describe('useMemoOne', () => {
    it('calls "compute" only when deps change', async () => {
        const mockedCallback = jest.fn<{ text: string }, []>()
        mockedCallback.mockReturnValue({ text: 'ok' })

        const sut = render(<UseMemoTestComponent compute={mockedCallback} deps={['1']} />)

        await wait(() => {
            expect(sut.queryByText('ok')).toBeInTheDocument()
            expect(mockedCallback.mock.calls).toHaveLength(1)
        })

        sut.rerender(<UseMemoTestComponent compute={mockedCallback} deps={['1']} />)

        await wait(() => {
            expect(sut.queryByText('ok')).toBeInTheDocument()
            expect(mockedCallback.mock.calls).toHaveLength(1) // mock has not been called again
        })

        sut.rerender(<UseMemoTestComponent compute={mockedCallback} deps={['2']} />)

        await wait(() => {
            expect(sut.queryByText('ok')).toBeInTheDocument()
            expect(mockedCallback.mock.calls).toHaveLength(2) // mock has been called once more
        })
    })

    it('does not call "compute" on every render when deps are undefined', async () => {
        const mockedCallback = jest.fn<{ text: string }, []>()
        mockedCallback.mockReturnValue({ text: 'ok' })

        const sut = render(<UseMemoTestComponent compute={mockedCallback} deps={undefined} />)

        await wait(() => {
            expect(sut.queryByText('ok')).toBeInTheDocument()
            expect(mockedCallback.mock.calls).toHaveLength(1)
        })

        sut.rerender(<UseMemoTestComponent compute={mockedCallback} deps={undefined} />)

        await wait(() => {
            expect(sut.queryByText('ok')).toBeInTheDocument()
            expect(mockedCallback.mock.calls).toHaveLength(1) // mock should be called again - FAILS
        })
    })
})

As expected the second test fails (same with the version that uses isNew).

Here is a codesandbox with the failing unit test: https://codesandbox.io/s/usememoone-test-entsz?fontsize=14&hidenavigation=1&module=%2Fsrc%2F__tests__%2FuseMemoOne.test.tsx&theme=dark

fabb commented 4 years ago

I have a different question: from this article and the vague documentation, I had the impression that React might keep not only the latest, but also previous values and deps arrays too, which could become a memory problem:

As a related note, if you have dependencies then it's quite possible React is hanging on to a reference to previous functions because memoization typically means that we keep copies of old values to return in the event we get the same dependencies as given previously. The especially astute of you will notice that this means React also has to hang on to a reference to the dependencies for this equality check (which incidentally is probably happening anyway thanks to your closure, but it's something worth mentioning anyway).

Source: https://kentcdodds.com/blog/usememo-and-usecallback

But unit tests could not verify that (see codesandbox).

Now my question is, can we take it for granted that React only keeps the latest value (cc @gaearon)? React keeping several previous values and deps arrays could cause quite some unwanted memory increase. It might make sense to improve the React documentation on these points. We're trying to pinpoint memory leaks in the new hooks-based styled-components (https://github.com/styled-components/styled-components/issues/2913) and the uses of useMemo have been suspects (accordingly to the tests injustified).

richardscarrott commented 3 years ago

Here is an alternative useMemoOne that seems to be concurrent mode safe 🤞

// @flow
import { useRef, useState, useEffect } from 'react';
import areInputsEqual from './are-inputs-equal';

type Result<T> = {|
  inputs: mixed[],
  result: T,
|};

export default function useMemoOne<T>(
  // getResult changes on every call,
  getResult: () => T,
  // the inputs array changes on every call
  inputs?: mixed[] = [],
): T {
  // using useState to generate initial value as it is lazy
  const initial: Result<T> = useState(() => ({
    inputs,
    result: getResult(),
  }))[0];

  const uncommitted = useRef<Result<T>>(initial);
  const committed = useRef<Result<T>>(initial);

  // persist any uncommitted changes
  useEffect(() => {
    committed.current = uncommitted.current;
  });

  if (areInputsEqual(inputs, committed.current.inputs)) {
    return committed.current.result;
  }

  uncommitted.current = {
    inputs,
    result: getResult(),
  };

  return uncommitted.current.result;
}

So is the above genuinely the simplest way to implement useMemo with a semantic guarantee safely?... surely all that complexity warrants inclusion in React itself or at least something in the React docs pointing to useMemoOne?

Unless maybe I'm doing something wrong, however I find I need to perform change detection on derived data quite often 🤔.

I wonder if users are generally getting away with useMemo as a semantic guarantee which is why useMemoOne is a little shy of React's 10 million weekly downloads 😅

skyboyer commented 3 years ago

Just a heads up that this is likely to be problematic in concurrent mode, since the function might be called many times with different props.

@aweary but does it also mean we cannot rely on useRef consistent updates as well when concurrent mode arrives? And all that useMemoOne will also be broken?

ps sorry for late call.

gmoniava commented 3 years ago

Functions returned from useCallback are often used as deps for useEffect. So semantic guarantee would be more important, isn't it? @gaearon (the docs about useCallbackshould also be more clear if same thing as useMemo applies to useCallback, isn't it?)

Here is a link to the same concern as I described above.

pindjur commented 3 years ago

We also use function returned from useCallback as dependency to useEffect for data fetching. Memoized function often comes from another custom hook. Cache busting will cause state to be overwriten.

loucadufault commented 7 months ago

Not sure if it's been mentioned, but useState is suitable for one-time initializations that do not have dependencies, e.g. const [myStableUuid] = useState(uuidv4)