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.

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

useMemo / useCallback cache busting opt out #15278

Open
alexreardon opened this issue Mar 31, 2019 · 37 comments
Open

useMemo / useCallback cache busting opt out #15278

alexreardon opened this issue Mar 31, 2019 · 37 comments

Comments

@alexreardon
Copy link

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 atlassian/react-beautiful-dnd#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

@alexreardon alexreardon changed the title useMemo / useCallback cache useMemo / useCallback cache busting opt out Mar 31, 2019
@gaearon
Copy link
Collaborator

gaearon commented Mar 31, 2019

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
Copy link
Author

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
Copy link
Author

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

@ntucker
Copy link

ntucker commented Mar 31, 2019

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

@alexreardon
Copy link
Author

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
Copy link

ntucker commented Mar 31, 2019

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

@aweary
Copy link
Contributor

aweary commented Apr 1, 2019

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
Copy link
Author

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
Copy link

ntucker commented Apr 1, 2019

@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
Copy link
Author

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
Copy link

urugator commented Apr 1, 2019

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
Copy link
Author

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
Copy link
Contributor

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
Copy link

skyboyer commented Apr 4, 2019

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

@otakustay
Copy link
Contributor

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
Copy link

@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
Copy link
Contributor

@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
Copy link
Contributor

jedwards1211 commented Apr 16, 2019

@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
Copy link
Contributor

jedwards1211 commented Apr 16, 2019

@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
Copy link
Contributor

@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
Copy link
Contributor

jedwards1211 commented Apr 16, 2019

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
Copy link
Collaborator

eps1lon commented Apr 16, 2019

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
Copy link

@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
Copy link
Contributor

jedwards1211 commented Apr 16, 2019

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
Copy link

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
Copy link

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

@bertho-zero
Copy link

@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
Copy link

@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
Copy link

fabb commented Jan 4, 2020

@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
Copy link

steve-taylor commented Jan 4, 2020

@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
Copy link

fabb commented Jan 4, 2020

@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
Copy link

fabb commented Jan 4, 2020

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 (styled-components/styled-components#2913) and the uses of useMemo have been suspects (accordingly to the tests injustified).

@richardscarrott
Copy link
Contributor

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
Copy link

skyboyer commented Apr 2, 2021

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
Copy link

gmoniava commented Apr 9, 2021

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 useCallback should 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
Copy link

pindjur commented May 15, 2021

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
Copy link

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests