From acca75ca3d8eca77831111f6f03f37e5f175735d Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 16 Jan 2019 16:44:46 -0500 Subject: [PATCH 1/6] Avoid copying entire cache on each optimistic read. Previously, the `InMemoryCache` maintained an array of recorded optimistic updates, which it would merge together into an entirely new `ObjectCache` whenever performing any single optimistic read. This merging behavior was wasteful, but the real performance killer was calling `this.extract(true)` each time, which copied the entire underlying (non-optimistic) cache, just to create a throw-away `ObjectCache` for the duration of the optimistic read. Worse still, `this.extract(true)` was also called in `recordOptimisticTransaction`, to create a throw-away `RecordingCache` object. Instead of creating temporary copies of the entire cache, `InMemoryCache` now maintains a linked list of `OptimisticCacheLayer` objects, which extend `ObjectCache` and implement the `NormalizedCache` interface, but cleverly delegate to a parent cache for any missing properties. This delegation happens simply by calling `this.parent.get(dataId)`, so there is no need to extract/copy the parent cache. When no optimistic data are currently active, `cache.optimisticData` will be the same (`===`) as `cache.data`, which means there are no additional layers on top of the actual data. Lookup time is proportional to the number of `OptimisticCacheLayer` objects in the linked list, so it's best if that list remains reasonably short, but at least that's something the application developer can control. Calling `removeOptimistic(id)` removes all `OptimisticCacheLayer` objects with the given `id`, and then reapplies the remaining layers by re-running their transaction functions. These improvements probably would have made the excessive memory usage reported in https://github.com/apollographql/apollo-client/issues/4210 much less severe, though disabling dependency tracking for optimistic reads (the previous solution) still seems like a good idea. --- .../src/__tests__/recordingCache.ts | 64 +++---- .../src/inMemoryCache.ts | 170 +++++++++--------- packages/apollo-cache-inmemory/src/index.ts | 1 - .../apollo-cache-inmemory/src/objectCache.ts | 19 +- .../src/recordingCache.ts | 56 ------ .../apollo-client/src/__tests__/client.ts | 9 +- 6 files changed, 139 insertions(+), 180 deletions(-) delete mode 100644 packages/apollo-cache-inmemory/src/recordingCache.ts diff --git a/packages/apollo-cache-inmemory/src/__tests__/recordingCache.ts b/packages/apollo-cache-inmemory/src/__tests__/recordingCache.ts index 48670d5dbbf..40c130ac831 100644 --- a/packages/apollo-cache-inmemory/src/__tests__/recordingCache.ts +++ b/packages/apollo-cache-inmemory/src/__tests__/recordingCache.ts @@ -1,43 +1,43 @@ -import { RecordingCache } from '../recordingCache'; +import { OptimisticCacheLayer } from '../inMemoryCache'; +import { ObjectCache } from '../objectCache'; import { NormalizedCacheObject } from '../types'; -describe('RecordingCache', () => { +describe('OptimisticCacheLayer', () => { describe('returns correct values during recording', () => { const data = { Human: { __typename: 'Human', name: 'Mark' }, Animal: { __typename: 'Mouse', name: '🐭' }, }; - const dataToRecord = { Human: { __typename: 'Human', name: 'John' } }; - let cache: RecordingCache; + const dataToRecord = { + Human: { __typename: 'Human', name: 'John' }, + }; + + const underlyingCache = new ObjectCache(data); + + let cache = new OptimisticCacheLayer('whatever', underlyingCache); beforeEach(() => { - cache = new RecordingCache({ ...data }); + cache = new OptimisticCacheLayer('whatever', underlyingCache); }); it('should passthrough values if not defined in recording', () => { - cache.record(() => { - expect(cache.get('Human')).toBe(data.Human); - expect(cache.get('Animal')).toBe(data.Animal); - }); + expect(cache.get('Human')).toBe(data.Human); + expect(cache.get('Animal')).toBe(data.Animal); }); it('should return values defined during recording', () => { - const recording = cache.record(() => { - cache.set('Human', dataToRecord.Human); - expect(cache.get('Human')).toBe(dataToRecord.Human); - }); - expect(recording.Human).toBe(dataToRecord.Human); + cache.set('Human', dataToRecord.Human); + expect(cache.get('Human')).toBe(dataToRecord.Human); + expect(underlyingCache.get('Human')).toBe(data.Human); }); it('should return undefined for values deleted during recording', () => { - const recording = cache.record(() => { - expect(cache.get('Animal')).toBe(data.Animal); - // delete should be registered in the recording: - cache.delete('Animal'); - expect(cache.get('Animal')).toBeUndefined(); - }); - - expect(recording).toHaveProperty('Animal'); + expect(cache.get('Animal')).toBe(data.Animal); + // delete should be registered in the recording: + cache.delete('Animal'); + expect(cache.get('Animal')).toBeUndefined(); + expect(cache.toObject()).toHaveProperty('Animal'); + expect(underlyingCache.get('Animal')).toBe(data.Animal); }); }); @@ -46,16 +46,20 @@ describe('RecordingCache', () => { Human: { __typename: 'Human', name: 'Mark' }, Animal: { __typename: 'Mouse', name: '🐭' }, }; - const dataToRecord = { Human: { __typename: 'Human', name: 'John' } }; - let cache: RecordingCache; + + const dataToRecord = { + Human: { __typename: 'Human', name: 'John' }, + }; + + const underlyingCache = new ObjectCache(data); + let cache = new OptimisticCacheLayer('whatever', underlyingCache); let recording: NormalizedCacheObject; beforeEach(() => { - cache = new RecordingCache({ ...data }); - recording = cache.record(() => { - cache.set('Human', dataToRecord.Human); - cache.delete('Animal'); - }); + cache = new OptimisticCacheLayer('whatever', underlyingCache); + cache.set('Human', dataToRecord.Human); + cache.delete('Animal'); + recording = cache.toObject(); }); it('should contain the property indicating deletion', () => { @@ -70,7 +74,7 @@ describe('RecordingCache', () => { }); it('should keep the original data unaffected', () => { - expect(cache.toObject()).toEqual(data); + expect(underlyingCache.toObject()).toEqual(data); }); }); }); diff --git a/packages/apollo-cache-inmemory/src/inMemoryCache.ts b/packages/apollo-cache-inmemory/src/inMemoryCache.ts index 41dd5ade92d..f7fb53b80b3 100644 --- a/packages/apollo-cache-inmemory/src/inMemoryCache.ts +++ b/packages/apollo-cache-inmemory/src/inMemoryCache.ts @@ -12,7 +12,6 @@ import { import { HeuristicFragmentMatcher } from './fragmentMatcher'; import { - OptimisticStoreItem, ApolloReducerConfig, NormalizedCache, NormalizedCacheObject, @@ -22,11 +21,9 @@ import { StoreReader } from './readFromStore'; import { StoreWriter } from './writeToStore'; import { DepTrackingCache } from './depTrackingCache'; -import { wrap, CacheKeyNode, OptimisticWrapperFunction } from './optimism'; +import { wrap, CacheKeyNode } from './optimism'; import { ObjectCache } from './objectCache'; -import { record } from './recordingCache'; - export interface InMemoryCacheConfig extends ApolloReducerConfig { resultCaching?: boolean; } @@ -50,10 +47,50 @@ export function defaultDataIdFromObject(result: any): string | null { return null; } +const hasOwn = Object.prototype.hasOwnProperty; + +export class OptimisticCacheLayer extends ObjectCache { + constructor( + public readonly optimisticId: string, + public parent: NormalizedCache | null = null, + ) { + super(Object.create(null)); + } + + public prune(idToRemove: string) { + if (this.parent instanceof OptimisticCacheLayer) { + this.parent = this.parent.prune(idToRemove); + } + + if (this.optimisticId === idToRemove) { + return this.parent; + } + + return this; + } + + public toObject(): NormalizedCacheObject { + return this.parent ? { + ...this.parent.toObject(), + ...this.data, + } : this.data; + } + + public get(dataId: string) { + if (hasOwn.call(this.data, dataId)) { + return this.data[dataId]; + } + if (this.parent) { + return this.parent.get(dataId); + } + } +} + export class InMemoryCache extends ApolloCache { - protected data: NormalizedCache; + private data: NormalizedCache; + private optimisticData: NormalizedCache; + protected config: InMemoryCacheConfig; - protected optimistic: OptimisticStoreItem[] = []; private watches = new Set(); private addTypename: boolean; private typenameDocumentCache = new Map(); @@ -85,9 +122,11 @@ export class InMemoryCache extends ApolloCache { } this.addTypename = this.config.addTypename; + this.data = this.config.resultCaching ? new DepTrackingCache() : new ObjectCache(); + this.optimisticData = this.data; this.storeReader = new StoreReader(this.cacheKeyRoot); this.storeWriter = new StoreWriter(); @@ -98,7 +137,7 @@ export class InMemoryCache extends ApolloCache { return maybeBroadcastWatch.call(this, c); }, { makeCacheKey(c: Cache.WatchOptions) { - if (c.optimistic && cache.optimistic.length > 0) { + if (c.optimistic) { // If we're reading optimistic data, it doesn't matter if this.data // is a DepTrackingCache, since it will be ignored. return; @@ -130,31 +169,21 @@ export class InMemoryCache extends ApolloCache { } public extract(optimistic: boolean = false): NormalizedCacheObject { - if (optimistic && this.optimistic.length > 0) { - const patches = this.optimistic.map(opt => opt.data); - return Object.assign({}, this.data.toObject(), ...patches); - } - - return this.data.toObject(); + return (optimistic ? this.optimisticData : this.data).toObject(); } - public read(query: Cache.ReadOptions): T | null { - if (query.rootId && this.data.get(query.rootId) === undefined) { + public read(options: Cache.ReadOptions): T | null { + if (options.rootId && this.data.get(options.rootId) === undefined) { return null; } - const store = - query.optimistic && this.optimistic.length - ? new ObjectCache(this.extract(true)) - : this.data; - return this.storeReader.readQueryFromStore({ - store, - query: this.transformDocument(query.query), - variables: query.variables, - rootId: query.rootId, + store: options.optimistic ? this.optimisticData : this.data, + query: this.transformDocument(options.query), + variables: options.variables, + rootId: options.rootId, fragmentMatcherFunction: this.config.fragmentMatcher.match, - previousResult: query.previousResult, + previousResult: options.previousResult, config: this.config, }); } @@ -174,13 +203,8 @@ export class InMemoryCache extends ApolloCache { } public diff(query: Cache.DiffOptions): Cache.DiffResult { - const store = - query.optimistic && this.optimistic.length - ? new ObjectCache(this.extract(true)) - : this.data; - return this.storeReader.diffQueryAgainstStore({ - store: store, + store: query.optimistic ? this.optimisticData : this.data, query: this.transformDocument(query.query), variables: query.variables, returnPartialData: query.returnPartialData, @@ -210,33 +234,36 @@ export class InMemoryCache extends ApolloCache { } public removeOptimistic(id: string) { - // Throw away optimistic changes of that particular mutation - const toPerform = this.optimistic.filter(item => item.id !== id); - - this.optimistic = []; - - // Re-run all of our optimistic data actions on top of one another. - toPerform.forEach(change => { - this.recordOptimisticTransaction(change.transaction, change.id); - }); - + if (this.optimisticData instanceof OptimisticCacheLayer) { + this.optimisticData = this.optimisticData.prune(id); + } this.broadcastWatches(); } - public performTransaction(transaction: Transaction) { - // TODO: does this need to be different, or is this okay for an in-memory cache? - - let alreadySilenced = this.silenceBroadcast; + public performTransaction( + transaction: Transaction, + optimisticId?: string, + ) { + const { data, silenceBroadcast } = this; this.silenceBroadcast = true; - transaction(this); + if (typeof optimisticId === 'string') { + // Add a new optimistic layer and temporarily make this.data refer to + // that layer for the duration of the transaction. + this.data = this.optimisticData = new OptimisticCacheLayer( + optimisticId, + this.optimisticData, + ); + } - if (!alreadySilenced) { - // Don't un-silence since this is a nested transaction - // (for example, a transaction inside an optimistic record) - this.silenceBroadcast = false; + try { + transaction(this); + } finally { + this.silenceBroadcast = silenceBroadcast; + this.data = data; } + // This broadcast does nothing if this.silenceBroadcast is true: this.broadcastWatches(); } @@ -244,26 +271,7 @@ export class InMemoryCache extends ApolloCache { transaction: Transaction, id: string, ) { - this.silenceBroadcast = true; - - const patch = record(this.extract(true), recordingCache => { - // swapping data instance on 'this' is currently necessary - // because of the current architecture - const dataCache = this.data; - this.data = recordingCache; - this.performTransaction(transaction); - this.data = dataCache; - }); - - this.optimistic.push({ - id, - transaction, - data: patch, - }); - - this.silenceBroadcast = false; - - this.broadcastWatches(); + return this.performTransaction(transaction, id); } public transformDocument(document: DocumentNode): DocumentNode { @@ -333,16 +341,8 @@ export class InMemoryCache extends ApolloCache { protected broadcastWatches() { if (!this.silenceBroadcast) { - const optimistic = this.optimistic.length > 0; this.watches.forEach((c: Cache.WatchOptions) => { this.maybeBroadcastWatch(c); - if (optimistic) { - // If we're broadcasting optimistic data, make sure we rebroadcast - // the real data once we're no longer in an optimistic state. - (this.maybeBroadcastWatch as OptimisticWrapperFunction< - (c: Cache.WatchOptions) => void - >).dirty(c); - } }); } } @@ -350,11 +350,13 @@ export class InMemoryCache extends ApolloCache { // This method is wrapped in the constructor so that it will be called only // if the data that would be broadcast has changed. private maybeBroadcastWatch(c: Cache.WatchOptions) { - c.callback(this.diff({ - query: c.query, - variables: c.variables, - previousResult: c.previousResult && c.previousResult(), - optimistic: c.optimistic, - })); + c.callback( + this.diff({ + query: c.query, + variables: c.variables, + previousResult: c.previousResult && c.previousResult(), + optimistic: c.optimistic, + }), + ); } } diff --git a/packages/apollo-cache-inmemory/src/index.ts b/packages/apollo-cache-inmemory/src/index.ts index d3e1779cb8b..429817f40ac 100644 --- a/packages/apollo-cache-inmemory/src/index.ts +++ b/packages/apollo-cache-inmemory/src/index.ts @@ -8,5 +8,4 @@ export * from './readFromStore'; export * from './writeToStore'; export * from './fragmentMatcher'; export * from './objectCache'; -export * from './recordingCache'; export * from './types'; diff --git a/packages/apollo-cache-inmemory/src/objectCache.ts b/packages/apollo-cache-inmemory/src/objectCache.ts index 7b33feee71f..c2023b09dbf 100644 --- a/packages/apollo-cache-inmemory/src/objectCache.ts +++ b/packages/apollo-cache-inmemory/src/objectCache.ts @@ -1,23 +1,28 @@ import { NormalizedCache, NormalizedCacheObject, StoreObject } from './types'; export class ObjectCache implements NormalizedCache { - constructor(private data: NormalizedCacheObject = Object.create(null)) {} - public toObject(): NormalizedCacheObject { + constructor(protected data: NormalizedCacheObject = Object.create(null)) {} + + public toObject() { return this.data; } - public get(dataId: string): StoreObject { + public get(dataId: string) { return this.data[dataId]; } + public set(dataId: string, value: StoreObject) { this.data[dataId] = value; } - public delete(dataId: string): void { - this.data[dataId] = undefined; + + public delete(dataId: string) { + this.data[dataId] = void 0; } - public clear(): void { + + public clear() { this.data = Object.create(null); } - public replace(newData: NormalizedCacheObject): void { + + public replace(newData: NormalizedCacheObject) { this.data = newData || Object.create(null); } } diff --git a/packages/apollo-cache-inmemory/src/recordingCache.ts b/packages/apollo-cache-inmemory/src/recordingCache.ts deleted file mode 100644 index a71249ef8cb..00000000000 --- a/packages/apollo-cache-inmemory/src/recordingCache.ts +++ /dev/null @@ -1,56 +0,0 @@ -import { NormalizedCache, NormalizedCacheObject, StoreObject } from './types'; - -export class RecordingCache implements NormalizedCache { - private recordedData: NormalizedCacheObject = {}; - - constructor(private readonly data: NormalizedCacheObject = {}) {} - - public record( - transaction: (recordingCache: RecordingCache) => void, - ): NormalizedCacheObject { - transaction(this); - const recordedData = this.recordedData; - this.recordedData = {}; - return recordedData; - } - - public toObject(): NormalizedCacheObject { - return { ...this.data, ...this.recordedData }; - } - - public get(dataId: string): StoreObject { - if (this.recordedData.hasOwnProperty(dataId)) { - // recording always takes precedence: - return this.recordedData[dataId]; - } - return this.data[dataId]; - } - - public set(dataId: string, value: StoreObject) { - if (this.get(dataId) !== value) { - this.recordedData[dataId] = value; - } - } - - public delete(dataId: string): void { - this.recordedData[dataId] = undefined; - } - - public clear(): void { - Object.keys(this.data).forEach(dataId => this.delete(dataId)); - this.recordedData = {}; - } - - public replace(newData: NormalizedCacheObject): void { - this.clear(); - this.recordedData = { ...newData }; - } -} - -export function record( - startingState: NormalizedCacheObject, - transaction: (recordingCache: RecordingCache) => void, -): NormalizedCacheObject { - const recordingCache = new RecordingCache(startingState); - return recordingCache.record(transaction); -} diff --git a/packages/apollo-client/src/__tests__/client.ts b/packages/apollo-client/src/__tests__/client.ts index 2b92befc798..56935619577 100644 --- a/packages/apollo-client/src/__tests__/client.ts +++ b/packages/apollo-client/src/__tests__/client.ts @@ -2189,13 +2189,18 @@ describe('client', () => { }, }, }); - expect((client.cache as any).optimistic.length).toBe(1); + + const { data, optimisticData } = client.cache as any; + expect(optimisticData).not.toBe(data); + expect(optimisticData.parent).toBe(data); + mutatePromise .then(_ => { done.fail(new Error('Returned a result when it should not have.')); }) .catch((_: ApolloError) => { - expect((client.cache as any).optimistic.length).toBe(0); + const { data, optimisticData } = client.cache as any; + expect(optimisticData).toBe(data); done(); }); }); From 531aa323c6d665a31372dc839a56effb754ae3fe Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 16 Jan 2019 19:37:26 -0500 Subject: [PATCH 2/6] Reapply remaining layers in removeOptimistic by rerunning transactions. --- .../src/__tests__/recordingCache.ts | 12 +++-- .../src/inMemoryCache.ts | 46 ++++++++++++------- 2 files changed, 37 insertions(+), 21 deletions(-) diff --git a/packages/apollo-cache-inmemory/src/__tests__/recordingCache.ts b/packages/apollo-cache-inmemory/src/__tests__/recordingCache.ts index 40c130ac831..b26d7f49951 100644 --- a/packages/apollo-cache-inmemory/src/__tests__/recordingCache.ts +++ b/packages/apollo-cache-inmemory/src/__tests__/recordingCache.ts @@ -3,6 +3,10 @@ import { ObjectCache } from '../objectCache'; import { NormalizedCacheObject } from '../types'; describe('OptimisticCacheLayer', () => { + function makeLayer(root: ObjectCache) { + return new OptimisticCacheLayer('whatever', root, () => {}); + } + describe('returns correct values during recording', () => { const data = { Human: { __typename: 'Human', name: 'Mark' }, @@ -15,9 +19,9 @@ describe('OptimisticCacheLayer', () => { const underlyingCache = new ObjectCache(data); - let cache = new OptimisticCacheLayer('whatever', underlyingCache); + let cache = makeLayer(underlyingCache); beforeEach(() => { - cache = new OptimisticCacheLayer('whatever', underlyingCache); + cache = makeLayer(underlyingCache); }); it('should passthrough values if not defined in recording', () => { @@ -52,11 +56,11 @@ describe('OptimisticCacheLayer', () => { }; const underlyingCache = new ObjectCache(data); - let cache = new OptimisticCacheLayer('whatever', underlyingCache); + let cache = makeLayer(underlyingCache); let recording: NormalizedCacheObject; beforeEach(() => { - cache = new OptimisticCacheLayer('whatever', underlyingCache); + cache = makeLayer(underlyingCache); cache.set('Human', dataToRecord.Human); cache.delete('Animal'); recording = cache.toObject(); diff --git a/packages/apollo-cache-inmemory/src/inMemoryCache.ts b/packages/apollo-cache-inmemory/src/inMemoryCache.ts index f7fb53b80b3..01f3192f2d7 100644 --- a/packages/apollo-cache-inmemory/src/inMemoryCache.ts +++ b/packages/apollo-cache-inmemory/src/inMemoryCache.ts @@ -52,23 +52,12 @@ const hasOwn = Object.prototype.hasOwnProperty; export class OptimisticCacheLayer extends ObjectCache { constructor( public readonly optimisticId: string, - public parent: NormalizedCache | null = null, + public readonly parent: NormalizedCache, + public readonly transaction: Transaction, ) { super(Object.create(null)); } - public prune(idToRemove: string) { - if (this.parent instanceof OptimisticCacheLayer) { - this.parent = this.parent.prune(idToRemove); - } - - if (this.optimisticId === idToRemove) { - return this.parent; - } - - return this; - } - public toObject(): NormalizedCacheObject { return this.parent ? { ...this.parent.toObject(), @@ -233,11 +222,33 @@ export class InMemoryCache extends ApolloCache { return Promise.resolve(); } - public removeOptimistic(id: string) { - if (this.optimisticData instanceof OptimisticCacheLayer) { - this.optimisticData = this.optimisticData.prune(id); + public removeOptimistic(idToRemove: string) { + const toReapply: OptimisticCacheLayer[] = []; + let removedCount = 0; + let layer = this.optimisticData; + + while (layer instanceof OptimisticCacheLayer) { + if (layer.optimisticId === idToRemove) { + ++removedCount; + } else { + toReapply.push(layer); + } + layer = layer.parent; + } + + if (removedCount > 0) { + // Reset this.optimisticData to the first non-OptimisticCacheLayer object, + // which is almost certainly this.data. + this.optimisticData = layer; + + // Reapply the layers whose optimistic IDs do not match the removed ID. + while (toReapply.length > 0) { + const layer = toReapply.pop(); + this.performTransaction(layer.transaction, layer.optimisticId); + } + + this.broadcastWatches(); } - this.broadcastWatches(); } public performTransaction( @@ -253,6 +264,7 @@ export class InMemoryCache extends ApolloCache { this.data = this.optimisticData = new OptimisticCacheLayer( optimisticId, this.optimisticData, + transaction, ); } From 801a490045168c7f21b23c459baf3fc349401de1 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Thu, 17 Jan 2019 09:50:35 -0500 Subject: [PATCH 3/6] More comments and a few minor simplifications. --- .../src/inMemoryCache.ts | 42 +++++++++++++------ 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/packages/apollo-cache-inmemory/src/inMemoryCache.ts b/packages/apollo-cache-inmemory/src/inMemoryCache.ts index 01f3192f2d7..94d9ddbd86d 100644 --- a/packages/apollo-cache-inmemory/src/inMemoryCache.ts +++ b/packages/apollo-cache-inmemory/src/inMemoryCache.ts @@ -52,6 +52,8 @@ const hasOwn = Object.prototype.hasOwnProperty; export class OptimisticCacheLayer extends ObjectCache { constructor( public readonly optimisticId: string, + // OptimisticCacheLayer objects always wrap some other parent cache, so + // this.parent should never be null. public readonly parent: NormalizedCache, public readonly transaction: Transaction, ) { @@ -59,19 +61,19 @@ export class OptimisticCacheLayer extends ObjectCache { } public toObject(): NormalizedCacheObject { - return this.parent ? { + return { ...this.parent.toObject(), ...this.data, - } : this.data; + }; } + // All the other accessor methods of ObjectCache work without knowing about + // this.parent, but the get method needs to be overridden to implement the + // fallback this.parent.get(dataId) behavior. public get(dataId: string) { - if (hasOwn.call(this.data, dataId)) { - return this.data[dataId]; - } - if (this.parent) { - return this.parent.get(dataId); - } + return hasOwn.call(this.data, dataId) + ? this.data[dataId] + : this.parent.get(dataId); } } @@ -112,9 +114,18 @@ export class InMemoryCache extends ApolloCache { this.addTypename = this.config.addTypename; + // 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. this.data = this.config.resultCaching ? new DepTrackingCache() : new ObjectCache(); + + // When no optimistic writes are currently active, cache.optimisticData === + // cache.data, so there are no additional layers on top of the actual data. + // When an optimistic update happens, this.optimisticData will become a + // linked list of OptimisticCacheLayer objects that terminates with the + // original this.data cache object. this.optimisticData = this.data; this.storeReader = new StoreReader(this.cacheKeyRoot); @@ -162,7 +173,8 @@ export class InMemoryCache extends ApolloCache { } public read(options: Cache.ReadOptions): T | null { - if (options.rootId && this.data.get(options.rootId) === undefined) { + if (typeof options.rootId === 'string' && + typeof this.data.get(options.rootId) === 'undefined') { return null; } @@ -253,6 +265,9 @@ export class InMemoryCache extends ApolloCache { public performTransaction( transaction: Transaction, + // This parameter is not part of the performTransaction signature inherited + // from the ApolloCache abstract class, but it's useful because it saves us + // from duplicating this implementation in recordOptimisticTransaction. optimisticId?: string, ) { const { data, silenceBroadcast } = this; @@ -262,6 +277,9 @@ export class InMemoryCache extends ApolloCache { // Add a new optimistic layer and temporarily make this.data refer to // that layer for the duration of the transaction. this.data = this.optimisticData = new OptimisticCacheLayer( + // Note that there can be multiple layers with the same optimisticId. + // When removeOptimistic(id) is called for that id, all matching layers + // will be removed, and the remaining layers will be reapplied. optimisticId, this.optimisticData, transaction, @@ -275,7 +293,7 @@ export class InMemoryCache extends ApolloCache { this.data = data; } - // This broadcast does nothing if this.silenceBroadcast is true: + // This broadcast does nothing if this.silenceBroadcast is true. this.broadcastWatches(); } @@ -353,9 +371,7 @@ export class InMemoryCache extends ApolloCache { protected broadcastWatches() { if (!this.silenceBroadcast) { - this.watches.forEach((c: Cache.WatchOptions) => { - this.maybeBroadcastWatch(c); - }); + this.watches.forEach(c => this.maybeBroadcastWatch(c)); } } From d92fc1c588bb6c5bd29367c2037ca35b343b8fba Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Thu, 17 Jan 2019 09:57:21 -0500 Subject: [PATCH 4/6] Remove redundant this.transformDocument calls from InMemoryCache. --- packages/apollo-cache-inmemory/src/inMemoryCache.ts | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/packages/apollo-cache-inmemory/src/inMemoryCache.ts b/packages/apollo-cache-inmemory/src/inMemoryCache.ts index 94d9ddbd86d..8032734e46f 100644 --- a/packages/apollo-cache-inmemory/src/inMemoryCache.ts +++ b/packages/apollo-cache-inmemory/src/inMemoryCache.ts @@ -336,9 +336,7 @@ export class InMemoryCache extends ApolloCache { optimistic: boolean = false, ): FragmentType | null { return this.read({ - query: this.transformDocument( - getFragmentQueryDocument(options.fragment, options.fragmentName), - ), + query: getFragmentQueryDocument(options.fragment, options.fragmentName), variables: options.variables, rootId: options.id, optimistic, @@ -351,7 +349,7 @@ export class InMemoryCache extends ApolloCache { this.write({ dataId: 'ROOT_QUERY', result: options.data, - query: this.transformDocument(options.query), + query: options.query, variables: options.variables, }); } @@ -362,9 +360,7 @@ export class InMemoryCache extends ApolloCache { this.write({ dataId: options.id, result: options.data, - query: this.transformDocument( - getFragmentQueryDocument(options.fragment, options.fragmentName), - ), + query: getFragmentQueryDocument(options.fragment, options.fragmentName), variables: options.variables, }); } From fc3c3f18a1efab5b5f8cefa9d924cef90efc1245 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Thu, 17 Jan 2019 10:11:21 -0500 Subject: [PATCH 5/6] Remove InMemoryCache methods that are now identical to ApolloCache methods. Once we stopped calling this.transformDocument in these method implementations, they became character-for-character identical to the implementations already provided by the ApolloCache superclass, so we can simply inherit them as-is. --- .../src/inMemoryCache.ts | 48 +------------------ 1 file changed, 1 insertion(+), 47 deletions(-) diff --git a/packages/apollo-cache-inmemory/src/inMemoryCache.ts b/packages/apollo-cache-inmemory/src/inMemoryCache.ts index 8032734e46f..11488ac6a92 100644 --- a/packages/apollo-cache-inmemory/src/inMemoryCache.ts +++ b/packages/apollo-cache-inmemory/src/inMemoryCache.ts @@ -3,10 +3,9 @@ import './fixPolyfills'; import { DocumentNode } from 'graphql'; -import { Cache, DataProxy, ApolloCache, Transaction } from 'apollo-cache'; +import { Cache, ApolloCache, Transaction } from 'apollo-cache'; import { - getFragmentQueryDocument, addTypenameToDocument, } from 'apollo-utilities'; @@ -320,51 +319,6 @@ export class InMemoryCache extends ApolloCache { return document; } - public readQuery( - options: DataProxy.Query, - optimistic: boolean = false, - ): QueryType { - return this.read({ - query: options.query, - variables: options.variables, - optimistic, - }); - } - - public readFragment( - options: DataProxy.Fragment, - optimistic: boolean = false, - ): FragmentType | null { - return this.read({ - query: getFragmentQueryDocument(options.fragment, options.fragmentName), - variables: options.variables, - rootId: options.id, - optimistic, - }); - } - - public writeQuery( - options: DataProxy.WriteQueryOptions, - ): void { - this.write({ - dataId: 'ROOT_QUERY', - result: options.data, - query: options.query, - variables: options.variables, - }); - } - - public writeFragment( - options: DataProxy.WriteFragmentOptions, - ): void { - this.write({ - dataId: options.id, - result: options.data, - query: getFragmentQueryDocument(options.fragment, options.fragmentName), - variables: options.variables, - }); - } - protected broadcastWatches() { if (!this.silenceBroadcast) { this.watches.forEach(c => this.maybeBroadcastWatch(c)); From 4a870221ca961e5d55ea9fb3d99ff0b839fb9fdb Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Thu, 17 Jan 2019 14:28:32 -0500 Subject: [PATCH 6/6] Mention PR #4319 in CHANGELOG.md. --- CHANGELOG.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 48a3e989e02..33023a33ded 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,11 +43,19 @@ ### Apollo Cache In-Memory (vNext) +- The speed and memory usage of optimistic reads and writes has been + improved dramatically using a new layering technique that does not + require copying the non-optimistic contents of the cache. + [PR #4319](https://github.com/apollographql/apollo-client/pull/4319/) + +- The `RecordingCache` abstraction has been removed, and thus is no longer + exported from `apollo-cache-inmemory`. + [PR #4319](https://github.com/apollographql/apollo-client/pull/4319/) + - Export the optimism `wrap` function using ES2015 export syntax, instead of CommonJS.
[@ardatan](https://github.com/ardatan) in [#4158](https://github.com/apollographql/apollo-client/pull/4158) - ## Apollo Client (2.4.8) ### Apollo Client (2.4.8)