From f23ffb87ef74e19f02f45c5d5cd4ebc92002b260 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Mon, 23 Nov 2020 18:31:13 -0500 Subject: [PATCH] Efficiently canonicalize InMemoryCache result objects. https://github.com/apollographql/apollo-client/issues/4141#issuecomment-733091694 --- src/__tests__/client.ts | 12 +-- src/cache/inmemory/__tests__/cache.ts | 7 +- src/cache/inmemory/__tests__/optimistic.ts | 2 +- src/cache/inmemory/__tests__/policies.ts | 13 +-- src/cache/inmemory/canon.ts | 106 +++++++++++++++++++++ src/cache/inmemory/helpers.ts | 5 +- src/cache/inmemory/readFromStore.ts | 17 ++-- src/core/__tests__/QueryManager/index.ts | 5 +- 8 files changed, 124 insertions(+), 43 deletions(-) create mode 100644 src/cache/inmemory/canon.ts diff --git a/src/__tests__/client.ts b/src/__tests__/client.ts index 9b38bd585cb..fb53375b01a 100644 --- a/src/__tests__/client.ts +++ b/src/__tests__/client.ts @@ -3024,11 +3024,9 @@ describe('@connection', () => { client.cache.evict({ fieldName: "a" }); await wait(); - // The results are structurally the same, but the result objects have - // been recomputed for queries that involved the ROOT_QUERY.a field. - expect(checkLastResult(aResults, a456)).not.toBe(a456); + expect(checkLastResult(aResults, a456)).toBe(a456); expect(checkLastResult(bResults, bOyez)).toBe(bOyez); - expect(checkLastResult(abResults, a456bOyez)).not.toBe(a456bOyez); + expect(checkLastResult(abResults, a456bOyez)).toBe(a456bOyez); const cQuery = gql`{ c }`; // Passing cache-only as the fetchPolicy allows the { c: "see" } @@ -3077,16 +3075,12 @@ describe('@connection', () => { { a: 123 }, { a: 234 }, { a: 456 }, - // Delivered again because we explicitly called resetLastResults. - { a: 456 }, ]); expect(bResults).toEqual([ { b: "asdf" }, { b: "ASDF" }, { b: "oyez" }, - // Delivered again because we explicitly called resetLastResults. - { b: "oyez" }, ]); expect(abResults).toEqual([ @@ -3094,8 +3088,6 @@ describe('@connection', () => { { a: 234, b: "asdf" }, { a: 234, b: "ASDF" }, { a: 456, b: "oyez" }, - // Delivered again because we explicitly called resetLastResults. - { a: 456, b: "oyez" }, ]); expect(cResults).toEqual([ diff --git a/src/cache/inmemory/__tests__/cache.ts b/src/cache/inmemory/__tests__/cache.ts index f58542e4449..1e4089893c7 100644 --- a/src/cache/inmemory/__tests__/cache.ts +++ b/src/cache/inmemory/__tests__/cache.ts @@ -1728,8 +1728,8 @@ describe("InMemoryCache#modify", () => { })).toBe(false); // Nothing actually modified. const resultAfterAuthorInvalidation = read(); - expect(resultAfterAuthorInvalidation).not.toBe(initialResult); expect(resultAfterAuthorInvalidation).toEqual(initialResult); + expect(resultAfterAuthorInvalidation).toBe(initialResult); expect(cache.modify({ id: cache.identify({ @@ -1743,8 +1743,8 @@ describe("InMemoryCache#modify", () => { })).toBe(false); // Nothing actually modified. const resultAfterBookInvalidation = read(); - expect(resultAfterBookInvalidation).not.toBe(resultAfterAuthorInvalidation); expect(resultAfterBookInvalidation).toEqual(resultAfterAuthorInvalidation); + expect(resultAfterBookInvalidation).toBe(resultAfterAuthorInvalidation); expect(resultAfterBookInvalidation.currentlyReading.author).toEqual({ __typename: "Author", name: "Maria Dahvana Headley", @@ -2591,9 +2591,8 @@ describe("ReactiveVar and makeVar", () => { }); const result2 = cache.readQuery({ query }); - // Without resultCaching, equivalent results will not be ===. - expect(result2).not.toBe(result1); expect(result2).toEqual(result1); + expect(result2).toBe(result1); expect(nameVar()).toBe("Ben"); expect(nameVar("Hugh")).toBe("Hugh"); diff --git a/src/cache/inmemory/__tests__/optimistic.ts b/src/cache/inmemory/__tests__/optimistic.ts index b5d7b79a29f..8030d43ed49 100644 --- a/src/cache/inmemory/__tests__/optimistic.ts +++ b/src/cache/inmemory/__tests__/optimistic.ts @@ -431,7 +431,7 @@ describe('optimistic cache layers', () => { const resultAfterRemovingBuzzLayer = readWithAuthors(); expect(resultAfterRemovingBuzzLayer).toEqual(resultWithBuzz); - expect(resultAfterRemovingBuzzLayer).not.toBe(resultWithBuzz); + expect(resultAfterRemovingBuzzLayer).toBe(resultWithBuzz); resultWithTwoAuthors.books.forEach((book, i) => { expect(book).toEqual(resultAfterRemovingBuzzLayer.books[i]); expect(book).toBe(resultAfterRemovingBuzzLayer.books[i]); diff --git a/src/cache/inmemory/__tests__/policies.ts b/src/cache/inmemory/__tests__/policies.ts index 452a6c377f7..75115f2edb9 100644 --- a/src/cache/inmemory/__tests__/policies.ts +++ b/src/cache/inmemory/__tests__/policies.ts @@ -4692,19 +4692,8 @@ describe("type policies", function () { }); const thirdFirstBookResult = readFirstBookResult(); - - // A change in VW's books field triggers rereading of result objects - // that previously involved her books field. - expect(thirdFirstBookResult).not.toBe(secondFirstBookResult); - - // However, since the new Book was not the earliest published, the - // second and third results are structurally the same. expect(thirdFirstBookResult).toEqual(secondFirstBookResult); - - // In fact, the original author.firstBook object has been reused! - expect(thirdFirstBookResult.author.firstBook).toBe( - secondFirstBookResult.author.firstBook, - ); + expect(thirdFirstBookResult).toBe(secondFirstBookResult); }); it("readField can read fields with arguments", function () { diff --git a/src/cache/inmemory/canon.ts b/src/cache/inmemory/canon.ts new file mode 100644 index 00000000000..563cd7683e0 --- /dev/null +++ b/src/cache/inmemory/canon.ts @@ -0,0 +1,106 @@ +import { KeyTrie } from "optimism"; +import { canUseWeakMap } from "../../utilities"; +import { objToStr } from "./helpers"; + +// When we say an object is "canonical" in programming, we mean it has +// earned a place in some abstract "canon" of official/blessed objects. +// This Canon class is a representation of such a collection, with the +// property that canon.add(value1) === canon.add(value2) if value1 and +// value2 are deeply equal to each other. The canonicalization process +// involves looking at every property in the provided object tree, so it +// takes the same order of time as deep equality checking (linear time), +// but already-canonized objects are returned immediately from canon.add, +// so ensuring subtrees have already been canonized tends to speed up +// canonicalization. Of course, since canonized objects may be shared +// widely between unrelated consumers, it's important to regard them as +// immutable. No detection of cycles is needed by the StoreReader class +// right now, so we don't bother keeping track of objects we've already +// seen during the recursion of the add method. Objects whose internal +// class name is neither Array nor Object can be included in the value +// tree, but they will not be replaced with a canonical version (to put it +// another way, they are assumed to be canonical already). We can easily +// add additional cases to the switch statement to handle other common +// object types, such as "[object Date]" objects, as needed. +export class Canon { + // All known objects this Canon has returned. + private known = new (canUseWeakMap ? WeakSet : Set)(); + + // Efficient storage/lookup structure for known objects. + private pool = new KeyTrie<{ + array?: any[]; + object?: Record; + keys?: { + sorted: string[]; + json: string; + }; + }>(canUseWeakMap); + + // Returns the canonical version of value. + public add(value: T): T; + public add(value: any) { + if (value && typeof value === "object") { + switch (objToStr.call(value)) { + case "[object Array]": { + if (this.known.has(value)) return value; + const array: any[] = value.map(this.add, this); + // Arrays are looked up in the KeyTrie using their recursively + // canonicalized elements, and the known version of the array is + // preserved as node.array. + const node = this.pool.lookupArray(array); + if (!node.array) { + this.known.add(node.array = array); + if (process.env.NODE_ENV !== "production") { + Object.freeze(array); + } + } + return node.array; + } + + case "[object Object]": { + if (this.known.has(value)) return value; + const proto = Object.getPrototypeOf(value); + const array = [proto]; + const keys = this.sortedKeys(value); + array.push(keys.json); + keys.sorted.forEach(key => { + array.push(this.add(value[key])); + }); + // Objects are looked up in the KeyTrie by their prototype + // (which is *not* recursively canonicalized), followed by a + // JSON representation of their (sorted) keys, followed by the + // sequence of recursively canonicalized values corresponding to + // those keys. To keep the final results unambiguous with other + // sequences (such as arrays that just happen to contain [proto, + // keys.json, value1, value2, ...]), the known version of the + // object is stored as node.object. + const node = this.pool.lookupArray(array); + if (!node.object) { + const obj = node.object = Object.create(proto); + this.known.add(obj); + keys.sorted.forEach((key, i) => { + obj[key] = array[i + 2]; + }); + if (process.env.NODE_ENV !== "production") { + Object.freeze(obj); + } + } + return node.object; + } + } + } + return value; + } + + // It's worthwhile to cache the sorting of arrays of strings, since the + // same initial unsorted arrays tend to be encountered many times. + // Fortunately, we can reuse the KeyTrie machinery to look up the sorted + // arrays in linear time (which is faster than sorting large arrays). + private sortedKeys(obj: object) { + const keys = Object.keys(obj); + const node = this.pool.lookupArray(keys); + return node.keys || (node.keys = { + sorted: keys.sort(), + json: JSON.stringify(keys), + }); + } +} diff --git a/src/cache/inmemory/helpers.ts b/src/cache/inmemory/helpers.ts index dc12bc65838..9777c8330a0 100644 --- a/src/cache/inmemory/helpers.ts +++ b/src/cache/inmemory/helpers.ts @@ -12,7 +12,10 @@ import { shouldInclude, } from '../../utilities'; -export const hasOwn = Object.prototype.hasOwnProperty; +export const { + hasOwnProperty: hasOwn, + toString: objToStr, +} = Object.prototype; export function getTypenameFromStoreObject( store: NormalizedCache, diff --git a/src/cache/inmemory/readFromStore.ts b/src/cache/inmemory/readFromStore.ts index 1abafb40f30..9354701ccd3 100644 --- a/src/cache/inmemory/readFromStore.ts +++ b/src/cache/inmemory/readFromStore.ts @@ -36,6 +36,7 @@ import { getTypenameFromStoreObject } from './helpers'; import { Policies } from './policies'; import { InMemoryCache } from './inMemoryCache'; import { MissingFieldError } from '../core/types/common'; +import { Canon } from './canon'; export type VariableMap = { [name: string]: any }; @@ -324,11 +325,7 @@ export class StoreReader { // Perform a single merge at the end so that we can avoid making more // defensive shallow copies than necessary. - finalResult.result = mergeDeepArray(objectsToMerge); - - if (process.env.NODE_ENV !== 'production') { - Object.freeze(finalResult.result); - } + finalResult.result = this.canon.add(mergeDeepArray(objectsToMerge)); // Store this result with its selection set so that we can quickly // recognize it again in the StoreReader#isFresh method. @@ -337,6 +334,8 @@ export class StoreReader { return finalResult; } + private canon = new Canon; + private knownResults = new WeakMap, SelectionSetNode>(); // Cached version of execSubSelectedArrayImpl. @@ -377,7 +376,7 @@ export class StoreReader { array = array.filter(context.store.canRead); } - array = array.map((item, i) => { + array = this.canon.add(array.map((item, i) => { // null value in array if (item === null) { return null; @@ -410,11 +409,7 @@ export class StoreReader { invariant(context.path.pop() === i); return item; - }); - - if (process.env.NODE_ENV !== 'production') { - Object.freeze(array); - } + })); return { result: array, missing }; } diff --git a/src/core/__tests__/QueryManager/index.ts b/src/core/__tests__/QueryManager/index.ts index db5669aa2f1..23ebb44b27b 100644 --- a/src/core/__tests__/QueryManager/index.ts +++ b/src/core/__tests__/QueryManager/index.ts @@ -902,10 +902,7 @@ describe('QueryManager', () => { break; case 2: expect(stripSymbols(result.data)).toEqual(data3); - expect(result.data).not.toBe(firstResultData); - expect(result.data.b).toEqual(firstResultData.b); - expect(result.data.d).not.toBe(firstResultData.d); - expect(result.data.d.f).toEqual(firstResultData.d.f); + expect(result.data).toBe(firstResultData); resolve(); break; default: