From 482f4d822d9538e4fcd3839a4cbeb2a67e415b6c Mon Sep 17 00:00:00 2001 From: Sofian Hnaide Date: Mon, 3 May 2021 17:42:51 -0700 Subject: [PATCH 1/4] add resultCacheMaxSize configuration --- src/cache/inmemory/__mocks__/optimism.ts | 5 + src/cache/inmemory/__tests__/cache.ts | 35 +++++++ src/cache/inmemory/__tests__/readFromStore.ts | 31 +++++++ src/cache/inmemory/inMemoryCache.ts | 64 +++++++------ src/cache/inmemory/readFromStore.ts | 93 ++++++++++--------- 5 files changed, 158 insertions(+), 70 deletions(-) create mode 100644 src/cache/inmemory/__mocks__/optimism.ts diff --git a/src/cache/inmemory/__mocks__/optimism.ts b/src/cache/inmemory/__mocks__/optimism.ts new file mode 100644 index 00000000000..a81787c6703 --- /dev/null +++ b/src/cache/inmemory/__mocks__/optimism.ts @@ -0,0 +1,5 @@ +const optimism = jest.requireActual('optimism'); +module.exports = { + ...optimism, + wrap: jest.fn(optimism.wrap), +}; diff --git a/src/cache/inmemory/__tests__/cache.ts b/src/cache/inmemory/__tests__/cache.ts index a0e00d24b6f..57d3a442a01 100644 --- a/src/cache/inmemory/__tests__/cache.ts +++ b/src/cache/inmemory/__tests__/cache.ts @@ -6,6 +6,9 @@ import { makeReference, Reference, makeVar, TypedDocumentNode, isReference, Docu import { Cache } from '../../../cache'; import { InMemoryCache, InMemoryCacheConfig } from '../inMemoryCache'; +jest.mock('optimism'); +import { wrap } from 'optimism'; + disableFragmentWarnings(); describe('Cache', () => { @@ -1733,6 +1736,38 @@ describe('Cache', () => { }); }); +describe('resultCacheMaxSize', () => { + let wrapSpy: jest.Mock = wrap as jest.Mock; + beforeEach(() => { + wrapSpy.mockClear(); + }); + + it("does not set max size on caches if resultCacheMaxSize is not configured", () => { + new InMemoryCache(); + expect(wrapSpy).toHaveBeenCalled(); + /* + * The first wrap call is for getFragmentQueryDocument which intentionally + * does not have a max set since it's not expected to grow. + */ + wrapSpy.mock.calls.splice(1).forEach(([, { max }]) => { + expect(max).toBeUndefined(); + }) + }); + + it("configures max size on caches when resultCacheMaxSize is set", () => { + const resultCacheMaxSize = 12345; + new InMemoryCache({ resultCacheMaxSize }); + expect(wrapSpy).toHaveBeenCalled(); + /* + * The first wrap call is for getFragmentQueryDocument which intentionally + * does not have a max set since it's not expected to grow. + */ + wrapSpy.mock.calls.splice(1).forEach(([, { max }]) => { + expect(max).toBe(resultCacheMaxSize); + }) + }); +}); + describe("InMemoryCache#broadcastWatches", function () { it("should keep distinct consumers distinct (issue #5733)", function () { const cache = new InMemoryCache(); diff --git a/src/cache/inmemory/__tests__/readFromStore.ts b/src/cache/inmemory/__tests__/readFromStore.ts index 2a25c064df9..234fcabb6a5 100644 --- a/src/cache/inmemory/__tests__/readFromStore.ts +++ b/src/cache/inmemory/__tests__/readFromStore.ts @@ -19,6 +19,37 @@ import { TypedDocumentNode, } from '../../../core'; +jest.mock('optimism'); +import { wrap } from 'optimism'; + +describe('resultCacheMaxSize', () => { + const cache = new InMemoryCache(); + let wrapSpy: jest.Mock = wrap as jest.Mock; + beforeEach(() => { + wrapSpy.mockClear(); + }); + + it("does not set max size on caches if resultCacheMaxSize is not configured", () => { + new StoreReader({ cache }); + expect(wrapSpy).toHaveBeenCalled(); + + wrapSpy.mock.calls.forEach(([, { max }]) => { + expect(max).toBeUndefined(); + }) + }); + + it("configures max size on caches when resultCacheMaxSize is set", () => { + const resultCacheMaxSize = 12345; + new StoreReader({ cache, resultCacheMaxSize }); + expect(wrapSpy).toHaveBeenCalled(); + + wrapSpy.mock.calls.forEach(([, { max }]) => { + expect(max).toBe(resultCacheMaxSize); + }) + }); +}); + + describe('reading from the store', () => { const reader = new StoreReader({ cache: new InMemoryCache(), diff --git a/src/cache/inmemory/inMemoryCache.ts b/src/cache/inmemory/inMemoryCache.ts index 5c2b2a9e2dd..60e93fab167 100644 --- a/src/cache/inmemory/inMemoryCache.ts +++ b/src/cache/inmemory/inMemoryCache.ts @@ -2,7 +2,7 @@ import './fixPolyfills'; import { DocumentNode } from 'graphql'; -import { wrap } from 'optimism'; +import { OptimisticWrapperFunction, wrap } from 'optimism'; import { ApolloCache, BatchOptions } from '../core/cache'; import { Cache } from '../core/types/Cache'; @@ -33,6 +33,7 @@ export interface InMemoryCacheConfig extends ApolloReducerConfig { resultCaching?: boolean; possibleTypes?: PossibleTypesMap; typePolicies?: TypePolicies; + resultCacheMaxSize?: number; } type BroadcastOptions = Pick< @@ -60,6 +61,11 @@ export class InMemoryCache extends ApolloCache { private storeReader: StoreReader; private storeWriter: StoreWriter; + private maybeBroadcastWatch: OptimisticWrapperFunction< + [Cache.WatchOptions, BroadcastOptions?], + any, + [Cache.WatchOptions]>; + // Dynamically imported code can augment existing typePolicies or // possibleTypes by calling cache.policies.addTypePolicies or // cache.policies.addPossibletypes. @@ -99,8 +105,37 @@ export class InMemoryCache extends ApolloCache { this.storeReader = new StoreReader({ cache: this, addTypename: this.addTypename, + resultCacheMaxSize: this.config.resultCacheMaxSize, }), ); + + this.maybeBroadcastWatch = wrap(( + c: Cache.WatchOptions, + options?: BroadcastOptions, + ) => { + return this.broadcastWatch(c, options); + }, { + max: this.config.resultCacheMaxSize, + makeCacheKey: (c: Cache.WatchOptions) => { + // Return a cache key (thus enabling result caching) only if we're + // currently using a data store that can track cache dependencies. + const store = c.optimistic ? this.optimisticData : this.data; + if (supportsResultCaching(store)) { + const { optimistic, rootId, variables } = c; + return store.makeCacheKey( + c.query, + // Different watches can have the same query, optimistic + // status, rootId, and variables, but if their callbacks are + // different, the (identical) result needs to be delivered to + // each distinct callback. The easiest way to achieve that + // separation is to include c.callback in the cache key for + // maybeBroadcastWatch calls. See issue #5733. + c.callback, + JSON.stringify({ optimistic, rootId, variables }), + ); + } + } + }); } public restore(data: NormalizedCacheObject): this { @@ -423,33 +458,6 @@ export class InMemoryCache extends ApolloCache { } } - private maybeBroadcastWatch = wrap(( - c: Cache.WatchOptions, - options?: BroadcastOptions, - ) => { - return this.broadcastWatch(c, options); - }, { - makeCacheKey: (c: Cache.WatchOptions) => { - // Return a cache key (thus enabling result caching) only if we're - // currently using a data store that can track cache dependencies. - const store = c.optimistic ? this.optimisticData : this.data; - if (supportsResultCaching(store)) { - const { optimistic, rootId, variables } = c; - return store.makeCacheKey( - c.query, - // Different watches can have the same query, optimistic - // status, rootId, and variables, but if their callbacks are - // different, the (identical) result needs to be delivered to - // each distinct callback. The easiest way to achieve that - // separation is to include c.callback in the cache key for - // maybeBroadcastWatch calls. See issue #5733. - c.callback, - JSON.stringify({ optimistic, rootId, variables }), - ); - } - } - }); - // This method is wrapped by maybeBroadcastWatch, which is called by // broadcastWatches, so that we compute and broadcast results only when // the data that would be broadcast might have changed. It would be diff --git a/src/cache/inmemory/readFromStore.ts b/src/cache/inmemory/readFromStore.ts index a463ce8af34..f9ed078573b 100644 --- a/src/cache/inmemory/readFromStore.ts +++ b/src/cache/inmemory/readFromStore.ts @@ -80,11 +80,62 @@ type ExecSubSelectedArrayOptions = { export interface StoreReaderConfig { cache: InMemoryCache, addTypename?: boolean; + resultCacheMaxSize?: number; } export class StoreReader { + // cached version of executeSelectionset + private executeSelectionSet: OptimisticWrapperFunction< + [ExecSelectionSetOptions], // Actual arguments tuple type. + ExecResult, // Actual return type. + // Arguments type after keyArgs translation. + [SelectionSetNode, StoreObject | Reference, ReadMergeModifyContext]>; + + // cached version of executeSubSelectedArray + private executeSubSelectedArray: OptimisticWrapperFunction< + [ExecSubSelectedArrayOptions], + ExecResult, + [ExecSubSelectedArrayOptions]>; + constructor(private config: StoreReaderConfig) { this.config = { addTypename: true, ...config }; + + this.executeSelectionSet = wrap(options => this.execSelectionSetImpl(options), { + keyArgs(options) { + return [ + options.selectionSet, + options.objectOrReference, + options.context, + ]; + }, + max: this.config.resultCacheMaxSize, + // Note that the parameters of makeCacheKey are determined by the + // array returned by keyArgs. + makeCacheKey(selectionSet, parent, context) { + if (supportsResultCaching(context.store)) { + return context.store.makeCacheKey( + selectionSet, + isReference(parent) ? parent.__ref : parent, + context.varString, + ); + } + } + }); + + this.executeSubSelectedArray = wrap((options: ExecSubSelectedArrayOptions) => { + return this.execSubSelectedArrayImpl(options); + }, { + max: this.config.resultCacheMaxSize, + makeCacheKey({ field, array, context }) { + if (supportsResultCaching(context.store)) { + return context.store.makeCacheKey( + field, + array, + context.varString, + ); + } + } + }); } /** @@ -152,33 +203,6 @@ export class StoreReader { return false; } - // Cached version of execSelectionSetImpl. - private executeSelectionSet: OptimisticWrapperFunction< - [ExecSelectionSetOptions], // Actual arguments tuple type. - ExecResult, // Actual return type. - // Arguments type after keyArgs translation. - [SelectionSetNode, StoreObject | Reference, ReadMergeModifyContext] - > = wrap(options => this.execSelectionSetImpl(options), { - keyArgs(options) { - return [ - options.selectionSet, - options.objectOrReference, - options.context, - ]; - }, - // Note that the parameters of makeCacheKey are determined by the - // array returned by keyArgs. - makeCacheKey(selectionSet, parent, context) { - if (supportsResultCaching(context.store)) { - return context.store.makeCacheKey( - selectionSet, - isReference(parent) ? parent.__ref : parent, - context.varString, - ); - } - } - }); - // Uncached version of executeSelectionSet. private execSelectionSetImpl({ selectionSet, @@ -330,21 +354,6 @@ export class StoreReader { private knownResults = new WeakMap, SelectionSetNode>(); - // Cached version of execSubSelectedArrayImpl. - private executeSubSelectedArray = wrap((options: ExecSubSelectedArrayOptions) => { - return this.execSubSelectedArrayImpl(options); - }, { - makeCacheKey({ field, array, context }) { - if (supportsResultCaching(context.store)) { - return context.store.makeCacheKey( - field, - array, - context.varString, - ); - } - } - }); - // Uncached version of executeSubSelectedArray. private execSubSelectedArrayImpl({ field, From 1a6781a76b43917052873725ba004ebd5196ce89 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 5 May 2021 10:04:05 -0400 Subject: [PATCH 2/4] Stylistic tweaks. --- src/cache/inmemory/__tests__/cache.ts | 54 +++++++++++++-------------- 1 file changed, 26 insertions(+), 28 deletions(-) diff --git a/src/cache/inmemory/__tests__/cache.ts b/src/cache/inmemory/__tests__/cache.ts index 57d3a442a01..08fc4766c44 100644 --- a/src/cache/inmemory/__tests__/cache.ts +++ b/src/cache/inmemory/__tests__/cache.ts @@ -1737,35 +1737,33 @@ describe('Cache', () => { }); describe('resultCacheMaxSize', () => { - let wrapSpy: jest.Mock = wrap as jest.Mock; - beforeEach(() => { - wrapSpy.mockClear(); - }); - - it("does not set max size on caches if resultCacheMaxSize is not configured", () => { - new InMemoryCache(); - expect(wrapSpy).toHaveBeenCalled(); - /* - * The first wrap call is for getFragmentQueryDocument which intentionally - * does not have a max set since it's not expected to grow. - */ - wrapSpy.mock.calls.splice(1).forEach(([, { max }]) => { - expect(max).toBeUndefined(); - }) - }); + let wrapSpy: jest.Mock = wrap as jest.Mock; + beforeEach(() => { + wrapSpy.mockClear(); + }); - it("configures max size on caches when resultCacheMaxSize is set", () => { - const resultCacheMaxSize = 12345; - new InMemoryCache({ resultCacheMaxSize }); - expect(wrapSpy).toHaveBeenCalled(); - /* - * The first wrap call is for getFragmentQueryDocument which intentionally - * does not have a max set since it's not expected to grow. - */ - wrapSpy.mock.calls.splice(1).forEach(([, { max }]) => { - expect(max).toBe(resultCacheMaxSize); - }) - }); + it("does not set max size on caches if resultCacheMaxSize is not configured", () => { + new InMemoryCache(); + expect(wrapSpy).toHaveBeenCalled(); + + // The first wrap call is for getFragmentQueryDocument which intentionally + // does not have a max set since it's not expected to grow. + wrapSpy.mock.calls.splice(1).forEach(([, { max }]) => { + expect(max).toBeUndefined(); + }) + }); + + it("configures max size on caches when resultCacheMaxSize is set", () => { + const resultCacheMaxSize = 12345; + new InMemoryCache({ resultCacheMaxSize }); + expect(wrapSpy).toHaveBeenCalled(); + + // The first wrap call is for getFragmentQueryDocument which intentionally + // does not have a max set since it's not expected to grow. + wrapSpy.mock.calls.splice(1).forEach(([, { max }]) => { + expect(max).toBe(resultCacheMaxSize); + }) + }); }); describe("InMemoryCache#broadcastWatches", function () { From 526c940f3c1298bf0a9a3c3764f9dbe01d4996ee Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 5 May 2021 10:07:47 -0400 Subject: [PATCH 3/4] Make cache.reset() also dump all StoreReader/EntityStore data. --- src/cache/inmemory/entityStore.ts | 11 ----------- src/cache/inmemory/inMemoryCache.ts | 7 +++++-- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/src/cache/inmemory/entityStore.ts b/src/cache/inmemory/entityStore.ts index 894069fe75a..6dc3124df19 100644 --- a/src/cache/inmemory/entityStore.ts +++ b/src/cache/inmemory/entityStore.ts @@ -341,17 +341,6 @@ export abstract class EntityStore implements NormalizedCache { } } - // Remove every Layer, leaving behind only the Root and the Stump. - public prune(): EntityStore { - if (this instanceof Layer) { - const parent = this.removeLayer(this.id); - if (parent !== this) { - return parent.prune(); - } - } - return this; - } - public abstract getStorage( idOrObj: string | StoreObject, ...storeFieldNames: (string | number)[] diff --git a/src/cache/inmemory/inMemoryCache.ts b/src/cache/inmemory/inMemoryCache.ts index 60e93fab167..dc3cc2c762d 100644 --- a/src/cache/inmemory/inMemoryCache.ts +++ b/src/cache/inmemory/inMemoryCache.ts @@ -85,6 +85,10 @@ export class InMemoryCache extends ApolloCache { typePolicies: this.config.typePolicies, }); + this.init(); + } + + private init() { // Passing { resultCaching: false } in the InMemoryCache constructor options // will completely disable dependency tracking, which will improve memory // usage but worsen the performance of repeated reads. @@ -320,8 +324,7 @@ export class InMemoryCache extends ApolloCache { } public reset(): Promise { - this.optimisticData = this.optimisticData.prune(); - this.data.clear(); + this.init(); this.broadcastWatches(); return Promise.resolve(); } From dacbbe0b912796497c8a3b0d20c3bd6777978ed2 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 5 May 2021 12:44:03 -0400 Subject: [PATCH 4/4] Mention PR #8107 in CHANGELOG.md. --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d74dff42da..69ffa821a71 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,9 @@ - Pass `variables` and `context` to a mutation's `update` function
[@jcreighton](https://github.com/jcreighton) in [#7902](https://github.com/apollographql/apollo-client/pull/7902) +- A `resultCacheMaxSize` option may be passed to the `InMemoryCache` constructor to limit the number of result objects that will be retained in memory (to speed up repeated reads), and calling `cache.reset()` now releases all such memory.
+ [@SofianHn](https://github.com/SofianHn) in [#8701](https://github.com/apollographql/apollo-client/pull/8701) + ### Documentation TBD