Memory leak with optimistic response

[mpoeter]

When using mutations with an optimisticResponse we found a significant increase in memory usage.

Intended outcome:
No or only small increase in memory usage.

Actual outcome:
Huge increase in memory usage.

How to reproduce the issue:
Given an application with multiple watched queries:

• take a memory snapshot
• perform a mutation with an optimistic response (it is sufficient to update a single field of some object)
• take another memory snapshot

Alternatively use the memory profiling function of Chrome instead of taking separate snapshots - this has the advantage that we get callstacks for the allocations.

Versions

• apollo-cache-inmemory: 1.3.8
• apollo-client: 2.4.5
• apollo-link: 1.2.3
• apollo-link-context: 1.0.9
• apollo-link-error: 1.1.1
• apollo-link-http: 1.5.5
• react-apollo: 2.2.4

We suspect that this was introduced in #3394. In particular I suspect the commit 45c4169 to be the main cause.

If InMemoryCache.optimistic contains some entry, read and diff create temporary stores with that optimistic response:

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

Lines 137 to 139 in 48a224d

	const store = (query.optimistic && this.optimistic.length)
	? defaultNormalizedCacheFactory(this.extract(true))
	: this.data;

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

Lines 167 to 169 in 48a224d

	const store = (query.optimistic && this.optimistic.length)
	? defaultNormalizedCacheFactory(this.extract(true))
	: this.data;

These temporary stores are than used in the calls to readQueryFromStore/ diffQueryAgainstStore and should later get garbage collected. But unfortunately these store objects are internally used as key in calls to cacheKeyRoot.lookup:

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

Lines 124 to 130 in 48a224d

	return reader.cacheKeyRoot.lookup(
	reader.keyMaker.forQuery(query).lookupQuery(query),
	contextValue.store,
	fragmentMatcher,
	JSON.stringify(variableValues),
	rootValue.id,
	);

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

Lines 144 to 150 in 48a224d

	return reader.cacheKeyRoot.lookup(
	reader.keyMaker.forQuery(execContext.query).lookupSelectionSet(selectionSet),
	execContext.contextValue.store,
	execContext.fragmentMatcher,
	JSON.stringify(execContext.variableValues),
	rootValue.id,
	);

45c4169 replaced the defaultMakeCacheKey function from optimism (which internally uses a WeakMap) with the newly introduced CacheKeyNode concept, causing these temporary stores to end up as key in one of the (strong) Maps, effectively leaking them.

The more watched queries you have, the bigger the problem because maybeBroadcastWatch calls diff for every single watcher. So we end up creating (and leaking) a temporary store for every single watcher for a single optimistic response (apart from the leakage, there is certainly some room for optimization here).

Not sure if this is the only problem or if there are other leaks as well. We saw a single mutation (that updates a single field) to cause a memory increase of 40-80MBs - not sure if such a huge increase can be caused by this problem alone.

@benjamn Since you were the main author of #3394, could you take a look at this? Let me know if you need more information!

[eltonio450]

Hello,

Does someone has a workaround so far ? Downgrading is a bit painful because of #4125 :( (version matching...).

Thank you :)

[SnidelyWhiplash]

Yeah this bit me too I think. Had to temporarily comment out all my optimisticResponses... would love an update here plz :)

[brianlovin]

Currently experiencing this on Spectrum (https://github.com/withspectrum/spectrum) - this is particularly painful for a chat app that uses optimistic responses for each message sent. I can reliably crash my browser with OOM error, and the trace is coming from apollo-cache-inmemory - removing the optimistic response removes the problem, but degrades the user experience.

[barbalex]

Even with optimistic response turned off (before which the app crashed after about 10 updates after reaching over 1GB of memory usage) it seems very weird how memory usage of my app changes over time:

• After starting up memory usage is a few dozen MB
• After very basic updates (checking a checkbox) it increases by a number between 0 and 100 MB
• Often the increase rises with the number of following updates: during the first few only a little, then more and more
• Then suddenly after an update it drops down back to a few dozen MB
• By this time the memory usage has risen to about 600 or 700MB

This gives me a very bad feeling.

[dajay0]

We have been hit by this issue too. Would love to see a fix for it. Any updates / workarounds would be highly appreciated.

Also is there anything we can do to help?

[benjamn]

Can folks try running

npm install apollo-cache-inmemory@1.3.12-beta.0

per #4251?

[barbalex]

I did not see a difference. But my project still has optimisticResponce turned off, so I guess that was to be expected.

[benjamn]

That's right, this change would only prevent memory leaks stemming from optimistic reads. If you're feeling adventurous, you could try reenabling optimistic responses to see whether the additional memory usage is less than before?

[mpoeter]

@barbalex The fact the your memory drops again to a few MB shows that you don't have a real leak, otherwise the GC would not be able to collect those 600-700MB. The question only is why the GC does not kick in earlier or why collection of those objects is not possible at that time. I recommend to investigate this with the Chrome Allocation Profiler.

@benjamn Thanks, I will give it a try tomorrow. Quick question: wouldn't it be sufficient to use the UniversalWeakMap from immutable-tuple for the children in CacheKeyNode?

[barbalex]

O.k., tested with optimistic response. Here comes the test of clicking an option repeatedly and watching memory usage:

• Without optimistic response: Memory rises, beginning at 70 MB. Stopps rising and drops a little at 400 MB after about 20 clicks. After some inactivity falls back to 70MB.
• With optimistic response, without this update: Memory keeps rising, beginning at 250 MB. App crashing at 2GB after 80 clicks.
• With optimistic response, with this update: Memory keeps rising, beginning at 70 MB. Does not seem to rise over 350 MB. After some inactivity falls back to 70MB.

The difference between using optimistic response and not may be accidental, as the numbers vary. But the update definitely seems to solve the crashing when using optimistic response in my app.

[brianlovin]

Tested this locally for Spectrum:

• Without this update: started at 60mb, initiated 20 optimistic updates, ended at 1gb memory. Chart is continuous line up in heap memory, no apparent gc.
• With this update: started at 60mb, initiated 20 optimistic updates, peaked at 304mb memory with very clear gc resets.

[benjamn]

@barbalex Since result caching is just an optimization, and it sounds like it may be contributing to memory problems that significantly worsen performance for you, I've pushed another change (in apollo-cache-inmemory@1.3.12-beta.1) that allows disabling result caching for a given InMemoryCache instance: cfc8fdf.

I imagine the { resultCaching: false } option will be useful for diagnosing result caching problems in the future, even if it's not something we recommend as a permanent solution.

[benjamn]

Ok, apollo-cache-inmemory@1.3.12 has just been published to npm. Closing this issue now, perhaps somewhat optimistically, because I believe the memory leaks that were specifically related to optimistic responses have been addressed. Please open new issues for any other memory problems. Thanks!

[brianlovin]

Thank you @benjamn - really appreciate the fix here!

[benjamn]

Quick question: wouldn't it be sufficient to use the UniversalWeakMap from immutable-tuple for the children in CacheKeyNode?

@mpoeter Yes, but the choice was deliberate! In 45c4169 we switched from defaultMakeCacheKey (which uses immutable-tuple) to the CacheKeyNode abstraction, specifically to avoid using WeakMap, which apparently isn't fully supported in React Native Android, like so many other long-standard ECMAScript features. Also, the local TypeScript implementation was simpler and easier to consume in other TypeScript modules.

[jeremycolin]

@benjamn I was using directly InMemoryCache for a project, I was writing one big query and reading pieces using readFragments. With some very little load, my instance was quickly running out of memory because of memory spikes. With latest version, it does not.

Thanks for the quick fix on this, very very much appreciated from graphQL lovers out there

[mpoeter]

@benjamn Thanks for the clarification. I saw the commit to remove the WeakMap, but it wasn't clear to me why this was necessary.

BTW: our tests also confirm that the memory issue is now fixed! 👍

[benjamn]

As I suspected when we were originally working on this issue, optimistic cache reads could be implemented much more efficiently. This PR should improve the speed and memory consumption of optimistic reads and writes considerably: #4319

[mpoeter]

Sounds great!