Avoid excessive `cache.watch` / unsubscribe on every render in `useFragment` #11464

jerelmiller · 2024-01-05T17:25:15Z

While looking through useFragment, I happened to notice that the subscribe callback passed to useSyncExternalStore was not wrapped in a useCallback. Because of this, useFragment will unsubscribe from the previous cache.watch and resubscribe with a cache.watch on every render. While cache.watch should be fairly low overhead, there is no need to have to unsubscribe/resubscribe on every render and instead the cache.watch should last until either the cache has been changed, or the options have changed.

Checklist:

If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
Make sure all of the significant new logic is covered by tests

changeset-bot · 2024-01-05T17:25:19Z

🦋 Changeset detected

Latest commit: 524aae7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@apollo/client	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-01-05T17:26:38Z

size-limit report 📦

Path	Size
dist/apollo-client.min.cjs	37.04 KB (+0.09% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs"	43.5 KB (+0.08% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production)	41.99 KB (+0.1% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js"	32.54 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production)	31.23 KB (0%)
import { ApolloProvider } from "dist/react/index.js"	1.21 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production)	1.2 KB (0%)
import { useQuery } from "dist/react/index.js"	4.27 KB (0%)
import { useQuery } from "dist/react/index.js" (production)	4.08 KB (0%)
import { useLazyQuery } from "dist/react/index.js"	4.58 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production)	4.39 KB (0%)
import { useMutation } from "dist/react/index.js"	2.55 KB (0%)
import { useMutation } from "dist/react/index.js" (production)	2.53 KB (0%)
import { useSubscription } from "dist/react/index.js"	2.23 KB (0%)
import { useSubscription } from "dist/react/index.js" (production)	2.19 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js"	4.6 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" (production)	4.04 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js"	4.12 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" (production)	3.54 KB (0%)
import { useReadQuery } from "dist/react/index.js"	2.98 KB (+0.04% 🔺)
import { useReadQuery } from "dist/react/index.js" (production)	2.92 KB (0%)
import { useFragment } from "dist/react/index.js"	2.17 KB (+3.8% 🔺)
import { useFragment } from "dist/react/index.js" (production)	2.11 KB (+3.75% 🔺)

jerelmiller · 2024-01-05T17:28:50Z

src/react/hooks/useFragment.ts

-        clearTimeout(lastTimeout);
-      };
-    },
+    React.useCallback(


Oddly, this change seems to now throw off the timing of the update between useQuery and useFragment when used in the same component.

Turns out we had tried to re-render a 2nd time in useFragment before the first had finished, so a lingering setTimeout had been queued before our cache.writeQuery in the test. Because of this, useFragment would re-render before useQuery and threw of the timing. I was able to fix this in 6c1372c by cancelling the timeout from the previous attempt in case it hadn't yet flushed before queuing another attempt at the render.

Turns out this change also makes the timing in React 17 and 18 the same. I was able to remove the additional React version check in the test.

jerelmiller · 2024-01-05T20:51:42Z

src/react/hooks/internal/useLazyRef.ts

+const INIT = {};
+
+export function useLazyRef<T>(getInitialValue: () => T) {
+  const ref = React.useRef<T>(INIT as unknown as T);


Comparing against this INIT sentinel here instead of something like undefined makes sure that we can still set undefined as a value on subsequent renders without re-running the init function.

jerelmiller · 2024-01-05T20:57:47Z

src/react/hooks/useFragment.ts

-        resultRef.current
-      : (resultRef.current = latestDiffToResult);
-  };
+  const getSnapshot = React.useCallback(() => resultRef.current, []);


This prevents React from doing extra work when the getSnapshot function isn't stable across renders.

jerelmiller · 2024-01-05T20:58:30Z

src/react/hooks/useFragment.ts


-  const resultRef = React.useRef<UseFragmentResult<TData>>();
-  let latestDiff = cache.diff<TData>(diffOptions);
+  const resultRef = useLazyRef<UseFragmentResult<TData>>(() =>


Introducing this useLazyRef avoids the need to call cache.diff on every render which should give a small perf benefit.

Did you check if we can utilize this in other places in our codebase?

I just did a quick glance through some of our other hooks. There is only one other place right now that could potentially utilize it, which is the useInternalState hook in useQuery that initializes the InternalState instance (would still need some of that if statement though, so not sure what the bundle size diff would be)

jerelmiller · 2024-01-05T21:01:40Z

src/react/hooks/useFragment.ts

-        clearTimeout(lastTimeout);
-      };
-    },
+    React.useCallback(


Turns out this change also makes the timing in React 17 and 18 the same. I was able to remove the additional React version check in the test.

phryneas

Looks very good - thank you for digging into this! :)

src/react/hooks/useFragment.ts

phryneas · 2024-01-08T10:54:23Z

src/react/hooks/useFragment.ts


-  const resultRef = React.useRef<UseFragmentResult<TData>>();
-  let latestDiff = cache.diff<TData>(diffOptions);
+  const resultRef = useLazyRef<UseFragmentResult<TData>>(() =>


Did you check if we can utilize this in other places in our codebase?

jerelmiller added 8 commits January 4, 2024 12:04

Create a useLazyRef hook

4566b34

Wrap diffOptions in useDeepMemo

6652d94

Compare results in resultRef.current

f1ea3a1

Use resultRef.current in getSnapshot

180fe81

Only call cache.diff when there is no result ref

6a62bed

useLazyRef to get initial diff result

c08a56c

Make getSnapshot a stable function reference

30aae50

Wrap useSyncExternalStore subscribe function in useCallback

5e1e8a2

Move diff options type to useDeepMemo generic arg

c0b6166

jerelmiller commented Jan 5, 2024

View reviewed changes

jerelmiller added 5 commits January 5, 2024 13:45

Clear timeout in callback when trying to update again

6c1372c

Add comment about clearTimeout

f51d7ad

No need for optional chaining

087fc4c

Add changeset

ecf9427

Update size limits

0d88921

jerelmiller changed the title ~~[WIP] Avoid excessive cache.watch / unsubscribe on every render in useFragment~~ Avoid excessive cache.watch / unsubscribe on every render in useFragment Jan 5, 2024

jerelmiller marked this pull request as ready for review January 5, 2024 20:55

jerelmiller requested review from phryneas and alessbell January 5, 2024 20:55

jerelmiller added ⚛️ React-related 🥚 backwards-compatible for PRs that do not introduce any breaking changes 🐎 performance 👩‍🏭 refactor labels Jan 5, 2024

Remove unneeded extra check for React 17 in useFragment tests

a10fc98

jerelmiller commented Jan 5, 2024

View reviewed changes

phryneas approved these changes Jan 8, 2024

View reviewed changes

github-actions bot added auto-cleanup 🤖 labels Jan 8, 2024

jerelmiller added 2 commits January 8, 2024 23:25

Fix typo in variable name

58384c3

Merge branch 'main' into jerel/more-optimal-usefragment

524aae7

jerelmiller merged commit aac12b2 into main Jan 9, 2024
25 checks passed

jerelmiller deleted the jerel/more-optimal-usefragment branch January 9, 2024 06:37

github-actions bot mentioned this pull request Jan 9, 2024

Version Packages #11469

Merged

jerelmiller mentioned this pull request Feb 1, 2024

Update import to rehackt in useLazyRef #11552

Merged

github-actions bot locked as resolved and limited conversation to collaborators Feb 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid excessive `cache.watch` / unsubscribe on every render in `useFragment` #11464

Avoid excessive `cache.watch` / unsubscribe on every render in `useFragment` #11464

jerelmiller commented Jan 5, 2024 •

edited

Loading

changeset-bot bot commented Jan 5, 2024 •

edited

Loading

github-actions bot commented Jan 5, 2024 •

edited

Loading

jerelmiller Jan 5, 2024

jerelmiller Jan 5, 2024

jerelmiller Jan 5, 2024

jerelmiller Jan 5, 2024 •

edited

Loading

jerelmiller Jan 5, 2024

jerelmiller Jan 5, 2024

phryneas Jan 8, 2024

jerelmiller Jan 9, 2024

jerelmiller Jan 5, 2024

phryneas left a comment

phryneas Jan 8, 2024

Avoid excessive cache.watch / unsubscribe on every render in useFragment #11464

Avoid excessive cache.watch / unsubscribe on every render in useFragment #11464

Conversation

jerelmiller commented Jan 5, 2024 • edited Loading

Checklist:

changeset-bot bot commented Jan 5, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Jan 5, 2024 • edited Loading

size-limit report 📦

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerelmiller Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phryneas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avoid excessive `cache.watch` / unsubscribe on every render in `useFragment` #11464

Avoid excessive `cache.watch` / unsubscribe on every render in `useFragment` #11464

jerelmiller commented Jan 5, 2024 •

edited

Loading

changeset-bot bot commented Jan 5, 2024 •

edited

Loading

github-actions bot commented Jan 5, 2024 •

edited

Loading

jerelmiller Jan 5, 2024 •

edited

Loading