-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Remove custom Hermes config for Android #31900
Conversation
|
Base commit: f3e8ea9 |
Base commit: f3e8ea9 |
Thanks for pulling this off! Let me cite my comments at facebook/hermes#511 (comment) for whom coming through here:
|
Okay, with that approach in mind I think we can break down the work into three parts:
This PR has been updated to handle 1. I can submit a follow-up PR to handle 2, but I'll need some help with 3 since I'm not familiar with how to compile and test a custom Hermes build on iOS and Android. If anybody has any pointers that would be great. Alternately, would be happy if somebody else offers to handle 3 :) |
86773fc
to
f8ce862
Compare
Rebased and tests are all looking clear! I think this is ready for review/merge. Adding some folks:
Regarding the changelog – I omitted one because this is because a refactor, and should be a no-op on its own. Can add one if people think it's necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Right now react-native always passes in a config when it calls hermes::makeHermesRuntime. We should update it so it doesn't pass in a config unless there is a custom one being specified. That way, Hermes will be able to determine a default config on its own.
I see where you were coming from, but I don't see this to be particularly useful because:
- the
runtimeConfig
is already an optional parameter soHermesExecutorFactory
is effectively overloaded as what you changed to. - the
runtimeConfig
is already default constructed for the case of 2-params overloads so it's effectively the same as deferring it to thehermes.h
. - the use of
folly::Optional
doesn't make anything safer.
- While iOS does not bother specifying a custom config currently, Android does. If we want both platforms to defer to Hermes for the default config, we should remove the custom Android config.
How do you feel about we repurpose this PR to just remove the custom Android config? The Hermes default config change should be committed soon.
EDIT: facebook/hermes@5f2b47d is commited.
Ah, I see what you mean... hermes::makeHermesRuntime(::hermes::vm::RuntimeConfig());
hermes::makeHermesRuntime(); But I guess that's not the case. If there's no plan to differentiate these, then I can do as you suggest and make this PR just about reverting the custom Android config. |
Summary: The default max heap size GC config of Hermes, 512MiB, is too small to be the default config for React Native apps from the nowadays standard. It casued memory issues on Android (#295) and was mediated by manually [overriding this number to 1024MiB from the RN Android code path](facebook/react-native@63d20d3), and a similar OOM issue was reported on iOS recently (#551). As Hermes being adopted by more platforms (Windows, macOS), it make no sense to override this number again and again in each platforms. Rather, the default max heap should be changed once and for all, The RN side of change is tracked at facebook/react-native#31900. We choose 3GiB which is "unlimited" in practice, making Hermes closer to JSC which doesn't have such limit as well. Reviewed By: neildhar Differential Revision: D30181817 fbshipit-source-id: 54a442e43b7884a9f5d7ed099cb9b9866ee4f109
@Ashoat I've updated the Hermes default GCConfig a while ago. Let me know when you can get back to this. Ideally we want this making into the upcoming RN 0.66 cut. |
Sorry this has been taking so long! My life has been a bit crazy recently and I haven't been able to find the time. Looking at this now :) |
Okay, the PR has been updated! |
@yungsters has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
@yungsters merged this pull request in a40f973. |
Heads up, @Huxpro flagged concerns with this change to me. I just submitted a revert, and he's planning to follow up with more details and proposal for next steps. |
This pull request has been reverted by 455433f. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @yungsters and @Ashoat !
Regarding the decision of a revert...In a nutshell, I felt like you went too far on removing stuffs that we actually would like to keep.
You removed some critical GCConfig customization (as I highlighted below) which will regress TTI significantly. I'm sorry if my previous wording of "remove the React-Android specific GCConfig in React Native instead" was a little misleading. What I really meant is reverting just the 63d20d3 as you pointed out as "On Android it appears to be overriden to 1024 MiB".
In addition, even if we want to remove all the customized GCConfig, there are way more RuntimeConfig items other than GCConfig so we probably don't want to remove all the RuntimeConfig threading and JNI scaffolding. I understand where you are coming from because GC heapSize happened to be the only POJO field here and rendered them like dead code. However:
- It's a moving part that we'll constantly modify whenever we felt the need to expose some Hermes config (e.g. We was using it for Proxy. Who knows what's next?).
- A unknown fun fact is that we actually have an internal mirror of this file at FB that exposed way more Hermes config to Java and we'd love to keep the OSS and internal interface consistent.
Due to the tight timeline we have for RN 0.66 (which is already cut and is waiting Hermes for RC2 release), I have to revert it myself at a8ac5ea to make sure things can be cherry-picked on time.
Big shout out to all the investigation and effort you've made and helped us resolving facebook/hermes#511 and similar issue once and for all!
// For the next two arguments: avoid GC before TTI by initializing the | ||
// runtime to allocate directly in the old generation, but revert to | ||
// normal operation when we reach the (first) TTI point. | ||
.withAllocInYoung(false) | ||
.withRevertToYGAtTTI(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this is a flip of the default GCConfig that we customized to optimize TTI for all React Native apps but not going to work on any other environments (hence not suitable to be the Hermes default).
No worries – I'm glad to have helped! |
Summary
Right now,
react-native
on Android passes in an explicit Hermes config when initializes Hermes. The upcoming version of Hermes shipping with React Native 0.66 will handle this default config itself, so there's no need to override it from Android anymore.Test Plan
I compiled and ran a React Native app using the Android build, making sure to build
ReactAndroid
from source. I confirmed that the config was actually being applied by testing how much memory an application could eat before being killed.