From a60b065f1d66833fc4f8c7746a69c3ee02969f0c Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Fri, 29 Nov 2019 13:01:52 -0500 Subject: [PATCH] Divide EntityStore into optimistic and non-optimistic CacheGroups. The result caching system introduced by #3394 gained the ability to cache optimistic results (rather than just non-optimistic results) in #5197, but since then has suffered from unnecessary cache key diversity during optimistic updates, because every EntityStore.Layer object (corresponding to a single optimistic update) counts as a distinct cache key, which prevents cached results from being reused if they were originally read from a different Layer object. This commit introduces the concept of a CacheGroup, store.group, which manages dependency tracking and also serves as a source of keys for the result caching system. While the Root object has its own CacheGroup, Layer objects share a CacheGroup object, which is the key to limiting diversity of cache keys when more than one optimistic update is pending. This separation allows the InMemoryCache to enjoy the full benefits of result caching for both optimistic (Layer) and non-optimistic (Root) data, separately. --- src/cache/inmemory/__tests__/optimistic.ts | 2 +- src/cache/inmemory/entityStore.ts | 134 ++++++++++++--------- src/cache/inmemory/inMemoryCache.ts | 2 +- src/cache/inmemory/readFromStore.ts | 12 +- 4 files changed, 86 insertions(+), 64 deletions(-) diff --git a/src/cache/inmemory/__tests__/optimistic.ts b/src/cache/inmemory/__tests__/optimistic.ts index 726c7034651..dabc2dc9a9d 100644 --- a/src/cache/inmemory/__tests__/optimistic.ts +++ b/src/cache/inmemory/__tests__/optimistic.ts @@ -427,7 +427,7 @@ describe('optimistic cache layers', () => { const spinelessAfterRemovingBuzz = readSpinelessFragment(); expect(spinelessBeforeRemovingBuzz).toEqual(spinelessAfterRemovingBuzz); expect(spinelessBeforeRemovingBuzz).not.toBe(spinelessAfterRemovingBuzz); - expect(spinelessBeforeRemovingBuzz.author).not.toBe( + expect(spinelessBeforeRemovingBuzz.author).toBe( spinelessAfterRemovingBuzz.author, ); diff --git a/src/cache/inmemory/entityStore.ts b/src/cache/inmemory/entityStore.ts index a66d1f6ff5b..635a21971bf 100644 --- a/src/cache/inmemory/entityStore.ts +++ b/src/cache/inmemory/entityStore.ts @@ -14,41 +14,10 @@ import { const hasOwn = Object.prototype.hasOwnProperty; -type DependType = OptimisticDependencyFunction | null; - -function makeDepKey(dataId: string, storeFieldName?: string) { - const parts = [dataId]; - if (typeof storeFieldName === "string") { - parts.push(fieldNameFromStoreName(storeFieldName)); - } - return JSON.stringify(parts); -} - -function depend(store: EntityStore, dataId: string, storeFieldName?: string) { - if (store.depend) { - store.depend(makeDepKey(dataId, storeFieldName)); - } -} - -function dirty(store: EntityStore, dataId: string, storeFieldName?: string) { - if (store.depend) { - store.depend.dirty( - typeof storeFieldName === "string" - ? makeDepKey(dataId, storeFieldName) - : makeDepKey(dataId), - ); - } -} - export abstract class EntityStore implements NormalizedCache { protected data: NormalizedCacheObject = Object.create(null); - // It seems like this property ought to be protected rather than public, - // but TypeScript doesn't realize it's inherited from a shared base - // class by both Root and Layer classes, so Layer methods are forbidden - // from accessing the .depend property of an arbitrary EntityStore - // instance, because it might be a Root instance (and vice-versa). - public readonly depend: DependType = null; + public readonly group: CacheGroup; public abstract addLayer( layerId: string, @@ -70,12 +39,12 @@ export abstract class EntityStore implements NormalizedCache { } public get(dataId: string): StoreObject { - depend(this, dataId); + this.group.depend(dataId); return this.data[dataId]; } public getFieldValue(dataId: string, storeFieldName: string): StoreValue { - depend(this, dataId, storeFieldName); + this.group.depend(dataId, storeFieldName); const storeObject = this.data[dataId]; return storeObject && storeObject[storeFieldName]; } @@ -87,15 +56,15 @@ export abstract class EntityStore implements NormalizedCache { if (merged !== existing) { this.data[dataId] = merged; delete this.refs[dataId]; - if (this.depend) { + if (this.group.caching) { // First, invalidate any dependents that called get rather than // getFieldValue. - dirty(this, dataId); + this.group.dirty(dataId); // Now invalidate dependents who called getFieldValue for any // fields that are changing as a result of this merge. Object.keys(incoming).forEach(storeFieldName => { if (!existing || incoming[storeFieldName] !== existing[storeFieldName]) { - dirty(this, dataId, storeFieldName); + this.group.dirty(dataId, storeFieldName); } }); } @@ -172,10 +141,10 @@ export abstract class EntityStore implements NormalizedCache { }); } - if (this.depend) { - dirty(this, dataId); + if (this.group.caching) { + this.group.dirty(dataId); Object.keys(fieldsToDirty).forEach(fieldName => { - dirty(this, dataId, fieldName); + this.group.dirty(dataId, fieldName); }); } } @@ -280,15 +249,62 @@ export abstract class EntityStore implements NormalizedCache { } } +// A single CacheGroup represents a set of one or more EntityStore objects, +// typically the Root store in a CacheGroup by itself, and all active Layer +// stores in a group together. A single EntityStore object belongs to one +// and only one CacheGroup, store.group. The CacheGroup is responsible for +// tracking dependencies, so store.group serves as a convenient key for +// storing cached results that should be invalidated when/if those +// dependencies change (see the various makeCachekey functions in +// inMemoryCache.ts and readFromStore.ts). If we used the EntityStore +// objects themselves as cache keys (that is, store rather than +// store.group), the cache would become unnecessarily fragmented by all the +// different Layer objects. Instead, the CacheGroup approach allows all +// optimistic Layer objects in the same linked list to belong to one +// CacheGroup, with the non-optimistic Root object belonging to another +// CacheGroup, allowing resultCaching dependencies to be tracked separately +// for optimistic and non-optimistic entity data. +class CacheGroup { + private d: OptimisticDependencyFunction | null = null; + + constructor(public readonly caching: boolean) { + this.d = caching ? dep() : null; + } + + public depend(dataId: string, storeFieldName?: string) { + if (this.d) { + this.d(makeDepKey(dataId, storeFieldName)); + } + } + + public dirty(dataId: string, storeFieldName?: string) { + if (this.d) { + this.d.dirty( + typeof storeFieldName === "string" + ? makeDepKey(dataId, storeFieldName) + : makeDepKey(dataId), + ); + } + } +} + +function makeDepKey(dataId: string, storeFieldName?: string) { + const parts = [dataId]; + if (typeof storeFieldName === "string") { + parts.push(fieldNameFromStoreName(storeFieldName)); + } + return JSON.stringify(parts); +} + export namespace EntityStore { // Refer to this class as EntityStore.Root outside this namespace. export class Root extends EntityStore { - // Although each Root instance gets its own unique this.depend - // function, any Layer instances created by calling addLayer need to - // share a single distinct dependency function. Since this shared - // function must outlast the Layer instances themselves, it needs to - // be created and owned by the Root instance. - private sharedLayerDepend: DependType = null; + // Although each Root instance gets its own unique CacheGroup object, + // any Layer instances created by calling addLayer need to share a + // single distinct CacheGroup object. Since this shared object must + // outlast the Layer instances themselves, it needs to be created and + // owned by the Root instance. + private sharedLayerGroup: CacheGroup = null; constructor({ resultCaching = true, @@ -298,11 +314,8 @@ export namespace EntityStore { seed?: NormalizedCacheObject; }) { super(); - if (resultCaching) { - // Regard this.depend as publicly readonly but privately mutable. - (this as any).depend = dep(); - this.sharedLayerDepend = dep(); - } + (this.group as any) = new CacheGroup(resultCaching); + this.sharedLayerGroup = new CacheGroup(resultCaching); if (seed) this.replace(seed); } @@ -311,7 +324,7 @@ export namespace EntityStore { replay: (layer: EntityStore) => any, ): EntityStore { // The replay function will be called in the Layer constructor. - return new Layer(layerId, this, replay, this.sharedLayerDepend); + return new Layer(layerId, this, replay, this.sharedLayerGroup); } public removeLayer(layerId: string): Root { @@ -328,7 +341,7 @@ class Layer extends EntityStore { public readonly id: string, public readonly parent: Layer | EntityStore.Root, public readonly replay: (layer: EntityStore) => any, - public readonly depend: DependType, + public readonly group: CacheGroup, ) { super(); replay(this); @@ -338,7 +351,7 @@ class Layer extends EntityStore { layerId: string, replay: (layer: EntityStore) => any, ): EntityStore { - return new Layer(layerId, this, replay, this.depend); + return new Layer(layerId, this, replay, this.group); } public removeLayer(layerId: string): EntityStore { @@ -348,7 +361,7 @@ class Layer extends EntityStore { if (layerId === this.id) { // Dirty every ID we're removing. // TODO Some of these IDs could escape dirtying if value unchanged. - if (this.depend) { + if (this.group.caching) { Object.keys(this.data).forEach(dataId => this.delete(dataId)); } return parent; @@ -379,8 +392,9 @@ class Layer extends EntityStore { // from calling this.depend for every optimistic layer we examine, but // ensures we call this.depend in the last optimistic layer before we // reach the root layer. - if (this.depend && this.depend !== this.parent.depend) { - depend(this, dataId); + + if (this.group.caching && this.group !== this.parent.group) { + this.group.depend(dataId); } return this.parent.get(dataId); @@ -394,8 +408,8 @@ class Layer extends EntityStore { } } - if (this.depend && this.depend !== this.parent.depend) { - depend(this, dataId, storeFieldName); + if (this.group.caching && this.group !== this.parent.group) { + this.group.depend(dataId, storeFieldName); } return this.parent.getFieldValue(dataId, storeFieldName); @@ -473,7 +487,7 @@ const storeObjectReconciler: ReconcilerFunction<[EntityStore]> = function ( export function supportsResultCaching(store: any): store is EntityStore { // When result caching is disabled, store.depend will be null. - return !!(store instanceof EntityStore && store.depend); + return !!(store instanceof EntityStore && store.group.caching); } export function defaultNormalizedCacheFactory( diff --git a/src/cache/inmemory/inMemoryCache.ts b/src/cache/inmemory/inMemoryCache.ts index 909447238ba..13ac80df595 100644 --- a/src/cache/inmemory/inMemoryCache.ts +++ b/src/cache/inmemory/inMemoryCache.ts @@ -103,8 +103,8 @@ export class InMemoryCache extends ApolloCache { if (supportsResultCaching(store)) { const { optimistic, rootId, variables } = c; return cache.cacheKeyRoot.lookup( + store.group, c.query, - store, JSON.stringify({ optimistic, rootId, variables }), ); } diff --git a/src/cache/inmemory/readFromStore.ts b/src/cache/inmemory/readFromStore.ts index e3f08d148a9..b914c637145 100644 --- a/src/cache/inmemory/readFromStore.ts +++ b/src/cache/inmemory/readFromStore.ts @@ -105,7 +105,13 @@ export class StoreReader { }: ExecSelectionSetOptions) { if (supportsResultCaching(context.store)) { return cacheKeyRoot.lookup( - context.store, + // EntityStore objects share the same store.group if their + // dependencies are tracked together (for example, optimistic + // versus non-optimistic data), so we can reduce cache key + // diversity by using context.store.group here instead of just + // context.store, which promotes reusability of cached + // optimistic results. + context.store.group, selectionSet, JSON.stringify(context.variables), isReference(objectOrReference) ? objectOrReference.__ref : objectOrReference, @@ -120,7 +126,9 @@ export class StoreReader { makeCacheKey({ field, array, context }) { if (supportsResultCaching(context.store)) { return cacheKeyRoot.lookup( - context.store, + // See comment above about why context.store.group is used + // here, instead of context.store. + context.store.group, field, array, JSON.stringify(context.variables),