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

[META] Upgrading JSC #19737

Open
kelset opened this Issue Jun 15, 2018 · 30 comments

Comments

Projects
None yet
@kelset
Collaborator

kelset commented Jun 15, 2018

The purpose of this issue is to create a "single point of truth" regarding the efforts related to upgrading the JSC used by the react-native repo/project, in order to coordinate and recap all the efforts connected to this subject.

Foreword

The JavaScriptCore (JSC), basically, allows Android developers to use JavaScript natively in their apps - you can read here for more details about how react native uses engines.

Currently, react-native bundles in the Android app android-jsc, which is outdated and relies on old native tooling (ex. NDK v10).

Why

Upgrading the JSC would improve greatly performances for react-native apps running on Android, and allow support for x64 builds on Android apps.

Which links back this to the main "Roadmap of future Android(s)" issue #19297 - quote:

In anticipation of future Android devices that support 64-bit code only, the Play Console will require that new apps and app updates with native libraries provide 64-bit versions in addition to their 32-bit versions. This can be within a single APK or as one of the multiple APKs published.

Related

PRs

Currently there are two approaches possible, keep relying on the android-jsc bundled but changing some other configs:

  • [WIP] Android 64-bit #17640

  • add arm64 and x86_64 support #18754

Which seems to not be the way to go, given that internally FB doesn't use it.

The alternative is to move to the JSC fork maintained by some important players in the core & community:

  • Build 64bit binaries for Android #19536

Issues

  • React Native for Android is incompatible with 3rd-party 64-bit libraries #2814

  • [...Array(5).keys()] not working in Android #18426

  • React Native apps fail to build since NDK update to r17 (due to removed support for ARMv5/armeabi) #19321

  • potentially (as @kmagiera wrote in core), this unfortunately would bring some significant footprint penalty: JSC built with intl enabled takes roughly 5M more than without it. This needs to be multiplied by the number of CPU archs included (e.g. x86, armv7, arm64, x86_64), which leads to an "Hello World" base app to ~40MBs - unless split apk is enabled. But this it potentially "automatically" fixed by Google via their new Android App Bundle feature.


Again, this issue is for discussing and keeping everyone updated with decisions around this subject (based on discussions happened with the core team and open issues in the repo). Please avoid going OT.

@newyankeecodeshop

This comment has been minimized.

Contributor

newyankeecodeshop commented Jun 15, 2018

Great job creating a centralized issue for this @kelset .
The question of whether to include Intl support is a good one. The JavaScriptCore on iOS 10.0+ has Intl support, so it would be good for consistency to have the Android one include it as well. However I can see how the increase in the binary size makes that problematic.

@TheSavior TheSavior referenced this issue Jun 15, 2018

Closed

string `startsWith` produces false negatives #19699

5 of 5 tasks complete
@LegNeato

This comment has been minimized.

LegNeato commented Jun 15, 2018

I believe you are mistaken...the "FB doesn't use it internally" appears to be referring to the updated SoftwareMansion JSC. That is, FB doesn't want to push an updated JSC to open source that they don't use internally, and the one they use internally is android-jsc. At least, that's how I read it.

Re: newer JavaScriptCore, I would imagine it is basically a non-starter to update JSC past the version shipped by the RN minimum supported iOS version...otherwise one could use JS features on Android that don't work on some subset of iOS devices and cause all sorts of annoying, hard to debug issues.

@LegNeato

This comment has been minimized.

LegNeato commented Jun 15, 2018

FWIW, I have patches out for both the two issues in Buck blocking RN:

For "React Native for Android is incompatible with 3rd-party 64-bit libraries": facebook/buck#1889
For "React Native apps fail to build since NDK update to r17 ": facebook/buck#1894

@gengjiawen

This comment has been minimized.

Collaborator

gengjiawen commented Jun 16, 2018

I prefer #19536. Also with this I think we can reconfig babel to generate modern js. @rafeca

@gengjiawen

This comment has been minimized.

Collaborator

gengjiawen commented Jun 16, 2018

For app bundle size, x86 and x86_64 should not bundled to the apk when you push the apk to market. Because real phone are using arm cpu.

@hey99xx

This comment has been minimized.

hey99xx commented Jun 16, 2018

@gengjiawen You can find X86 devices out in the market but they're so much rarer, especially in modern Android versions. It's mostly ignorable, my company only uses it for emulator builds.

@gengjiawen

This comment has been minimized.

Collaborator

gengjiawen commented Jun 16, 2018

Yes, ignorable is the precise desription.

@hramos

This comment has been minimized.

Contributor

hramos commented Jun 16, 2018

@LegNeato we're not using either JSC at Facebook, therefore there's no reason to force OSS to use android-jsc and we're happy to help unblock folks here.

@LegNeato

This comment has been minimized.

LegNeato commented Jun 16, 2018

Good to know...but, it would be weird if the JSC of react native was newer than the minimum iOS version's JSC, right? Then you would have Android and modern iOS working, and older iOS devices degrading in quality and compatibility. But perhaps that's ok?

@hey99xx

This comment has been minimized.

hey99xx commented Jun 16, 2018

we're not using either JSC at Facebook

Wait, what are you guys using as the alternative then?

@hramos

This comment has been minimized.

Contributor

hramos commented Jun 16, 2018

@hey99xx I can't answer that just yet, but hopefully soon we can share more information. Let's keep this issue scoped to what needs to be done to allow open source to use an arbitrary JSC.

@vovkasm

This comment has been minimized.

Contributor

vovkasm commented Jun 16, 2018

@LegNeato

Then you would have Android and modern iOS working, and older iOS devices degrading in quality and compatibility.

It is more or less already the case. Many devs start coding for ios and later builds for android. So short example: we started to use mobx 5, it requires native Proxy support. All worked good until we run our app on ios 9 (iPhone 4s) and bum!... we just discovered that ios 9 JSC too old for Proxy support and this can't be polifilled. For Android we know that we can bundle our own fresh enough JSC. But for ios, we can't find ready to use solution.
So we already have modern iOS working, and broken old devices (that can't run iOS > 9).

(PS. Yes I seen JSCWrapper code and it seems that FB can use custom JSC on iOS. But building JSC itself for iOS seems like hard work for most devs.)

@vovkasm

This comment has been minimized.

Contributor

vovkasm commented Jun 17, 2018

Some thoughts about possibility to have one JS engine for build on any platform. If this possible it will be big benefit for manage package process.
Just idea (sorry if it looks like offtopic), currently we have only two JS engines which have enough features and can be built for Android/iOS:

  • JSC - has version divergence even on iOS where it is system framework, and building one version for any supported platform looks problematically.
  • ChakraCore - also don't ready for iOS in upstream repo, but see https://github.com/janeasystems/nodejs-mobile - they have working build system for iOS and Android.
@kelset

This comment has been minimized.

Collaborator

kelset commented Jun 18, 2018

@dulmandakh

This comment has been minimized.

Collaborator

dulmandakh commented Jun 28, 2018

@vovkasm

This comment has been minimized.

Contributor

vovkasm commented Jun 28, 2018

@dulmandakh Do you really built it for android and ios with this script? )

@dulmandakh

This comment has been minimized.

Collaborator

dulmandakh commented Jun 28, 2018

@vovkasm nope, but from the repo history I thought that the script is actively maintained

@vovkasm

This comment has been minimized.

Contributor

vovkasm commented Jun 28, 2018

Ok, seems it contains some condition logic for ios... will try it soon... (I currently evaluate the possibility to build ios version of JSC to allow use of Proxy api in our applications which required by latest mobx versions)

@gengjiawen

This comment has been minimized.

Collaborator

gengjiawen commented Jun 29, 2018

@dulmandakh I have built android-jsc and jsc-android-buildscripts. Both require lots of work for android specific. And you also need to build an icu. Both take very long to build.

@kelset kelset referenced this issue Jul 2, 2018

Open

[META] Roadmap of future Android(s) #19297

5 of 13 tasks complete
@hey99xx

This comment has been minimized.

hey99xx commented Jul 5, 2018

jsc-android-buildscripts recently updated their readme regarding minSdkVersion being 21 ( react-community/jsc-android-buildscripts@fd68310 ).

Will this have an impact in future of RN, should we expect the same in this repo too? Will whatever Facebook use internally, if ever released, come with the increased minSdkVersion?

New apps can easily accept this new requirement, but for existing brownfield apps with only some pages written in RN, I fear this might be a obstacle of updating JSC versions :(

@fungilation

This comment has been minimized.

fungilation commented Jul 7, 2018

On binary size. I'll say that should not be a primary concern in stopping 64 bit adoption, as Google is going to require it in 2019. Different methods of binary size reduction by splitting or otherwise should be integrated or documented for users as side issues. I for one am willing to take the binary size penalty, given compatibility and performance benefits for 64 bit.

@dongyuwei

This comment has been minimized.

dongyuwei commented Jul 10, 2018

babel-preset-react-native has 27 plugins, after upgrade JSC to 224109.0.0 , in my tests, only 6 plugins/transformers is needed. The latest JSC has better native support for ES2015+ language features(eg: async-await, Proxy). So my suggestion is if JSC is upgraded, we should think about how to customize the babel-preset-react-native. Not only for better performance, debugging experience will be much better(Currently, the async-await use regenerator runtime, the call-stack is very bad for debugging).

@newyankeecodeshop

This comment has been minimized.

Contributor

newyankeecodeshop commented Jul 10, 2018

That's a good point @dongyuwei . Once the RFC process is up for React Native, I'm going to propose a change to babel-preset-react-native to add parameters that define the minimum supported iOS and Android JSC version of the app. That way, the babel plugins can be tailored to the specific runtime support of the JSC. It could make use of babel's own env preset (https://babeljs.io/docs/en/babel-preset-env).
Here's an example of the idea, describing the app with a minimum iOS 10.3 version and Android using the latest JSC.

{
  "presets": [
    ["react-native", {
      "targets": {
        "ios": "10.3",
        "android": "224109.0.0"
      }
    }]
  ]
}
@paramaggarwal

This comment has been minimized.

Contributor

paramaggarwal commented Jul 21, 2018

Long term, the plan should be to start using default V8 bundled within Android when using React Native. When RN Android was launched a year back, it supported Android versions down to API19 because of which it made sense to bundle a custom JSC binary into each and every RN app that is shipped to Android today. As you can imagine, this is not ideal.

With compilers like Babel and the babel-preset-env it should be possible to selectively compile JS given the feature set of the target JSC vs. V8. If we are anyways going to put in effort towards 64-bit support, I would believe it worthwhile to focus directly on V8 support rather than simply upgrading bundled JSC on Android.

@hushicai

This comment has been minimized.

hushicai commented Jul 28, 2018

If upgraded to Latest jsc,and if we use Proxy,how to polyfill it in lower IOS?

@dulmandakh

This comment has been minimized.

Collaborator

dulmandakh commented Jul 28, 2018

I was able to compile RN with latest NDK release (17b), and created a PR for it (#20357). I can compile and run RNTester app without any issues.

I think it might be better to bump NDK and use it to build JSC with it, because next NDK release will drop GCC and GNU STL, making CLANG and libc++ only supported option.

@newyankeecodeshop

This comment has been minimized.

Contributor

newyankeecodeshop commented Jul 28, 2018

@paramaggarwal I don't think it will be possible for RN to use the default V8 bundled with Android. According to the NDK documentation, V8 is not available as an API to use from C/C++ code.
https://developer.android.com/ndk/guides/stable_apis

I think it's a similar situation to the ICU library, which is also in Android but not available to be used. That's why the JSC build needs to include ICU as well, inflating the size of the app's binary.

Using JavaScriptCore also has the benefit of aligning the runtime with that of iOS, as long we target JSC versions that are closing aligned with a production iOS implementation.

@warpech

This comment has been minimized.

warpech commented Nov 14, 2018

Any updates on this? Thanks!

@gengjiawen

This comment has been minimized.

Collaborator

gengjiawen commented Nov 14, 2018

maybe update jsc building with clang first, then update to new version. Also Microsoft is working on v8 for android with jsi launched.

@dvicory

This comment has been minimized.

dvicory commented Nov 14, 2018

@gengjiawen do you have a link to more information about Microsoft's effort for V8 with JSI? Can't find anything out there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment