Avoid copying entire cache on each optimistic read. #4319

benjamn · 2019-01-16T23:08:29Z

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 === 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 #4210 much less severe, though disabling dependency tracking for optimistic reads (the previous solution) still seems like a good idea.

benjamn · 2019-01-16T23:13:45Z

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

+    return this;
+  }
+
+  public toObject(): NormalizedCacheObject {


It seemed important to implement the toObject method correctly, but one big benefit of this PR is that toObject should no longer be called in the course of normal InMemoryCache usage. Maybe if you happen to call cache.extract(true) while optimistic updates are active, but only then.

benjamn · 2019-01-16T23:15:42Z

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

      return null;
    }

-    const store =
-      query.optimistic && this.optimistic.length
-        ? new ObjectCache(this.extract(true))


This line was previously really expensive.

benjamn · 2019-01-16T23:15:51Z

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

@@ -174,13 +203,8 @@ export class InMemoryCache extends ApolloCache<NormalizedCacheObject> {
  }

  public diff<T>(query: Cache.DiffOptions): Cache.DiffResult<T> {
-    const store =
-      query.optimistic && this.optimistic.length
-        ? new ObjectCache(this.extract(true))


This line was previously really expensive, too.

benjamn · 2019-01-16T23:16:42Z

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

-
-    // Re-run all of our optimistic data actions on top of one another.
-    toPerform.forEach(change => {
-      this.recordOptimisticTransaction(change.transaction, change.id);


Every single time we called this.recordOptimisticTransaction here, the entire cache was copied.

benjamn · 2019-01-16T23:20:02Z

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)) {}


The only functional change in this module was making data a protected rather than private member, since we need to access it in the OptimisticCacheLayer subclass.

benjamn · 2019-01-17T15:35:07Z

packages/apollo-cache-inmemory/src/__tests__/recordingCache.ts

 import { NormalizedCacheObject } from '../types';

-describe('RecordingCache', () => {
+describe('OptimisticCacheLayer', () => {


I was tempted just to git rm src/__tests__/recordingCache.ts, since src/recordingCache.ts no longer exists, but then I realized these tests could be adapted to use the new OptimisticCacheLayer class instead. Similar in spirit, though very different in implementation.

benjamn · 2019-01-17T15:52:52Z

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';


I very much doubt anyone was using the RecordingCache outside of the tests in this package, though I suppose this could be considered a minor breaking change.

mxstbr · 2019-01-17T16:50:12Z

SO PUMPED ABOUT THIS!!!!

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 #4210 much less severe, though disabling dependency tracking for optimistic reads (the previous solution) still seems like a good idea.

…thods. 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.

hwillson

👍 👍 👍 @benjamn - so great! L(immediately)GTM!

benjamn · 2019-01-17T21:40:30Z

This has been released in [email protected]. The minor version bump is just because RecordingCache is no longer exported. These changes should be backwards-compatible otherwise.

benjamn self-assigned this Jan 16, 2019

benjamn requested a review from hwillson January 16, 2019 23:08

benjamn added the 🐎 performance label Jan 16, 2019

benjamn commented Jan 16, 2019

View reviewed changes

benjamn mentioned this pull request Jan 17, 2019

Memory leak with optimistic response #4210

Closed

benjamn commented Jan 17, 2019

View reviewed changes

benjamn requested a review from jbaxleyiii January 17, 2019 16:39

benjamn added 6 commits January 17, 2019 14:31

Reapply remaining layers in removeOptimistic by rerunning transactions.

531aa32

More comments and a few minor simplifications.

801a490

Remove redundant this.transformDocument calls from InMemoryCache.

d92fc1c

Mention PR #4319 in CHANGELOG.md.

4a87022

benjamn force-pushed the avoid-copying-entire-cache-for-optimistic-reads branch from 2658796 to 4a87022 Compare January 17, 2019 19:32

hwillson approved these changes Jan 17, 2019

View reviewed changes

benjamn merged commit c2295e6 into master Jan 17, 2019

benjamn deleted the avoid-copying-entire-cache-for-optimistic-reads branch January 17, 2019 19:57

benjamn mentioned this pull request Aug 16, 2019

Implement EntityCache as replacement for DepTrackingCache and ObjectCache. #5197

Merged

hwillson mentioned this pull request Jan 10, 2020

Make InMemoryCache#evict remove data from all EntityStore layers. #5773

Merged

github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid copying entire cache on each optimistic read. #4319

Avoid copying entire cache on each optimistic read. #4319

benjamn commented Jan 16, 2019 •

edited

Loading

benjamn Jan 16, 2019

benjamn Jan 16, 2019

benjamn Jan 16, 2019

benjamn Jan 16, 2019

benjamn Jan 16, 2019

benjamn Jan 17, 2019

benjamn Jan 17, 2019

mxstbr commented Jan 17, 2019

hwillson left a comment

benjamn commented Jan 17, 2019

Avoid copying entire cache on each optimistic read. #4319

Avoid copying entire cache on each optimistic read. #4319

Conversation

benjamn commented Jan 16, 2019 • edited Loading

benjamn Jan 16, 2019

Choose a reason for hiding this comment

benjamn Jan 16, 2019

Choose a reason for hiding this comment

benjamn Jan 16, 2019

Choose a reason for hiding this comment

benjamn Jan 16, 2019

Choose a reason for hiding this comment

benjamn Jan 16, 2019

Choose a reason for hiding this comment

benjamn Jan 17, 2019

Choose a reason for hiding this comment

benjamn Jan 17, 2019

Choose a reason for hiding this comment

mxstbr commented Jan 17, 2019

hwillson left a comment

Choose a reason for hiding this comment

benjamn commented Jan 17, 2019

benjamn commented Jan 16, 2019 •

edited

Loading