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.

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

Decouple canonicalStringify from ObjectCanon #11254

Merged
merged 18 commits into from
Oct 5, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
ff4bb9f
Move stub implementation of canonicalStringify to utilities/common.
benjamn Sep 26, 2023
729b48e
Trivial implementation of canonicalStringify that makes tests pass.
benjamn Sep 26, 2023
aeaf611
Avoid hack of calling getStoreKeyName.setStringify(canonicalStringify).
benjamn Sep 26, 2023
5d57cda
Improve canonicalStringify implementation using SortingTrie.
benjamn Sep 26, 2023
be458d3
Bump .size-limit.cjs limits slightly.
benjamn Sep 26, 2023
fa4ff5f
Add a changeset file.
benjamn Sep 26, 2023
50e485a
Make API extractor happy about canonicalStringify move.
benjamn Sep 26, 2023
9211fd4
Keep getStoreKeyName.setStringify but use canonicalStringify by default.
benjamn Sep 27, 2023
badac8b
Give `storeKeyNameStringify` an explicit type signature.
benjamn Sep 28, 2023
cbfdb9b
Better comment for `canonicalStringify` function.
benjamn Sep 28, 2023
c8cab39
Conserve total number of sorted arrays retained by canonicalStringify.
benjamn Sep 28, 2023
2311b8d
Use Map to avoid two-level trie nodes and Object.create(null).
benjamn Sep 28, 2023
1b4aad3
Add tests of canonicalStringify and helper lookupSortedKeys.
benjamn Sep 28, 2023
dad8607
Run prettier.
benjamn Sep 28, 2023
c5446ac
Switch to a simpler lookup strategy not involving a trie.
benjamn Sep 28, 2023
5636a34
Reduce size limits after simplifying canonicalStringify.
benjamn Sep 28, 2023
c2a8e34
Another comment, and one less call to Object.getPrototypeOf.
benjamn Sep 28, 2023
d0aa030
Replace isArraySorted with keys.every(everyKeyInOrder).
benjamn Oct 2, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 4 additions & 8 deletions .api-reports/api-report-cache.md
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ export const cacheSlot: {

// @public (undocumented)
export const canonicalStringify: ((value: any) => string) & {
reset: typeof resetCanonicalStringify;
reset(): void;
};

// @public (undocumented)
Expand Down Expand Up @@ -858,9 +858,6 @@ export interface Reference {
readonly __ref: string;
}

// @public (undocumented)
function resetCanonicalStringify(): void;

// @public (undocumented)
type SafeReadonly<T> = T extends object ? Readonly<T> : T;

Expand Down Expand Up @@ -947,10 +944,9 @@ interface WriteContext extends ReadMergeModifyContext {

// Warnings were encountered during analysis:
//
// src/cache/inmemory/object-canon.ts:203:32 - (ae-forgotten-export) The symbol "resetCanonicalStringify" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:98:3 - (ae-forgotten-export) The symbol "FragmentMap" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:167:3 - (ae-forgotten-export) The symbol "KeySpecifier" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:167:3 - (ae-forgotten-export) The symbol "KeyArgsFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:92:3 - (ae-forgotten-export) The symbol "FragmentMap" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:161:3 - (ae-forgotten-export) The symbol "KeySpecifier" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:161:3 - (ae-forgotten-export) The symbol "KeyArgsFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/types.ts:126:3 - (ae-forgotten-export) The symbol "KeyFieldsFunction" needs to be exported by the entry point index.d.ts

// (No @packageDocumentation comment for this package)
Expand Down
6 changes: 3 additions & 3 deletions .api-reports/api-report-core.md
Original file line number Diff line number Diff line change
Expand Up @@ -2179,9 +2179,9 @@ interface WriteContext extends ReadMergeModifyContext {

// Warnings were encountered during analysis:
//
// src/cache/inmemory/policies.ts:98:3 - (ae-forgotten-export) The symbol "FragmentMap" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:167:3 - (ae-forgotten-export) The symbol "KeySpecifier" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:167:3 - (ae-forgotten-export) The symbol "KeyArgsFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:92:3 - (ae-forgotten-export) The symbol "FragmentMap" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:161:3 - (ae-forgotten-export) The symbol "KeySpecifier" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:161:3 - (ae-forgotten-export) The symbol "KeyArgsFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/types.ts:126:3 - (ae-forgotten-export) The symbol "KeyFieldsFunction" needs to be exported by the entry point index.d.ts
// src/core/ObservableQuery.ts:112:5 - (ae-forgotten-export) The symbol "QueryManager" needs to be exported by the entry point index.d.ts
// src/core/ObservableQuery.ts:113:5 - (ae-forgotten-export) The symbol "QueryInfo" needs to be exported by the entry point index.d.ts
Expand Down
23 changes: 11 additions & 12 deletions .api-reports/api-report-utilities.md
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,11 @@ const enum CacheWriteBehavior {
OVERWRITE = 1
}

// @public (undocumented)
export const canonicalStringify: ((value: any) => string) & {
reset(): void;
};

// @public (undocumented)
type CanReadFunction = (value: StoreValue) => boolean;

Expand Down Expand Up @@ -1072,9 +1077,7 @@ export function getOperationName(doc: DocumentNode): string | null;
export function getQueryDefinition(doc: DocumentNode): OperationDefinitionNode;

// @public (undocumented)
export const getStoreKeyName: ((fieldName: string, args?: Record<string, any> | null, directives?: Directives) => string) & {
setStringify(s: typeof stringify): (value: any) => string;
};
export function getStoreKeyName(fieldName: string, args?: Record<string, any> | null, directives?: Directives): string;

// @public (undocumented)
export function getTypenameFromResult(result: Record<string, any>, selectionSet: SelectionSetNode, fragmentMap?: FragmentMap): string | undefined;
Expand Down Expand Up @@ -2298,9 +2301,6 @@ type StoreObjectValueMaybeReference<StoreVal> = StoreVal extends Record<string,
// @public (undocumented)
export type StoreValue = number | string | string[] | Reference | Reference[] | null | undefined | void | Object;

// @public (undocumented)
let stringify: (value: any) => string;

// @public (undocumented)
export function stringifyForDisplay(value: any, space?: number): string;

Expand Down Expand Up @@ -2498,11 +2498,11 @@ interface WriteContext extends ReadMergeModifyContext {
// Warnings were encountered during analysis:
//
// src/cache/core/types/DataProxy.ts:141:5 - (ae-forgotten-export) The symbol "MissingFieldError" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:63:3 - (ae-forgotten-export) The symbol "TypePolicy" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:167:3 - (ae-forgotten-export) The symbol "KeySpecifier" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:167:3 - (ae-forgotten-export) The symbol "KeyArgsFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:168:3 - (ae-forgotten-export) The symbol "FieldReadFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:169:3 - (ae-forgotten-export) The symbol "FieldMergeFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:57:3 - (ae-forgotten-export) The symbol "TypePolicy" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:161:3 - (ae-forgotten-export) The symbol "KeySpecifier" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:161:3 - (ae-forgotten-export) The symbol "KeyArgsFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:162:3 - (ae-forgotten-export) The symbol "FieldReadFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:163:3 - (ae-forgotten-export) The symbol "FieldMergeFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/types.ts:126:3 - (ae-forgotten-export) The symbol "KeyFieldsFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/writeToStore.ts:65:7 - (ae-forgotten-export) The symbol "MergeTree" needs to be exported by the entry point index.d.ts
// src/core/ApolloClient.ts:47:3 - (ae-forgotten-export) The symbol "UriFunction" needs to be exported by the entry point index.d.ts
Expand All @@ -2517,7 +2517,6 @@ interface WriteContext extends ReadMergeModifyContext {
// 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/storeUtils.ts:202:12 - (ae-forgotten-export) The symbol "stringify" needs to be exported by the entry point index.d.ts
// src/utilities/policies/pagination.ts:76:3 - (ae-forgotten-export) The symbol "TRelayEdge" needs to be exported by the entry point index.d.ts
// src/utilities/policies/pagination.ts:77:3 - (ae-forgotten-export) The symbol "TRelayPageInfo" needs to be exported by the entry point index.d.ts

Expand Down
6 changes: 3 additions & 3 deletions .api-reports/api-report.md
Original file line number Diff line number Diff line change
Expand Up @@ -2858,9 +2858,9 @@ interface WriteContext extends ReadMergeModifyContext {

// Warnings were encountered during analysis:
//
// src/cache/inmemory/policies.ts:98:3 - (ae-forgotten-export) The symbol "FragmentMap" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:167:3 - (ae-forgotten-export) The symbol "KeySpecifier" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:167:3 - (ae-forgotten-export) The symbol "KeyArgsFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:92:3 - (ae-forgotten-export) The symbol "FragmentMap" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:161:3 - (ae-forgotten-export) The symbol "KeySpecifier" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/policies.ts:161:3 - (ae-forgotten-export) The symbol "KeyArgsFunction" needs to be exported by the entry point index.d.ts
// src/cache/inmemory/types.ts:126:3 - (ae-forgotten-export) The symbol "KeyFieldsFunction" needs to be exported by the entry point index.d.ts
// src/core/ObservableQuery.ts:112:5 - (ae-forgotten-export) The symbol "QueryManager" needs to be exported by the entry point index.d.ts
// src/core/ObservableQuery.ts:113:5 - (ae-forgotten-export) The symbol "QueryInfo" needs to be exported by the entry point index.d.ts
Expand Down
5 changes: 5 additions & 0 deletions .changeset/beige-geese-wink.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/client": patch
---

Decouple `canonicalStringify` from `ObjectCanon` for better time and memory performance.
4 changes: 2 additions & 2 deletions .size-limit.cjs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
const checks = [
{
path: "dist/apollo-client.min.cjs",
limit: "38000",
limit: "38060",
},
{
path: "dist/main.cjs",
Expand All @@ -10,7 +10,7 @@ const checks = [
{
path: "dist/index.js",
import: "{ ApolloClient, InMemoryCache, HttpLink }",
limit: "32052",
limit: "32095",
},
...[
"ApolloProvider",
Expand Down
1 change: 1 addition & 0 deletions src/__tests__/__snapshots__/exports.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,7 @@ Array [
"canUseSymbol",
"canUseWeakMap",
"canUseWeakSet",
"canonicalStringify",
"checkDocument",
"cloneDeep",
"compact",
Expand Down
8 changes: 5 additions & 3 deletions src/cache/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,11 @@ export type {
export { MissingFieldError } from "./core/types/common.js";

export type { Reference } from "../utilities/index.js";
export { isReference, makeReference } from "../utilities/index.js";
export {
isReference,
makeReference,
canonicalStringify,
} from "../utilities/index.js";

export { EntityStore } from "./inmemory/entityStore.js";
export {
Expand All @@ -38,8 +42,6 @@ export type {
} from "./inmemory/policies.js";
export { Policies } from "./inmemory/policies.js";

export { canonicalStringify } from "./inmemory/object-canon.js";

export type { FragmentRegistryAPI } from "./inmemory/fragmentRegistry.js";
export { createFragmentRegistry } from "./inmemory/fragmentRegistry.js";

Expand Down
2 changes: 1 addition & 1 deletion src/cache/inmemory/__tests__/key-extractor.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { KeySpecifier } from "../policies";
import { canonicalStringify } from "../object-canon";
import { canonicalStringify } from "../../../utilities";
import {
getSpecifierPaths,
collectSpecifierPaths,
Expand Down
2 changes: 1 addition & 1 deletion src/cache/inmemory/inMemoryCache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
addTypenameToDocument,
isReference,
DocumentTransform,
canonicalStringify,
} from "../../utilities/index.js";
import type { InMemoryCacheConfig, NormalizedCacheObject } from "./types.js";
import { StoreReader } from "./readFromStore.js";
Expand All @@ -24,7 +25,6 @@ import { EntityStore, supportsResultCaching } from "./entityStore.js";
import { makeVar, forgetCache, recallCache } from "./reactiveVars.js";
import { Policies } from "./policies.js";
import { hasOwn, normalizeConfig, shouldCanonizeResults } from "./helpers.js";
import { canonicalStringify } from "./object-canon.js";
import type { OperationVariables } from "../../core/index.js";

type BroadcastOptions = Pick<
Expand Down
32 changes: 0 additions & 32 deletions src/cache/inmemory/object-canon.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,35 +195,3 @@ type SortedKeysInfo = {
sorted: string[];
json: string;
};

// Since the keys of canonical objects are always created in lexicographically
// sorted order, we can use the ObjectCanon to implement a fast and stable
// version of JSON.stringify, which automatically sorts object keys.
export const canonicalStringify = Object.assign(
function (value: any): string {
if (isObjectOrArray(value)) {
if (stringifyCanon === void 0) {
resetCanonicalStringify();
}
const canonical = stringifyCanon.admit(value);
let json = stringifyCache.get(canonical);
if (json === void 0) {
stringifyCache.set(canonical, (json = JSON.stringify(canonical)));
}
phryneas marked this conversation as resolved.
Show resolved Hide resolved
return json;
}
return JSON.stringify(value);
},
{
reset: resetCanonicalStringify,
}
);

// Can be reset by calling canonicalStringify.reset().
let stringifyCanon: ObjectCanon;
let stringifyCache: WeakMap<object, string>;

function resetCanonicalStringify() {
stringifyCanon = new ObjectCanon();
stringifyCache = new (canUseWeakMap ? WeakMap : Map)();
}
6 changes: 0 additions & 6 deletions src/cache/inmemory/policies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,17 +48,11 @@ import type {
} from "../core/types/common.js";
import type { WriteContext } from "./writeToStore.js";

// Upgrade to a faster version of the default stable JSON.stringify function
// used by getStoreKeyName. This function is used when computing storeFieldName
// strings (when no keyArgs has been configured for a field).
import { canonicalStringify } from "./object-canon.js";
import {
keyArgsFnFromSpecifier,
keyFieldsFnFromSpecifier,
} from "./key-extractor.js";

getStoreKeyName.setStringify(canonicalStringify);

export type TypePolicies = {
[__typename: string]: TypePolicy;
};
Expand Down
3 changes: 2 additions & 1 deletion src/cache/inmemory/readFromStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
isNonNullObject,
canUseWeakMap,
compact,
canonicalStringify,
} from "../../utilities/index.js";
import type { Cache } from "../core/types/Cache.js";
import type {
Expand All @@ -50,7 +51,7 @@ import type { Policies } from "./policies.js";
import type { InMemoryCache } from "./inMemoryCache.js";
import type { MissingTree } from "../core/types/common.js";
import { MissingFieldError } from "../core/types/common.js";
import { canonicalStringify, ObjectCanon } from "./object-canon.js";
import { ObjectCanon } from "./object-canon.js";

export type VariableMap = { [name: string]: any };

Expand Down
2 changes: 1 addition & 1 deletion src/cache/inmemory/writeToStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
addTypenameToDocument,
isNonEmptyArray,
argumentsObjectFromField,
canonicalStringify,
} from "../../utilities/index.js";

import type {
Expand All @@ -44,7 +45,6 @@ import type { StoreReader } from "./readFromStore.js";
import type { InMemoryCache } from "./inMemoryCache.js";
import type { EntityStore } from "./entityStore.js";
import type { Cache } from "../../core/index.js";
import { canonicalStringify } from "./object-canon.js";
import { normalizeReadFieldOptions } from "./policies.js";
import type { ReadFieldFunction } from "../core/types/common.js";

Expand Down
78 changes: 78 additions & 0 deletions src/utilities/common/canonicalStringify.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
// Like JSON.stringify, but with object keys always sorted in the same order.
benjamn marked this conversation as resolved.
Show resolved Hide resolved
export const canonicalStringify = Object.assign(
function canonicalStringify(value: any): string {
return JSON.stringify(value, stableObjectReplacer);
},
{
reset() {
// Blowing away the root-level trie map will reclaim all memory stored in
// the trie, without affecting the logical results of canonicalStringify,
// but potentially sacrificing performance until the trie is refilled.
sortingTrieRoot.next = Object.create(null);
},
}
);

// The JSON.stringify function takes an optional second argument called a
// replacer function. This function is called for each key-value pair in the
// object being stringified, and its return value is used instead of the
// original value. If the replacer function returns a new value, that value is
// stringified as JSON instead of the original value of the property.
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#the_replacer_parameter
function stableObjectReplacer(key: string, value: any) {
benjamn marked this conversation as resolved.
Show resolved Hide resolved
if (value && typeof value === "object") {
const proto = Object.getPrototypeOf(value);
// We don't want to mess with objects that are not "plain" objects, which
// means their prototype is either Object.prototype or null.
if (proto === Object.prototype || proto === null) {
const keys = Object.keys(value);
const sortedKeys = sortKeys(keys);
if (sortedKeys !== keys) {
const sorted = Object.create(null);
// Reassigning the keys in sorted order will cause JSON.stringify to
// serialize them in sorted order.
sortedKeys.forEach((key) => {
sorted[key] = value[key];
});
return sorted;
}
}
}
return value;
}

interface SortingTrie {
sorted?: readonly string[];
next: Record<string, SortingTrie>;
}

const sortingTrieRoot: SortingTrie = {
sorted: [],
next: Object.create(null),
};

// Sort the given keys using a lookup trie, and return the same (===) array in
// case it was already sorted, so we can avoid always creating a new object in
// the replacer function above.
function sortKeys(keys: readonly string[]): readonly string[] {
let node = sortingTrieRoot;
let alreadySorted = true;
for (let k = 0, len = keys.length; k < len; ++k) {
const key = keys[k];
if (k > 0 && keys[k - 1] > key) {
alreadySorted = false;
}
const next = node.next;
node = next[key] || (next[key] = { next: Object.create(null) });
}
if (alreadySorted) {
// There may already be a node.sorted array that's equivalent to the
// already-sorted keys array, but if keys was already sorted, we always want
// to return that array, not node.sorted.
return node.sorted ? keys : (node.sorted = keys);
phryneas marked this conversation as resolved.
Show resolved Hide resolved
}
// The .slice(0) is necessary so that we do not modify the original keys array
// by calling keys.sort(), and also so that we always return a new (!==)
// sorted array when keys was not already sorted.
return node.sorted || (node.sorted = keys.slice(0).sort());
Copy link
Member Author

Choose a reason for hiding this comment

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

Fun TC39/ECMAScript fact: thanks to this proposal which recently reached stage 4, we will eventually be able to use keys.toSorted() instead of keys.slice(0).sort(). "Eventually" because even evergreen browsers take some time to ship new features.

Copy link
Member

Choose a reason for hiding this comment

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

At least it's one of those things that can easily be polyfilled in userland, so we can make that switch in two years or so :)

}
phryneas marked this conversation as resolved.
Show resolved Hide resolved