Android memory leak due to JSC

[ben-manes]

related to: facebook/react-native#23259; cc @akshetpandey

Issue Description

A full GC is performed only when Heap#overCriticalMemoryThreshold returns true or if the default is changed to disable the generational GC. Otherwise only the young generation is collected. Unfortunately, overCriticalMemoryThreshold is only implemented for iOS and always returns false for other platforms. This means that any objects promoted from young to old are not GC'd and the app will eventually crash due to an out of memory error.

Please note that running RN through the Chrome debugger does not use JSC. This will use Chrome's JS engine and promptly reclaim memory. We confirmed this bug and fix by reducing the maximum memory for JSC, observing the leak, and then healthy behavior when resolved.

Version, config, any additional info

The least invasive change was to force a full collection and we are satisfied with the resulting performance. In our application, we did not experience long pause times that would result in a negative experience. We changed overCriticalMemoryThreshold to return true on non-iOS platforms and believe that is the correct default behavior. A more advanced solution would be to add Android support to calculate if the threshold was crossed.

iff --git a/webkit/Source/JavaScriptCore/heap/Heap.cpp b/webkit/Source/JavaScriptCore/heap/Heap.cpp
index ffeac67d..9dcf4113 100644
--- a/webkit/Source/JavaScriptCore/heap/Heap.cpp
+++ b/webkit/Source/JavaScriptCore/heap/Heap.cpp
@@ -498,7 +498,7 @@ bool Heap::overCriticalMemoryThreshold(MemoryThresholdCallType memoryThresholdCa
     return m_overCriticalMemoryThreshold;
 #else
     UNUSED_PARAM(memoryThresholdCallType);
-    return false;
+    return true;
 #endif
 }

We substitute the android-jsc dependency with our patched version,

// replace the webkit jsc package with react native with our customized only do full 
// garbage collection to avoid the memory leak issue in js heap
configurations.all {
  resolutionStrategy.dependencySubstitution {
    substitute module('org.webkit:android-jsc') with module('com.withvector:android-jsc:r225067')
  }
}

[kmagiera]

Thanks @ben-manes for your report. We discussed this issue internally on contributors discord and with help from some jsc maintainers figured one other approach to fix this. We are hoping to find some time to investigate it and patch soon

[Kudo]

@ben-manes I am trying to deal with the issue. Do you mind to share how you profile the memory usage? Before solving the issue, I would like to write some JS test cases to show the leakage in current JSC. But unfortunately, it seems I cannot force JS allocation marked as old object.

Here are my test case:

class GCTestCase extends React.Component<{}> {
  state = {
    counter: 0,
  };

  componentDidMount() {
    global.buckets = [];
    this.timer = setInterval(() => {
      if (this.state.counter >= 1000 * 60 * 1) {
        global.buckets = [];
        this.setState({counter: 0});
      } else {
        global.buckets.push(new ArrayBuffer(1024));
        this.setState(state => ({counter: state.counter + 1}));
      }
    }, 1);
  }

  componentWillUnmount() {
    if (this.timer != null) {
      clearInterval(this.timer);
      this.timer = null;
    }
  }

  render() {
    return (
      <View
        style={{
          flex: 1,
          justifyContent: 'center',
          alignItems: 'center',
          backgroundColor: 'blue',
        }}>
        <Text style={{fontSize: 32, color: '#ffffff'}}>
          ${this.state.counter}
        </Text>
      </View>
    );
  }
}

And check the memory usage with Android Studio memory profiler or adb shell dumpsys meminfo.

Hopefully if there are some test cases and could prove fixes works.

[ben-manes]

@Kudo,

Thanks for taking a look at this! We used adb to investigate with since Android Studio's profiler didn't work with Android 4.4, iirc. We were able to test on a memory constrained device, which made it easier to use our application and monitor the memory report.

We'll try to get some notes together and review our steps. We did this last year during a scramble, as it blocked a partnership, so I am a little foggy at the moment.

[Kudo]

Thanks @ben-manes,

Do you remember more information about the app, will the app being killed by Android after long run?

JSC's memory model is complicated, AFAIK, JSC will do full GC if eden / old ratio is too small.
Please see https://trac.webkit.org/browser/webkit/releases/WebKitGTK/webkit-2.22.2/Source/JavaScriptCore/heap/Heap.cpp#L2263.
From this, the memory usage may not as good as JSC on iOS, but it should not cause long running memory leakage.

[ben-manes]

We have it failing in our app with logs, and an early failure with yours. We're wrapping up a test case for you. The main observation is that we used 50 * 1024 * 1024 whereas you are using 1024, which will take much longer to exhaust. The larger arrays will be more aggressively promoted and seems to do the trick for us. I hope to post a sample based on your in a few hours.

jsc.profile.tar.gz

[ben-manes]

@Kudo We modified your code to increase the interval from 1ms to 5s and also increased the buffer from 1K to 50MB. So every 5s, allocates 50MB of buffer and after 250MB deallocates everything. We tested this on Android 8 (API 26).

• The default.jsc contains the logs for default jsc that comes with react native 0.58.3.
• The custom.full.gc.jsc uses are patched version (above)

android8.api26.log.tar.gz

android8.api26
├── custom.full.gc.jsc
│   ├── logcat.txt
│   └── memory.usage.log.txt
├── default.jsc
│   ├── logcat.txt
│   └── memory.usage.log.txt
└── diff.txt

[Kudo]

Awesome @ben-manes !

Thank you for the useful information you provided and I did see the leakage from your log.
May I further ask your device model to known which ABI you used and how many RAM you have?
I will try to see if I could reproduce and troubleshoot the leakage with your 5s / 50MB allocation method.

[Kudo]

I've tested on Samsung Galaxy Note 5 (Android 7, arm32, 4GB RAM) without the leakage issue.
adb result range from 300MB - 900MB in total and will recycle memory.

Note that JSC by default will enable concurrent GC only for 64-bit devices.

#if !CPU(X86_64) && !CPU(ARM64)
Options::useConcurrentGC() = false;
#endif 

Not sure the concurrent GC will affect the leakage or not.

[ben-manes]

It was a moto-e armabi-v7a, not sure about the model & ram until I get to the office

[ben-manes]

The attached tests were on a moto e5 plus 2gb.

[Kudo]

@ben-manes,

Thanks for your information.
I was wrong that my Galaxy Note 5 is arm64-v8a as well.
Right now I have no idea why my device cannot reproduce the leakage.
Will turn another way to implement the missing part of bmalloc::availableMemory() for Android.
This enables us to have overCriticalMemoryThreshold() like iOS did.
Hope that will help the leak issue.

[akshetpandey]

I have access to a whole bunch of android devices and will run the tests on them sometime in the next 24 and let you all know what the results are.

[akshetpandey]

@ben-manes : Can you tell me how to create the gc profiles?

[ben-manes]

@akshetpandey I think you want adb shell dumpsys meminfo <package>