-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Large observable arrays do not play well with React Native Android #734
Comments
+1 |
1 similar comment
+1 |
Is the problem still present if you |
|
Is there any reproducible setup? Also curious: |
So far it is reproducible on android devices, as I described earlier. If that is indeed a bug in webkit, maybe some old Chrome/Safari will show the problem, too. Like, when I started investigation and lodash was in the mix, too, its source code mentioned this bug: https://bugs.webkit.org/show_bug.cgi?id=156034 - which I guess didnt made the fix into Android 6.0.1 yet. Though, this particular bug is probably not the source of the problem, some related one might. We can try Chrome from late 2015 or something like that. Another detail: during my investigations, I've realized that when I'm dealing with large arrays (>1000 elements), reading from them is really broken. When I printed array, first 1000 elements were always fine, and all elements 1000+ were always broken. I started to wonder if that is some JIT constant (after running a function 1000 times, it is good candidate for JITing), or MobX related. Especially that it had this hardcoded 1000 initial getters/setters in the observable array. So, I changed 1000 to 998 in MobX source code - lo and behold, elements from 998 started to be broken on read. So, the MobX getters are not working fine, for sure. Surely, the MobX implementation of observable array and its getters looks totally fine. Pretty stressing on JS VM, but still seemingly correct. If the fault is on MobX side, we can fix it quickly, at least ;) |
@Nopik interesting! So at least it seems a combination of what MobX is doing with the JVM. What happens if you increase the default array size to e.g. 1000000? (You can easily test that by directly modifying |
Yeah, I also wondered about it ;) Will try tomorrow. If that is the case, I'd expose it from MobX as some advanced settings API ;) |
I've been playing with this for several hours today on my sample hello world app, collected a number of information bits:
Seeing Anyway, after a number of working with my sample hello world app, it became obvious, that if I increase the initial amount of getters, it helps. So, I left that and went back to my main app. Only to find in horror, that in bigger app the workaround doesnt work. Even if initial MobX code would create large amount of getters, they would still be crashing. That basically left me with no option, but making sure that we're not indexing large arrays by index at all in our application. Actually, severely undermining the confidence on small arrays, too ;( Fortunately we have very few potentially big arrays, and I've been able to easily guard them to use Still, the problem basically means that observable arrays are unreliable on Android 6.x (will try to check other versions) - to the point that whole MobX usage in the app is under question, as MobX is so less useful without observable arrays. For the time being I do not know of any good fix for this. Might as well be unsolvable on Android 6.x. Maybe there is a way to prevent JIT on some functions, or catch all array getters in some other way. Until new information will be found out, calling Any chances we can get PS. I'll also try to find some proper place for Android JS bug reports, whether it is webkit itself or some other components. Any pointers would be welcome, too. |
Added sample app @ https://github.com/IDTLabs/RNCrashTest |
Hey @Nopik thanks for the extensive investigation! Really nasty... Let's devise a plan:
|
v3, yay ;) Congrats! Btw. reading through the changelog reminded me how poorly disposers are documented ;) Now, answering your points in order:
|
As a followup, I've upgraded our app to MobX v3 (which was pretty straightforward, a number of map->ObservableMap and transaction->runInAction renames), so if you want to expose |
@Nopik could you try |
Thanks, I'll check it out and let you know, hopefully tomorrow. |
Did you pushed the branch already? I dont see it on the list |
now I have :)
Op di 17 jan. 2017 om 17:22 schreef Kamil Burzynski <
[email protected]>:
… Did you pushed the branch already? I dont see it on the list
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#734 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABvGhIFhUKlNDdeyXnknuhF1R9JueDMSks5rTOrQgaJpZM4LcBD8>
.
|
I've been playing with
In summary, it still doesnt work. Yet, the fact that global array of descriptors do sometimes allow to avoid crashes is pretty interesting. |
Could you try using `.get()` and `.set()`, instead of the index accessors,
and check how that work out?
Op wo 18 jan. 2017 om 10:13 schreef Kamil Burzynski <
[email protected]>:
… I've been playing with fix-734 branch for a while, and found it to
improve the stability, but not fixing it completely. Here are my results:
- I started with my hello world app, verified that it still crashes on
old branch
- switched to new branch, added code to call reserveArrayBuffer to be
big enough
- crashes stopped
- I dropped the reserveArrayBuffer call, crashes were not there. That
was pretty surprising to me.
- I started to check the diff and apparently storing descriptors to
global array had something to do with it.
- so I switched back to old version, crashes appeared
- I added var all_descriptors = [] before createArrayBufferItem and
assigned descriptor to this array
- crashes stopped
- the moment I stopped assigning descriptors to the global array
crashes were back.
- then I switched to the full app, applied the same stuff, crashes
were still there. No matter if I used fix-734 or patched master
myself, or called reserveArrayBuffer, the getters were still
unreliable.
In summary, it still doesnt work. Yet, the fact that global array of
descriptors do sometimes allow to avoid crashes is pretty interesting.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#734 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABvGhD3PczHiTMGpwSGr5uX6C-6dEZPbks5rTdesgaJpZM4LcBD8>
.
|
Since it seems all JIT related, it might make a difference to move all the logic which is now in the generated getters / setters to the newly created get / set methods on the observable array, and generate really small getters / setters.... |
When I earlier was playing with it, I created get/set methods myself, so the getters/setters were just 1-line invocation of get/set. That changed nothing, sadly. Wasn't playing with your new get/set, but the approach you took was identical to what I've tested before. |
haha ok, let me refactor it first to be not dependent on the property descriptors then :) |
@Nopik would you mind testing |
Not pushed yet? ;) |
Eh no everything pushed, didn't |
Sorry for late response. This version seem to behave identically like the previous one. |
Did using .get / .set instead of array indexes solve the problem/ provide a
valid work around?
Op vr 27 jan. 2017 16:31 schreef Kamil Burzynski <[email protected]>:
… Sorry for late response. This version seem to behave identically like the
previous one.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#734 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABvGhHTWYo98saYHUDRArQLVnIg3QLyrks5rWg2-gaJpZM4LcBD8>
.
|
@Nopik any updates around this one? |
Released the changes for this issue as part of mobx 3.1.9. Feel free to ping if any further follow up is needed. |
I've got my own custom code that was using It seems this is an issue with the JIT compiler on android that react-native uses as it's rather old (2014). So I ended up following the instructions here for the SoftwareMansion/jsc-android-buildscripts library that allows for using a more modern JavaScript engine instead of the one included with react-native and voila, my problems have gone away. I'm guessing it should work for you as well. Hopefully others that run into this issue will also see this post as I spent quite some time first to find the issue in the first place since it doesn't crash immediately at the spot where the array is changed. Anyway, thanks for your efforts here because it pointed me in the right direction. |
@ragamufin I encountered the same issue and your solution works great, thanks |
Just thought I'd mention the following here in case it helps anyone, as this issue seems to be one of the only places on the internet that combines MobX with I don't have large observable arrays, but I do have a few nested ones. I noticed that changing observables frequently by tapping around the screen like a mad man would crash my app within 10-30 seconds, with (usually, not always) a Relevant environment details: {
"jsc-android": "224109.1.0",
"lodash": "4.17.11",
"mobx": "4.5.0",
"mobx-react": "5.3.3",
"react": "16.3.1",
"react-native": "0.55.4"
} Solution: |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions. |
I'm not sure if this is Android bug or MobX, but here it is:
Generally, we've noticed that our RN app has problems handling large arrays - but only on Android devices, and only when not debugging remotely. That points it pretty much to Android JS VM, but, it might be MobX problem as well.
After tons of debugging, I've been able to distill minimal example showing the problem.
Now, in
index.android.js
inmobxtest
class add this:import { observable } from 'mobx';
Now,
react-native run-android
and withadb logcat *:S ReactNativeJS
see the output. Here it goes:and it goes on for a bit, then app usually crashes.
I'm using Samsung Galaxy S7, Android 6.0.1, RN 0.40 & RN 0.39, MobX 2.7.0 and 2.6.5 (our app is RN 0.39 + MobX 2.6.5, reproduced on RN 0.40 + MobX 2.7.0).
If the array is shorter (like the commented line is removed), it usually works well.
We do see the bad behaviour in much, much complex case (loading >1000 contacts from device's address book, then displaying them in ListView). Lots of lodash, etc. But I've been able to untangle it down to this simple equivalent. In our app, we've also seen pretty strange crashes with MobX warnings about attempting to read element like 2081 out of 1531 possible, etc. That was basically on
_.filter( foo, (c)=> ... )
whereasfoo
was observable array. The source seem to be the same, MobX started to severely misbehave.I think there is something wrong with the JS JIT, but that is only a remote shot.
The text was updated successfully, but these errors were encountered: