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

Unreasonable heap size calling fetchMore, most prevalent with JavaScriptCore environments #5007

Closed
jasongaare opened this issue Jun 25, 2019 · 8 comments
Assignees

Comments

@jasongaare
Copy link

jasongaare commented Jun 25, 2019

Intended outcome:
We've implemented a <Query> component that fetches 50 photos at a time from our database, along with information related to those photos. We expect as we call fetchMore and updateQuery the JS heap would grow only in relation to the size of the response for the next 50 photos

Actual outcome:
As fetchMore gets called on our component, the heap size grows unreasonably, specifically on Safari and in our React Native app (the root issue!) - both of which use JavaScriptCore. Running our code in a V8 JS environment (read: Chrome) the heap grows at a more reasonable rate. (but perhaps still too much?) Here are some screenshots of heap snapshots on each browser, using the same query to fetch 250, 500, then 750 photos via pages of 50.

Photos Query

Screen Shot 2019-06-25 at 1 23 51 PM

Screen Shot 2019-06-25 at 1 37 15 PM

Curiously, when we added an @connection directive things got worse...

Photos Query with @connection

Screen Shot 2019-06-25 at 1 27 40 PM

Screen Shot 2019-06-25 at 1 34 05 PM

This issue is a blocker for our mobile app at the moment, as we are using React Native and are experiencing the drastic heap inflation with the shipped JSC version. We are currently on React Native 55.3, but have also tested on the latest version (which has a newer JSC) but are seeing the same results (aka still exists in new JSC versions, as demonstrated in the latest version of Safari above)

Further, we have also attempted to use the new immutable features in the cache/client as seen here (and on the reproductions below), but it did not improve results as we hoped it might

this.client = new ApolloClient({
  link: concat(authMiddlewareLink, httpLink),
  cache: new InMemoryCache({ freezeResults: true }),
  assumeImmutableResults: true
});

How to reproduce the issue:
Here are the reproductions with our production API and actual query we are running:

Photos Query

Photos Query with @connection

Versions

  Browsers:
    Chrome: 75.0.3770.100
    Safari: 12.1
  npmPackages:
    apollo-boost: 0.4.3 => 0.4.3
      -> apollo-cache-inmemory: 1.6.2 => 1.6.2
      -> apollo-client: 2.6.3 => 2.6.3
      -> apollo-link: 1.0.6 => 1.2.12
      -> apollo-link-http: 1.3.1 => 1.5.15
    react-apollo: 2.5.8 => 2.5.8
@jasongaare
Copy link
Author

jasongaare commented Jun 25, 2019

We are fairly certain this is a bug, but if it is expected behavior and a matter of refining/optimizing our query: here's a cross-reference to a Spectrum thread I started on the same topic (with a few other details as well): https://spectrum.chat/apollo/apollo-client/inmemorycache-inflates-drastically-calling-fetchmore-on-a-deeply-nested-query~17b06b40-5c53-4856-93c6-0948e768477f

@benjamn benjamn self-assigned this Jun 27, 2019
@benjamn
Copy link
Member

benjamn commented Jun 28, 2019

I’m still investigating this, but in the meantime could you try disabling result caching (the layer of caching on top of the normalized cache)?

this.client = new ApolloClient({
  link: concat(authMiddlewareLink, httpLink),
  cache: new InMemoryCache({
    freezeResults: true,
    resultCaching: false, // add this option
  }),
  assumeImmutableResults: true
});

This will make repeated reads a bit slower, but should also make the cache use less memory.

@benjamn
Copy link
Member

benjamn commented Jun 28, 2019

@jasongaare I can confirm my suggestion above seems to reduce memory usage in the reproductions you provided. Still working on a longer-term fix.

@jasongaare
Copy link
Author

Thank you for looking into this!

@jasongaare I can confirm my suggestion above seems to reduce memory usage in the reproductions you provided. Still working on a longer-term fix.

I, too, have seen improvements on Safari and the iOS simulator using resultCaching: false.

However, at some point in the last few weeks hunting this down, I had tried using resultCaching: false before, but only while I was testing on an Android device... which didn't seem to have any improvement. At that point I'd kind of ditched the concept since it didn't seem to help much on Android

I've been using adb shell dumpsys meminfo on a small mobile reproduction that mimics the above web reproductions. Here are my latest tests on Android from just this afternoon

resultCaching:   true / false
50 photos     - 104.3 / 100.3 MB    (total memory usage)
250 photos    - 120.5 / 127.1 MB
500  photos   - 141.8 / 153.2 MB
750  photos   - 163.1 / 170.7 MB
1000 photos   - 189.1 / 176.8 MB
1250 photos   - 211.9 / 203.8 MB

A couple things here:

  1. This may be due to the JSC version shipped with RN on Android? I can try using this library to see if there might be improvement.
  2. Those results ^ are from a development build so perhaps a release build might be better? I can run tests on that too.

So if this is a viable solution in the short-term, what are the underlying reproductions of resultCaching? I see this:

// Passing { resultCaching: false } in the InMemoryCache constructor options
// will completely disable dependency tracking, which will improve memory
// usage but worsen the performance of repeated reads.
this.data = this.config.resultCaching
? new DepTrackingCache()
: new ObjectCache();

so presumably it just removes the usage of the optimism library? Or perhaps there's more?

Thanks again for digging into this.

@benjamn
Copy link
Member

benjamn commented Jun 28, 2019

That’s right, when ObjectCache is used instead of DepTrackingCache, the makeCacheKey functions in the StoreReader class don’t return anything, which effectively disables any caching of cache results. When I designed this system in #3394, I made sure this layer of caching was entirely optional, so I am confident you can disable it with no adverse consequences. In desktop browsers, using memory to save time is an easy tradeoff, but I fully acknowledge the constraints are different on mobile.

@jasongaare
Copy link
Author

jasongaare commented Jul 2, 2019

@benjamn here's a couple other things we are noticing, and are curious if it is all related or not.

  • connection directive makes cache LARGER?
    It is quite evident in those screenshots above, but even with resultCaching: false when we add the connection directive, the cache grows faster, which seems counterintuitive based on our understanding of the purpose of the connection directive. It shows up on those web reproductions and on mobile, too. Here are some screenshots from the web with resultCaching: false (loading 250, 500, 750 photos) [and of course the outcomes are MUCH more drastic when resultCaching is true]

Screen Shot 2019-07-02 at 9 35 04 AM

Screen Shot 2019-07-02 at 9 36 45 AM

  • fetchMore gets slower and slower as you load more pages
    When we get more and more data associated with a query, fetchMore takes longer and longer to process the new data. This is noticeable across the board with Apollo, regardless of the resultCaching or usage of the connection directive. Testing outside of Apollo via a simple GraphQL request and storing pages in this.state did not yield the same slow down fetching more pages

Here's some mobile memory usage data to back these up (Android device):

452 MB - 1100 photos with @connection, resultCaching default
469 MB - 1100 photos with @connection, resultCaching false  <- also, fetchMore takes much longer

125 MB - 1100 photos with NO @connection, resultCaching default
163 MB - 1100 photos with NO @connection, resultCaching false

_Again, not sure if these are related or separate issues. If they are separate I can certainly open some other issues to keep this thread on-topic _

@matthargett
Copy link

@jasongaare the android-jsc-buildscripts version of JavaScriptCore is already used in React Native 0.59 and newer. In fact, RN 0.59.10 bumps the JSC dependency to solve some issues reported by users. So, no need to build your own to get a newer one.

Additionally, we are also using apollo-client in a React Native context. We haven't seen this particular problem yet, but reported our own major performance issue in #4991 . Thanks for reporting this detailed issue, I think it will help a lot of people having trouble with Apollo in React Native! 🥇

@hwillson
Copy link
Member

Please try a recent version of @apollo/client and let us know if this issue is still happening. Thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants