Android React Native 64bit support #2366
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hey - looks like you forgot to add a T:* label - could you please add one? |
|
Have you tried running it on an actual device? Then ran it on my OnePlus. It crashes on startup with: |
kneth
reviewed
May 10, 2019
|
@nhachicha @cmelchior We have some ifdefs in https://github.com/realm/realm-js/blob/master/src/android/jsc_override.cpp which can explain the crash. |
bmunkholm
reviewed
May 10, 2019
|
Wow! Can't wait for this to be merged. |
kneth
suggested changes
May 10, 2019
There was a problem hiding this comment.
We need to go through https://github.com/realm/realm-js/blob/master/src/android/jsc_override.cpp and see if any changes are required.
|
I'm a bit surprised that a class as magical as https://github.com/realm/realm-js/blob/master/src/android/jsc_override.cpp doesn't have more comments, it is completely opaque what is happening in there 🤔 |
|
That function patches the beginning of The change needed there is we need an arm64 version of the injected code for the hook. |
|
@tgoyne Thanks for clarifying. |
Do you know how to fix it in arm64? |
nhachicha
force-pushed
the
nh/react_native_64
branch
from
7106ec5 to
0f7c8b0
Compare
Co-Authored-By: Brian Munkholm <bmunkholm@users.noreply.github.com>
|
@tgoyne @kneth I updated the assembly for AARCH64 & also added some documentation that should help future maintainer https://github.com/realm/realm-js/pull/2366/files#diff-e61c77a661cd0add924e9a8bf52700bdR31 For the future, we should consider migrating into the new bridge (JSI) which allows invoking synchronous calls & also abstract the underlying VM used. FB can switch to V8 or another Javascript engine & we shouldn't rely upon a low-level internal dependency (JavaScriptCore) for our implementation. The assembly was tested on x86, x86_64, arm-v7 & arm-v8 (64bit) |
|
Switching to JSI is probably going to be the way to go long-term, but as of React Native 0.58 it was going to be all downside: it'd require adding a JSI version of all of the JSC/v8 specific code, it'd add a bit of overhead, and it wouldn't be enough to let us create realm-js as a normal RN module without mucking about with implementation details. If/when that third part changes then the downsides of JSI will be massively outweighed by having something that actually keeps working with new versions of RN. |
tgoyne
approved these changes
May 20, 2019
Co-Authored-By: Thomas Goyne <tgoyne@gmail.com>
kneth
approved these changes
May 21, 2019
|
Hi, @nhachicha, I tested on 2.28 it work for simulator and real devices, but when i build on samsung s6, note 8. It's crashed. |
|
It looks like the adding Realm to the global context is failing. We currently don't have access to the two specific devices. You mention "tested on [...] real devices" - which ones? |
|
we tested working on nokia x7, nokia 7 plus, Xiaomi Redmi Note 5A Prime, but it's crash on Samsung s6, samsung note 8 and s8 |
|
@duynvt-ibl Thanks for the update. Please create a new issue including the stack trace and info on the devices. |
|
@duynvt-ibl I haven't tested it yet, but when browsing through react-native issues I found this: facebook/react-native#24261. It seems like might be an issue with react-native 0.59.x (just my guess though). |
|
arm64-v8 dont work, tested on Samsung S7 |
|
As @kneth mentioned, please open a new issue as discussing on a merged PR is hardly efficient for us to keep track of what's working and what is not. |
|
@jerson Can you open an issue? |
|
@nirinchev @kneth @nhachicha Here's the issue: #2391 |
|
@janczizikow I'm using Kudo's JSC version for fixing the RN issue, but still getting crash, see detail in #2391 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tested on x86_64 emulator using
"react": "16.8.3", "react-native": "0.59.8"