Improve read performance for complicated queries by up to 50% (#3300)

* Add missing typecast in diffQueryAgainstStore.

* Optimize merge function in graphql-anywhere.

This is extremely hot code, so small improvements matter.

Key observation: dest tends to be the current result object, which can
have a lot of keys that are irrelevant to the keys from src that we're
trying to merge, so the real win here is to avoid computing and iterating
over the keys of dest.

* Stop needlessly printing stored query documents.

As you can see from these changes, we weren't using the printed query
string for anything except error checking, and the query document object
should suffice for that.

* Stop using Object.defineProperty in resultMapper.

I respectfully disagree with the comment about hiding the ID field, since
the property is still discoverable in principle. If we really wanted to
keep hiding it, we could do that only in development.

Simple property assignment is not only faster than calling
Object.defineProperty, but also makes subsequent usage of the object
faster. Kind of a shame that defineProperty is still slow in 2018, but
that's the world we live in.

* Use Object.create(null) instead of {} for ObjectCache.

If you're going to use an Object for a cache, you don't want keys like
"toString" returning truthy values, and it's slightly faster to look up
nonexistent keys if the prototype chain is shorter.

* Optimize readStoreResolver.

Not calling getStoreKeyName when we know it's just going to return its
first argument unmodified makes a noticeable difference here.

Not looking up values in an empty object {} also helps.

* Update CHANGELOG.md files for PR #3300.

* Add myself to the apollo-client/AUTHORS file.

* Address feedback from @jamesreggio.

packages/apollo-cache-inmemory/CHANGELOG.md

@@ -1,6 +1,7 @@
 # Change log
 
 ### vNext
+- Various optimizations for cache read performance [#3300](https://github.com/apollographql/apollo-client/pull/3300)
 - Fix typo in documentation
 
 ### 1.1.12

packages/apollo-cache-inmemory/src/objectCache.ts

@@ -1,7 +1,7 @@
 import { NormalizedCache, NormalizedCacheObject, StoreObject } from './types';
 
 export class ObjectCache implements NormalizedCache {
-  constructor(private data: NormalizedCacheObject = {}) {}
+  constructor(private data: NormalizedCacheObject = Object.create(null)) {}
   public toObject(): NormalizedCacheObject {
     return this.data;
   }
@@ -15,10 +15,10 @@ export class ObjectCache implements NormalizedCache {
     this.data[dataId] = undefined;
   }
   public clear(): void {
-    this.data = {};
+    this.data = Object.create(null);
   }
   public replace(newData: NormalizedCacheObject): void {
-    this.data = newData || {};
+    this.data = newData || Object.create(null);
   }
 }
 

packages/apollo-cache-inmemory/src/readFromStore.ts

@@ -10,14 +10,17 @@ import {
   isIdValue,
   toIdValue,
   getStoreKeyName,
+  StoreValue,
 } from 'apollo-utilities';
+
 import { Cache } from 'apollo-cache';
 
 import {
   ReadQueryOptions,
   IdValueWithPreviousResult,
   ReadStoreContext,
   DiffQueryAgainstStoreOptions,
+  StoreObject,
 } from './types';
 
 /**
@@ -67,13 +70,24 @@ const readStoreResolver: Resolver = (
 
   const objId = idValue.id;
   const obj = context.store.get(objId);
-  const storeKeyName = getStoreKeyName(fieldName, args, directives);
-  let fieldValue = (obj || {})[storeKeyName];
 
-  if (typeof fieldValue === 'undefined') {
+  let storeKeyName = fieldName;
+  if (args || directives) {
+    // We happen to know here that getStoreKeyName returns its first
+    // argument unmodified if there are no args or directives, so we can
+    // avoid calling the function at all in that case, as a small but
+    // important optimization to this frequently executed code.
+    storeKeyName = getStoreKeyName(storeKeyName, args, directives);
+  }
+
+  let fieldValue: StoreValue | string | void = void 0;
+
+  if (obj) {
+    fieldValue = obj[storeKeyName];
+
     if (
+      typeof fieldValue === 'undefined' &&
       context.cacheRedirects &&
-      obj &&
       (obj.__typename || objId === 'ROOT_QUERY')
     ) {
       const typename = obj.__typename || 'Query';
@@ -85,11 +99,12 @@ const readStoreResolver: Resolver = (
         const resolver = type[fieldName];
         if (resolver) {
           fieldValue = resolver(obj, args, {
-            getCacheKey: (obj: { __typename: string; id: string | number }) =>
-              toIdValue({
-                id: context.dataIdFromObject(obj),
-                typename: obj.__typename,
-              }),
+            getCacheKey(storeObj: StoreObject) {
+              return toIdValue({
+                id: context.dataIdFromObject(storeObj),
+                typename: storeObj.__typename,
+              });
+            },
           });
         }
       }
@@ -193,7 +208,7 @@ export function diffQueryAgainstStore<T>({
   );
 
   return {
-    result,
+    result: result as T,
     complete: !context.hasMissingField,
   };
 }
@@ -300,14 +315,7 @@ function resultMapper(resultFields: any, idValue: IdValueWithPreviousResult) {
     }
   }
 
-  // Add the id to the result fields. It should be non-enumerable so users can’t see it without
-  // trying very hard.
-  Object.defineProperty(resultFields, ID_KEY, {
-    enumerable: false,
-    configurable: false,
-    writable: false,
-    value: idValue.id,
-  });
+  resultFields[ID_KEY] = idValue.id;
 
   return resultFields;
 }

packages/apollo-client/AUTHORS

@@ -4,6 +4,7 @@ Abhi Aiyer <abhiaiyer91@gmail.com>
 Avindra Goolcharan <aavindraa@gmail.com>
 Bazyli Brzóska <bazyli.brzoska@gmail.com>
 Ben James <benhjames@sky.com>
+Ben Newman <ben@apollographql.com>
 Ben Straub <ben@straub.cc>
 Brady Whitten <bwhitten518@gmail.com>
 Brett Jurgens <brett@brettjurgens.com>

packages/apollo-client/CHANGELOG.md

@@ -2,8 +2,8 @@
 # Change log
 
 ### vNEXT
-
 - fixed edge case bug of changing fetchPolicies right after resetStore with no variables present
+- Various optimizations for cache read performance [#3300](https://github.com/apollographql/apollo-client/pull/3300)
 
 ### 2.2.8
 - Added the graphQLResultHasError in QueryManager.ts to check not only if result.errors is null, but also empty.

packages/apollo-client/src/core/QueryManager.ts

@@ -343,7 +343,6 @@ export class QueryManager<TStore> {
 
     this.queryStore.initQuery({
       queryId,
-      queryString: print(query),
       document: query,
       storePreviousVariables: shouldFetch,
       variables,
@@ -512,7 +511,7 @@ export class QueryManager<TStore> {
               console.info(
                 'An unhandled error was thrown because no error handler is registered ' +
                   'for the query ' +
-                  queryStoreValue.queryString,
+                  print(queryStoreValue.document),
               );
             }
           }

packages/apollo-client/src/data/queries.ts

@@ -1,10 +1,10 @@
 import { DocumentNode, GraphQLError, ExecutionResult } from 'graphql';
+import { print } from 'graphql/language/printer';
 import { isEqual } from 'apollo-utilities';
 
 import { NetworkStatus } from '../core/networkStatus';
 
 export type QueryStoreValue = {
-  queryString: string;
   document: DocumentNode;
   variables: Object;
   previousVariables?: Object | null;
@@ -27,7 +27,6 @@ export class QueryStore {
 
   public initQuery(query: {
     queryId: string;
-    queryString: string;
     document: DocumentNode;
     storePreviousVariables: boolean;
     variables: Object;
@@ -38,7 +37,11 @@ export class QueryStore {
   }) {
     const previousQuery = this.store[query.queryId];
 
-    if (previousQuery && previousQuery.queryString !== query.queryString) {
+    if (
+      previousQuery &&
+      previousQuery.document !== query.document &&
+      print(previousQuery.document) !== print(query.document)
+    ) {
       // XXX we're throwing an error here to catch bugs where a query gets overwritten by a new one.
       // we should implement a separate action for refetching so that QUERY_INIT may never overwrite
       // an existing query (see also: https://github.com/apollostack/apollo-client/issues/732)
@@ -84,7 +87,6 @@ export class QueryStore {
     // the store. We probably want a refetch action instead, because I suspect that if you refetch
     // before the initial fetch is done, you'll get an error.
     this.store[query.queryId] = {
-      queryString: query.queryString,
       document: query.document,
       variables: query.variables,
       previousVariables,

packages/graphql-anywhere/CHANGELOG.md

@@ -1,6 +1,7 @@
 # Change log
 
 ## vNEXT
+- Various optimizations for cache read performance [#3300](https://github.com/apollographql/apollo-client/pull/3300)
 
 ### 4.1.8
 - Map coverage to original source

packages/graphql-anywhere/src/graphql.ts

@@ -228,23 +228,15 @@ function executeSubSelectedArray(field, result, execContext) {
   });
 }
 
-export function merge(dest, src) {
-  if (src === null || typeof src !== 'object') {
-    // These types just override whatever was in dest
-    return src;
-  }
-
-  // Merge sub-objects
-  Object.keys(dest).forEach(destKey => {
-    if (src.hasOwnProperty(destKey)) {
-      merge(dest[destKey], src[destKey]);
-    }
-  });
+const hasOwn = Object.prototype.hasOwnProperty;
 
-  // Add props only on src
-  Object.keys(src).forEach(srcKey => {
-    if (!dest.hasOwnProperty(srcKey)) {
-      dest[srcKey] = src[srcKey];
+export function merge(dest, src) {
+  Object.keys(src).forEach(key => {
+    const srcVal = src[key];
+    if (!hasOwn.call(dest, key)) {
+      dest[key] = srcVal;
+    } else if (srcVal && typeof srcVal === 'object') {
+      merge(dest[key], srcVal);
     }
   });
 }