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.

Sign up for GitHub

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

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

Merged
merged 7 commits into from
Nov 9, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
54f770d
Add `reset` method to `DocumentTransform`, ...
phryneas Nov 6, 2023
a86a3da
size
phryneas Nov 6, 2023
55588db
add docblock
phryneas Nov 7, 2023
4526c4b
rename `reset` to `resetCache`
phryneas Nov 8, 2023
3487742
Merge remote-tracking branch 'origin/release-3.9' into pr/documentTra…
phryneas Nov 8, 2023
e593c2e
Update .changeset/hot-ducks-burn.md
phryneas Nov 9, 2023
4dbe10b
Merge remote-tracking branch 'origin/release-3.9' into pr/documentTra…
phryneas Nov 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .api-reports/api-report-core.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -601,6 +601,8 @@ export class DocumentTransform {
// (undocumented)
static identity(): DocumentTransform;
// (undocumented)
resetCache(): void;
// (undocumented)
static split(predicate: (document: DocumentNode) => boolean, left: DocumentTransform, right?: DocumentTransform): DocumentTransform;
// (undocumented)
transformDocument(document: DocumentNode): DocumentNode;
Expand Down
4 changes: 3 additions & 1 deletion .api-reports/api-report-react.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -728,6 +728,8 @@ class DocumentTransform {
// (undocumented)
static identity(): DocumentTransform;
// (undocumented)
resetCache(): void;
// (undocumented)
static split(predicate: (document: DocumentNode) => boolean, left: DocumentTransform, right?: DocumentTransform): DocumentTransform;
// (undocumented)
transformDocument(document: DocumentNode): DocumentNode;
Expand Down Expand Up @@ -2221,7 +2223,7 @@ interface WatchQueryOptions<TVariables extends OperationVariables = OperationVar
// src/core/watchQueryOptions.ts:191:3 - (ae-forgotten-export) The symbol "UpdateQueryFn" needs to be exported by the entry point index.d.ts
// src/react/hooks/useBackgroundQuery.ts:24:3 - (ae-forgotten-export) The symbol "FetchMoreFunction" needs to be exported by the entry point index.d.ts
// src/react/hooks/useBackgroundQuery.ts:25:3 - (ae-forgotten-export) The symbol "RefetchFunction" needs to be exported by the entry point index.d.ts
// src/utilities/graphql/DocumentTransform.ts:122:7 - (ae-forgotten-export) The symbol "DocumentTransformCacheKey" needs to be exported by the entry point index.d.ts
// src/utilities/graphql/DocumentTransform.ts:130:7 - (ae-forgotten-export) The symbol "DocumentTransformCacheKey" needs to be exported by the entry point index.d.ts

// (No @packageDocumentation comment for this package)

Expand Down
4 changes: 3 additions & 1 deletion .api-reports/api-report-react_components.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -635,6 +635,8 @@ class DocumentTransform {
// (undocumented)
static identity(): DocumentTransform;
// (undocumented)
resetCache(): void;
// (undocumented)
static split(predicate: (document: DocumentNode) => boolean, left: DocumentTransform, right?: DocumentTransform): DocumentTransform;
// (undocumented)
transformDocument(document: DocumentNode): DocumentNode;
Expand Down Expand Up @@ -1753,7 +1755,7 @@ interface WatchQueryOptions<TVariables extends OperationVariables = OperationVar
// src/core/types.ts:178:3 - (ae-forgotten-export) The symbol "MutationQueryReducer" needs to be exported by the entry point index.d.ts
// src/core/types.ts:205:5 - (ae-forgotten-export) The symbol "Resolver" needs to be exported by the entry point index.d.ts
// src/core/watchQueryOptions.ts:191:3 - (ae-forgotten-export) The symbol "UpdateQueryFn" needs to be exported by the entry point index.d.ts
// src/utilities/graphql/DocumentTransform.ts:122:7 - (ae-forgotten-export) The symbol "DocumentTransformCacheKey" needs to be exported by the entry point index.d.ts
// src/utilities/graphql/DocumentTransform.ts:130:7 - (ae-forgotten-export) The symbol "DocumentTransformCacheKey" needs to be exported by the entry point index.d.ts

// (No @packageDocumentation comment for this package)

Expand Down
4 changes: 3 additions & 1 deletion .api-reports/api-report-react_context.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -618,6 +618,8 @@ class DocumentTransform {
// (undocumented)
static identity(): DocumentTransform;
// (undocumented)
resetCache(): void;
// (undocumented)
static split(predicate: (document: DocumentNode) => boolean, left: DocumentTransform, right?: DocumentTransform): DocumentTransform;
// (undocumented)
transformDocument(document: DocumentNode): DocumentNode;
Expand Down Expand Up @@ -1649,7 +1651,7 @@ interface WatchQueryOptions<TVariables extends OperationVariables = OperationVar
// src/core/types.ts:178:3 - (ae-forgotten-export) The symbol "MutationQueryReducer" needs to be exported by the entry point index.d.ts
// src/core/types.ts:205:5 - (ae-forgotten-export) The symbol "Resolver" needs to be exported by the entry point index.d.ts
// src/core/watchQueryOptions.ts:191:3 - (ae-forgotten-export) The symbol "UpdateQueryFn" needs to be exported by the entry point index.d.ts
// src/utilities/graphql/DocumentTransform.ts:122:7 - (ae-forgotten-export) The symbol "DocumentTransformCacheKey" needs to be exported by the entry point index.d.ts
// src/utilities/graphql/DocumentTransform.ts:130:7 - (ae-forgotten-export) The symbol "DocumentTransformCacheKey" needs to be exported by the entry point index.d.ts

// (No @packageDocumentation comment for this package)

Expand Down
4 changes: 3 additions & 1 deletion .api-reports/api-report-react_hoc.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,6 +620,8 @@ class DocumentTransform {
// (undocumented)
static identity(): DocumentTransform;
// (undocumented)
resetCache(): void;
// (undocumented)
static split(predicate: (document: DocumentNode) => boolean, left: DocumentTransform, right?: DocumentTransform): DocumentTransform;
// (undocumented)
transformDocument(document: DocumentNode): DocumentNode;
Expand Down Expand Up @@ -1694,7 +1696,7 @@ export function withSubscription<TProps extends TGraphQLVariables | {} = {}, TDa
// src/core/types.ts:178:3 - (ae-forgotten-export) The symbol "MutationQueryReducer" needs to be exported by the entry point index.d.ts
// src/core/types.ts:205:5 - (ae-forgotten-export) The symbol "Resolver" needs to be exported by the entry point index.d.ts
// src/core/watchQueryOptions.ts:191:3 - (ae-forgotten-export) The symbol "UpdateQueryFn" needs to be exported by the entry point index.d.ts
// src/utilities/graphql/DocumentTransform.ts:122:7 - (ae-forgotten-export) The symbol "DocumentTransformCacheKey" needs to be exported by the entry point index.d.ts
// src/utilities/graphql/DocumentTransform.ts:130:7 - (ae-forgotten-export) The symbol "DocumentTransformCacheKey" needs to be exported by the entry point index.d.ts

// (No @packageDocumentation comment for this package)

Expand Down
4 changes: 3 additions & 1 deletion .api-reports/api-report-react_hooks.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -699,6 +699,8 @@ class DocumentTransform {
// (undocumented)
static identity(): DocumentTransform;
// (undocumented)
resetCache(): void;
// (undocumented)
static split(predicate: (document: DocumentNode) => boolean, left: DocumentTransform, right?: DocumentTransform): DocumentTransform;
// (undocumented)
transformDocument(document: DocumentNode): DocumentNode;
Expand Down Expand Up @@ -2115,7 +2117,7 @@ interface WatchQueryOptions<TVariables extends OperationVariables = OperationVar
// src/core/watchQueryOptions.ts:191:3 - (ae-forgotten-export) The symbol "UpdateQueryFn" needs to be exported by the entry point index.d.ts
// src/react/hooks/useBackgroundQuery.ts:24:3 - (ae-forgotten-export) The symbol "FetchMoreFunction" needs to be exported by the entry point index.d.ts
// src/react/hooks/useBackgroundQuery.ts:25:3 - (ae-forgotten-export) The symbol "RefetchFunction" needs to be exported by the entry point index.d.ts
// src/utilities/graphql/DocumentTransform.ts:122:7 - (ae-forgotten-export) The symbol "DocumentTransformCacheKey" needs to be exported by the entry point index.d.ts
// src/utilities/graphql/DocumentTransform.ts:130:7 - (ae-forgotten-export) The symbol "DocumentTransformCacheKey" needs to be exported by the entry point index.d.ts

// (No @packageDocumentation comment for this package)

Expand Down
4 changes: 3 additions & 1 deletion .api-reports/api-report-react_ssr.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,8 @@ class DocumentTransform {
// (undocumented)
static identity(): DocumentTransform;
// (undocumented)
resetCache(): void;
// (undocumented)
static split(predicate: (document: DocumentNode) => boolean, left: DocumentTransform, right?: DocumentTransform): DocumentTransform;
// (undocumented)
transformDocument(document: DocumentNode): DocumentNode;
Expand Down Expand Up @@ -1635,7 +1637,7 @@ interface WatchQueryOptions<TVariables extends OperationVariables = OperationVar
// src/core/types.ts:178:3 - (ae-forgotten-export) The symbol "MutationQueryReducer" needs to be exported by the entry point index.d.ts
// src/core/types.ts:205:5 - (ae-forgotten-export) The symbol "Resolver" needs to be exported by the entry point index.d.ts
// src/core/watchQueryOptions.ts:191:3 - (ae-forgotten-export) The symbol "UpdateQueryFn" needs to be exported by the entry point index.d.ts
// src/utilities/graphql/DocumentTransform.ts:122:7 - (ae-forgotten-export) The symbol "DocumentTransformCacheKey" needs to be exported by the entry point index.d.ts
// src/utilities/graphql/DocumentTransform.ts:130:7 - (ae-forgotten-export) The symbol "DocumentTransformCacheKey" needs to be exported by the entry point index.d.ts

// (No @packageDocumentation comment for this package)

Expand Down
4 changes: 3 additions & 1 deletion .api-reports/api-report-testing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -582,6 +582,8 @@ class DocumentTransform {
// (undocumented)
static identity(): DocumentTransform;
// (undocumented)
resetCache(): void;
// (undocumented)
static split(predicate: (document: DocumentNode) => boolean, left: DocumentTransform, right?: DocumentTransform): DocumentTransform;
// (undocumented)
transformDocument(document: DocumentNode): DocumentNode;
Expand Down Expand Up @@ -1698,7 +1700,7 @@ export function withWarningSpy<TArgs extends any[], TResult>(it: (...args: TArgs
// src/core/types.ts:178:3 - (ae-forgotten-export) The symbol "MutationQueryReducer" needs to be exported by the entry point index.d.ts
// src/core/types.ts:205:5 - (ae-forgotten-export) The symbol "Resolver" needs to be exported by the entry point index.d.ts
// src/core/watchQueryOptions.ts:191:3 - (ae-forgotten-export) The symbol "UpdateQueryFn" needs to be exported by the entry point index.d.ts
// src/utilities/graphql/DocumentTransform.ts:122:7 - (ae-forgotten-export) The symbol "DocumentTransformCacheKey" needs to be exported by the entry point index.d.ts
// src/utilities/graphql/DocumentTransform.ts:130:7 - (ae-forgotten-export) The symbol "DocumentTransformCacheKey" needs to be exported by the entry point index.d.ts

// (No @packageDocumentation comment for this package)

Expand Down
4 changes: 3 additions & 1 deletion .api-reports/api-report-testing_core.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,6 +581,8 @@ class DocumentTransform {
// (undocumented)
static identity(): DocumentTransform;
// (undocumented)
resetCache(): void;
// (undocumented)
static split(predicate: (document: DocumentNode) => boolean, left: DocumentTransform, right?: DocumentTransform): DocumentTransform;
// (undocumented)
transformDocument(document: DocumentNode): DocumentNode;
Expand Down Expand Up @@ -1654,7 +1656,7 @@ export function withWarningSpy<TArgs extends any[], TResult>(it: (...args: TArgs
// src/core/types.ts:178:3 - (ae-forgotten-export) The symbol "MutationQueryReducer" needs to be exported by the entry point index.d.ts
// src/core/types.ts:205:5 - (ae-forgotten-export) The symbol "Resolver" needs to be exported by the entry point index.d.ts
// src/core/watchQueryOptions.ts:191:3 - (ae-forgotten-export) The symbol "UpdateQueryFn" needs to be exported by the entry point index.d.ts
// src/utilities/graphql/DocumentTransform.ts:122:7 - (ae-forgotten-export) The symbol "DocumentTransformCacheKey" needs to be exported by the entry point index.d.ts
// src/utilities/graphql/DocumentTransform.ts:130:7 - (ae-forgotten-export) The symbol "DocumentTransformCacheKey" needs to be exported by the entry point index.d.ts

// (No @packageDocumentation comment for this package)

Expand Down
2 changes: 2 additions & 0 deletions .api-reports/api-report-utilities.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -759,6 +759,8 @@ export class DocumentTransform {
// (undocumented)
static identity(): DocumentTransform;
// (undocumented)
resetCache(): void;
// (undocumented)
static split(predicate: (document: DocumentNode) => boolean, left: DocumentTransform, right?: DocumentTransform): DocumentTransform;
// (undocumented)
transformDocument(document: DocumentNode): DocumentNode;
Expand Down
2 changes: 2 additions & 0 deletions .api-reports/api-report.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -751,6 +751,8 @@ export class DocumentTransform {
// (undocumented)
static identity(): DocumentTransform;
// (undocumented)
resetCache(): void;
// (undocumented)
static split(predicate: (document: DocumentNode) => boolean, left: DocumentTransform, right?: DocumentTransform): DocumentTransform;
// (undocumented)
transformDocument(document: DocumentNode): DocumentNode;
Expand Down
5 changes: 5 additions & 0 deletions .changeset/hot-ducks-burn.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/client": patch
---

Add a `resetCache` method to `DocumentTransform` and hook `InMemoryCache.addTypenameTransform` up to `InMemoryCache.gc`
4 changes: 2 additions & 2 deletions .size-limit.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
const checks = [
{
path: "dist/apollo-client.min.cjs",
limit: "38101",
limit: "38124",
},
{
path: "dist/main.cjs",
Expand All @@ -10,7 +10,7 @@ const checks = [
{
path: "dist/index.js",
import: "{ ApolloClient, InMemoryCache, HttpLink }",
limit: "32132",
limit: "32162",
},
...[
"ApolloProvider",
Expand Down
1 change: 1 addition & 0 deletions src/cache/inmemory/inMemoryCache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,7 @@ export class InMemoryCache extends ApolloCache<NormalizedCacheObject> {
}) {
canonicalStringify.reset();
print.reset();
this.addTypenameTransform.resetCache();
const ids = this.optimisticData.gc();
if (options && !this.txCount) {
if (options.resetResultCache) {
Expand Down
8 changes: 8 additions & 0 deletions src/utilities/graphql/DocumentTransform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,14 @@ export class DocumentTransform {
}
}

/**
* Resets the internal cache of this transform, if it has one.
*/
resetCache() {
this.stableCacheKeys =
this.stableCacheKeys && new Trie(canUseWeakMap, (key) => ({ key }));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@jerelmiller jerelmiller Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@phryneas phryneas Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

transformDocument(document: DocumentNode) {
// If a user passes an already transformed result back to this function,
// immediately return it.
Expand Down
Loading