Add `reset` method to `DocumentTransform`, hook `InMemoryCache.addTypenameTransform` up to `InMemoryCache.gc` #11344

phryneas · 2023-11-06T15:17:38Z

An alternative implementation here would be to leave the reset method off DocumentTransform and just re-create the transform when .gc is called.
@jerelmiller what are your thoughts?

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

hook `InMemoryCache.addTypenameTransform` up to `InMemoryCache.gc`

changeset-bot · 2023-11-06T15:17:42Z

🦋 Changeset detected

Latest commit: 4dbe10b

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 · 2023-11-06T15:19:32Z

size-limit report 📦

Path	Size
dist/apollo-client.min.cjs	37.23 KB (+0.07% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs"	43.71 KB (+0.09% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production)	42.18 KB (+0.06% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js"	32.75 KB (+0.12% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production)	31.41 KB (+0.1% 🔺)
import { ApolloProvider } from "dist/react/index.js"	1.28 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production)	1.26 KB (0%)
import { useQuery } from "dist/react/index.js"	4.34 KB (0%)
import { useQuery } from "dist/react/index.js" (production)	4.16 KB (0%)
import { useLazyQuery } from "dist/react/index.js"	4.65 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production)	4.47 KB (0%)
import { useMutation } from "dist/react/index.js"	2.6 KB (0%)
import { useMutation } from "dist/react/index.js" (production)	2.58 KB (0%)
import { useSubscription } from "dist/react/index.js"	2.29 KB (0%)
import { useSubscription } from "dist/react/index.js" (production)	2.25 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js"	4.33 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" (production)	3.79 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js"	3.83 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" (production)	3.27 KB (0%)
import { useReadQuery } from "dist/react/index.js"	3.05 KB (0%)
import { useReadQuery } from "dist/react/index.js" (production)	3 KB (0%)
import { useFragment } from "dist/react/index.js"	2.15 KB (0%)
import { useFragment } from "dist/react/index.js" (production)	2.1 KB (0%)

jerelmiller

I see no reason not to want these changes so this is great! I had a couple very minor suggestions in regards to the name and behavior, but would be glad to get this in.

jerelmiller · 2023-11-06T17:36:42Z

src/utilities/graphql/DocumentTransform.ts

@@ -80,6 +80,11 @@ export class DocumentTransform {
    }
  }

+  reset() {


Suggested change

reset() {

resetCache() {

Could we be a bit more explicit in the name here? I'd like to make it clear what is being reset here.

Hmm.. I'd like to bounce this back - we already have a bunch of reset methods and I'd hate to have two naming conventions for the same thing without a good reason. What about a DocBlock?

The only counterpoint I have here is that most of the other usages of reset are typically on internal-facing utilities (i.e. canonicalStringify). Sure, they are available for external use as well, but likely aren't. The other point here is that the document cache is very much a part of the API and called out in public documentation, so connecting the two makes more sense. Stuff like print.reset() or canonicalStringify.reset don't call out the internal cache in the public API, so I think those names are appropriate there.

This API is specifically designed for external use and I felt like the resetCache more explictly described what we are resetting here. That and we don't always have a consistent name for this anyways. For example, we have cache.gc, client.resetStore, and client.clearStore.

Am I going to die on this hill? Definitely not and am not entirely opposed to plain reset. Really I just look at it as DocumentTransform.resetCache() is more self-explanatory than DocumentTransform.reset() without resorting to documentation.

Feel free to use your best judgement. I'll be happy with whatever you choose. Wanted to at least poke at this a little more so you understand where I'm coming from.

I've taken another look - we have reset, resetCaching, resetCaches, resetResultCache, resetCanon and probably a few more, so my argument about consistency is pretty moot. I'll rename it :)

jerelmiller · 2023-11-06T17:39:04Z

src/utilities/graphql/DocumentTransform.ts

@@ -80,6 +80,11 @@ export class DocumentTransform {
    }
  }

+  reset() {
+    this.stableCacheKeys =
+      this.stableCacheKeys && new Trie(canUseWeakMap, (key) => ({ key }));


I think we should also reset the resultCache here as well. That resultCache is essentially just a way for us to record the final transformed documents so that if you try and transform an already transformed doc, you get the same object back.

If we are clearing the cached documents by key, that resultCache will be stuck with outdated objects that are likely unreachable, so no need to keep them around either. Best to start with a clean slate there.

Hmmm...
On second look, I'm not 100% sure. Currently, the resultCache does something, even if the cache option is disabled, so doing this would change the observed functionality of the DocumentTransform in an unintuitive way (if the users have their own memoization, before their transform would not be called, now it suddenly would).

On the other hand, assuming they have WeakSets available (and at this point, we really should, and we should remove canUseWeakSet in 4.0 and have users provide a polyfill instead), nothing will be kept around if it's unreachable in the first place. So not sure if this even ever realistically needs a cleanup.

I guess the question is the full purpose of this function. From my understanding, the other usages of reset that you're adding for other APIs are meant as a lever for users to completely flush caches and start over, in case of memory overhead. For example, the print function uses a WeakMap for its cache, yet it still has a reset function to recreate it.

Perhaps we need a distinction between the two?

An option could be that we provide both a resetCache function that just touches the internal cache (i.e. stableCacheKeys), and another reset function that acts more like all the other reset functions you're creating that completely flushes everything. Feels a bit heavy and unnecessary, but just trying to make sure if we keep a reset function that it behaves like the others do.

Thoughts?

I think the distinction here is between a WeakMap and a WeakSet:

A WeakMap will hold a (potentially big) object in cache if the key value still has a reference in memory. So if the original object stays in memory (and there are lots of them), you get a memory leak.

A WeakSet doesn't really do this - it's just a collection of pointers that might at some point in time fade away - I'd say that it's almost impossible to get a measurable negative memory impact from this - especially if you compare it with the memory impact of the objects that the references point to.

At that point, I think the value you'd get from "we've actually created this and we don't need to create a new object from it again" might be higher than ever emptying the WeakSet - if it prevents one DocumentNode from being created, ever, it will probably have paid for itself.

I also have the nagging feeling that it might actually cause bugs to run the same DocumentNode through a transform twice while resetting the resultCache in-between: you might end up with very different result nodes, depending if you reset the resultCache in-between or not (at this point it's no longer "this is idempotent and will just take a moment longer if we reset the cache"), and I kinda want to avoid subtle bugs like that at all cost.

Thats fair. I'm ok leaving this then. Appreciate you talking through this with me!

…nsform-reset

jerelmiller

🎉

.changeset/hot-ducks-burn.md

jerelmiller · 2023-11-08T17:55:14Z

src/utilities/graphql/DocumentTransform.ts

@@ -80,6 +80,11 @@ export class DocumentTransform {
    }
  }

+  reset() {
+    this.stableCacheKeys =
+      this.stableCacheKeys && new Trie(canUseWeakMap, (key) => ({ key }));


Thats fair. I'm ok leaving this then. Appreciate you talking through this with me!

Co-authored-by: Jerel Miller <[email protected]>

…nsform-reset

Add reset method to DocumentTransform, ...

54f770d

hook `InMemoryCache.addTypenameTransform` up to `InMemoryCache.gc`

phryneas requested review from jerelmiller and alessbell November 6, 2023 15:17

size

a86a3da

jerelmiller approved these changes Nov 6, 2023

View reviewed changes

phryneas added 3 commits November 7, 2023 10:36

add docblock

55588db

rename reset to resetCache

4526c4b

Merge remote-tracking branch 'origin/release-3.9' into pr/documentTra…

3487742

…nsform-reset

jerelmiller approved these changes Nov 8, 2023

View reviewed changes

phryneas and others added 2 commits November 9, 2023 10:22

Update .changeset/hot-ducks-burn.md

e593c2e

Co-authored-by: Jerel Miller <[email protected]>

Merge remote-tracking branch 'origin/release-3.9' into pr/documentTra…

4dbe10b

…nsform-reset

phryneas merged commit bd26676 into release-3.9 Nov 9, 2023
6 checks passed

github-actions bot mentioned this pull request Nov 9, 2023

Version Packages (alpha) #11353

Merged

phryneas deleted the pr/documentTransform-reset branch November 9, 2023 10:21

phryneas added this to the MemoryAnalysis milestone Nov 9, 2023

github-actions bot locked as resolved and limited conversation to collaborators Dec 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `reset` method to `DocumentTransform`, hook `InMemoryCache.addTypenameTransform` up to `InMemoryCache.gc` #11344

Add `reset` method to `DocumentTransform`, hook `InMemoryCache.addTypenameTransform` up to `InMemoryCache.gc` #11344

phryneas commented Nov 6, 2023

changeset-bot bot commented Nov 6, 2023 •

edited

Loading

github-actions bot commented Nov 6, 2023 •

edited

Loading

jerelmiller left a comment

jerelmiller Nov 6, 2023

phryneas Nov 7, 2023

jerelmiller Nov 7, 2023

phryneas Nov 8, 2023

jerelmiller Nov 6, 2023

phryneas Nov 7, 2023

jerelmiller Nov 7, 2023 •

edited

Loading

phryneas Nov 8, 2023 •

edited

Loading

jerelmiller Nov 8, 2023

jerelmiller left a comment

jerelmiller Nov 8, 2023

Add reset method to DocumentTransform, hook InMemoryCache.addTypenameTransform up to InMemoryCache.gc #11344

Add reset method to DocumentTransform, hook InMemoryCache.addTypenameTransform up to InMemoryCache.gc #11344

Conversation

phryneas commented Nov 6, 2023

Checklist:

changeset-bot bot commented Nov 6, 2023 • edited Loading

🦋 Changeset detected

github-actions bot commented Nov 6, 2023 • edited Loading

size-limit report 📦

jerelmiller left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerelmiller Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

phryneas Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerelmiller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `reset` method to `DocumentTransform`, hook `InMemoryCache.addTypenameTransform` up to `InMemoryCache.gc` #11344

Add `reset` method to `DocumentTransform`, hook `InMemoryCache.addTypenameTransform` up to `InMemoryCache.gc` #11344

changeset-bot bot commented Nov 6, 2023 •

edited

Loading

github-actions bot commented Nov 6, 2023 •

edited

Loading

jerelmiller Nov 7, 2023 •

edited

Loading

phryneas Nov 8, 2023 •

edited

Loading