Memory Usage

[shalomvolchok]

It seems that memory usage, in the example todo app of this repo, grows with every request. After about 5,000 request, on my local computer, the app consistently crashes with process out of memory.

The star wars example from isomorphic-relay doesn't seem to have this issue. After 25,000 requests memory usage was still consistent.

Am I missing something simple here? I know a lot of work was done to make Relay state contextual and it seems like that has all been finished.

I've attached two graphs from logging process.memoryUsage() on each request. The first (steady) graph is from isomorphic-relay and the second graph is from isomorphic-relay-router.

[ntharim]

It's probably related to this issue.

[tomconroy]

@nthtran following on from discussion in that thread – the problem has to do with relay using the container component as a cache key, and since isomorphic-relay-router creates a new instance of QueryAggregator in every request, that cache grows boundlessly.

I thought I had solved this here tomconroy@d03e3a5 – this works, but it was problematic when the server was processing concurrent requests.

@nthtran suggests we cache these QueryAggregator instances, keyed by the router location (?)

[ntharim]

My initial thought is after matching we can get the routes array from renderProps. By using paths from the matched routes as the cache key, we can cache a QueryAggregator instance per route combination. @denvned you understand the internals much better, do you think this would be a good idea?

[KCraw]

@nthtran Won't you run into the same problems as @tomconroy regarding concurrent requests if they happen to be for the same path? Isn't a unique QueryAggregator instance per request a requirement for concurrency?

I think the problem here is with the use of QueryAggregator at all. I believe that Relay.getQueries is expecting a component class/constructor and not an instance of a component, which would make sense. Unfortunately, QueryAggregator implements the required static methods of RelayContainer as instance methods, and passing an instance as the Container works, but for the whole 'inevitably crash your production app through normal use' problem. It seems unlikely to find a scalable solution that involves passing instances.

The star wars example in isomorophic-relay obviously avoids this problem because it is passing a constructor. Not sure how QueryAggregator could be made to work here...

[denvned]

I've just pushed a fix for this issue. I believe the fix is solid, but if it doesn't work for you please tell.

[KCraw]

Looks like an AggregateContainer is cached per unique route, and is only caching the fragment information, so can safely be shared between "concurrent" users requesting the same unique path. The params are still unique per request and handled by QueryAggregator.

That is an extremely clever solution. Nice!

[rodrigopr]

Hi @denvned, thanks for taking a look into this issue and for the project as a whole, it's been super helpful to us.

Testing locally the new version it appears that the queryCache from getRelayQueries.js behaves as expected. But we still see a leak in buildRQL.js queryCache.
The cache is growing unbounded with instances of wrappedQuery (aparently this one)

should we reopen this?

[denvned]

But we still see a leak in buildRQL.js queryCache.
The cache is growing unbounded with instances of wrappedQuery

@rodrigopr It would be strange if it really happens, because queryCache in buildRQL.js is used only by the buildRQL.Query that is called only by getRelayQueries and only when there is a local cache miss. That means the cache of buildRQL.Query can grow only when the cache of getRelayQueries grows. So if there is no leak in getRelayQueries there cannot be a leak in buildRQL.Query.

Still cache of buildRQL.Query can grow big, and this is a problem of react-router-relay in general, but not specifically of isomorphic-relay-router.

[rodrigopr]

Hmm, I'll have to investigate further.
On a quick look seems like getRelayQueries cache isn't growing beyond the number of routes .

Will report back if I find something. But as you said, the fix might be on react-router-relay.
Thanks :)

[denvned]

On a quick look seems like getRelayQueries cache isn't growing beyond the number of routes

Yes, but there might be multiple queries per route, that's why the buildRQL cache might grow a bit faster than the getRelayQueries cache.

[shalomvolchok]

Looks great!! Testing locally with ab, stable after 275,000 requests...

[denvned]

@shalomvolchok Thank you for confirming!