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

[WIP] Android 64-bit #17640

Closed
wants to merge 5 commits into from
Closed

[WIP] Android 64-bit #17640

wants to merge 5 commits into from

Conversation

govi
Copy link

@govi govi commented Jan 17, 2018

Motivation

Scratching my own itch #2814 to make sure react-native works on 64-bit Android devices. Using abifilters to restrict it to bundle only 32-bit .so files drops performance heavily on 64-bit devices.

I have attempted to tackle the issue. Much of it 'just' worked, but without being able to verify them with the full suite of tests, I will leave this as Work in Progress. I pored over https://facebook.github.io/react-native/docs/testing.html but it wasn't a lot of help. I'll leave the details of the errors from the PR description for now.

Headline changes

  1. Enabled the 64bit arm eabi and x86_64
  2. Changed the CXX11 flags for folly & cxxreact
  3. Upgraded to use the new soloader(0.3.0) that no longer restricts the so files to be copied.
  4. Upgraded double-conversion to 1.1.6, for 64-bit support.
  5. Updated generator templates

Test Plan

  1. Used gradle to make the .aar file and verified that it contains the .so files for all architectures ("armeabi-v7a", "x86", "x86_64", "arm64-v8a").
  2. Used that aar as a gradle dependency to build my react native app and verified it on a Nexus 6p.
  3. SoLoader copies the right files, and React Native screens work as they should.

Automated Tests

It's not ideal. Probably because this is the first time I have attempted to run these.

Android Unit Tests pass but having massive trouble getting the integration tests going. Its the same both on master and on my branch. Would like some help with it.

Gradle tests fail on master (at least on my setup) for all yoga tests as its unable to load yoga.so. We have the same failures on this branch as well.

There also an oddity with robolectric, where the jars are not getting copied to their correct locations. Had to do it manually.

Buck tests for iOS fail on snapshots for Scroll View. Looks like the scroll view test page now has a new component that is not there in the snapshots. But that component has loading animation, and it is not going to be possible to have a 100% match (it's probable, but the probability is low). Have the same issue on master.

Other Considerations

Finally, we also need the 64-bit enabled JSC. There are two options,

  1. Is to resurrect this Add x86_64 support facebookarchive/android-jsc#19. The tar location is gone now, so the script has to be modified to checkout the repo at the correct revision. Its a known version and since there are no "tests", seems like a safer option for this PR. Which is what I had to do locally and pointed it to our internal mvn that has the 64-bit enabled JSC.

  2. The other option is to https://github.com/SoftwareMansion/jsc-android-buildscripts. Seems to be what expo are using/planning to use. Of course, it needs more testing. Last I have seen is expo rolling it back

Related PRs

Nothing at the moment.

Release Notes

[ANDROID] [BUGFIX] [ReactAndroid] - Enabled 64-bit support

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. cla signed labels Jan 17, 2018
@pull-bot
Copy link

pull-bot commented Jan 17, 2018

Warnings
⚠️

👷 Work In Progress - This PR appears to be a work in progress, and may not be ready to be merged yet.

Generated by 🚫 dangerJS

@gengjiawen
Copy link
Contributor

Why bundle so instead of generate from source?

@govi
Copy link
Author

govi commented Feb 8, 2018

@gengjiawen that was a mistake.. i have since cleaned it up. Would really like some help to complete this PR.

@gengjiawen
Copy link
Contributor

I think first we need to get facebookarchive/android-jsc#19 merged.

@facebook-github-bot
Copy link
Contributor

@govi I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

@aaronechiu
Copy link
Contributor

@ayc1 @mdvacca @axe-fb ex-FB Android Messenger people at Uber are asking for this.

@govi
Copy link
Author

govi commented Feb 27, 2018

@gengjiawen yup.. but we prolly have to update the PR. The tar ball location is 404. I had to checkout their SVN repo and up to the revision. I think I'll prolly get to it. If nothing, it will make some of these tests pass.

@sdwilsh sdwilsh removed the cla signed label Mar 1, 2018
@hramos hramos added the Android label Mar 13, 2018
@react-native-bot react-native-bot added Bug Fix 🐛 Ran Commands One of our bots successfully processed a command. labels Mar 16, 2018
@react-native-bot react-native-bot added Platform: Android Android applications. Ran Commands One of our bots successfully processed a command. labels Mar 18, 2018
@gengjiawen
Copy link
Contributor

@hramos @janicduplessis Can you guys to push facebookarchive/android-jsc#19 get merged, we have been stuck with old jsc and lack of armeabi-64 support for a long time.

@hovox
Copy link
Contributor

hovox commented Mar 21, 2018

This issue is super important for us. We have started RN integration into our app, and this may hold us back. Please let us know what we need to do to increase the priority of this issue, or if we can help somehow to get issue resolved.

@elan
Copy link

elan commented Apr 4, 2018

Also important to us.

@hramos hramos added this to the 0.58 RC milestone Sep 25, 2018
@hramos hramos removed this from the 0.58 RC milestone Jan 16, 2019
@hey99xx
Copy link

hey99xx commented Jan 24, 2019

Can we close this now? New JSC has been merged into master branch and it's 64-bit architectures are enabled in Application.mk

@gengjiawen gengjiawen closed this Jan 24, 2019
@hramos hramos added the Resolution: Fixed A PR that fixes this issue has been merged. label Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Platform: Android Android applications. Ran Commands One of our bots successfully processed a command. Resolution: Fixed A PR that fixes this issue has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.