Large observable arrays do not play well with React Native Android

Nopik commented 7 years ago

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.

react-native create mobxtest
cd mobxtest
yarn add mobx

Now, in index.android.js in mobxtest class add this:

@observable foo = [];
constructor( props, context ){
  super( props, context );

  let f = ()=>{
    let ff = [ { a: 10 }, { b: 20 }, { c: 30 } ];
    ff = ff.concat( ff );
    ff = ff.concat( ff );
    ff = ff.concat( ff );
    ff = ff.concat( ff );
    ff = ff.concat( ff );
    ff = ff.concat( ff );
    ff = ff.concat( ff );
    ff = ff.concat( ff );
    ff = ff.concat( ff );
    ff = ff.concat( ff ); // remove this
    this.foo = ff;

    console.log('APEEK1', this.foo[10], ff[ 10 ], this.foo.peek()[10]);
    console.log('APEEK2', this.foo[1000], ff[ 1000 ], this.foo.peek()[1000]);
  };

  setInterval( f, 2000 );       
}

import { observable } from 'mobx';

Now, react-native run-android and with adb logcat *:S ReactNativeJS see the output. Here it goes:

01-05 19:51:49.746 12358 13327 I ReactNativeJS: Running application "mobxtest" with appParams: {"initialProps":{},"rootTag":1}. __DEV__ === true, development-level warning are ON, performance optimizations are OFF
01-05 19:51:51.826 12358 13327 I ReactNativeJS: 'APEEK1', { b: [Getter/Setter] }, { b: [Getter/Setter] }, { b: [Getter/Setter] }
01-05 19:51:51.826 12358 13327 I ReactNativeJS: 'APEEK2', { b: [Getter/Setter] }, { b: [Getter/Setter] }, { b: [Getter/Setter] }
01-05 19:51:53.806 12358 13327 I ReactNativeJS: 'APEEK1', { b: [Getter/Setter] }, { b: [Getter/Setter] }, { b: [Getter/Setter] }
01-05 19:51:53.806 12358 13327 I ReactNativeJS: 'APEEK2', { b: [Getter/Setter] }, { b: [Getter/Setter] }, { b: [Getter/Setter] }
01-05 19:51:55.836 12358 13327 I ReactNativeJS: 'APEEK1', { b: [Getter/Setter] }, { b: [Getter/Setter] }, { b: [Getter/Setter] }
01-05 19:51:55.836 12358 13327 I ReactNativeJS: 'APEEK2', { b: [Getter/Setter] }, { b: [Getter/Setter] }, { b: [Getter/Setter] }
01-05 19:51:57.836 12358 13327 I ReactNativeJS: 'APEEK1', { b: [Getter/Setter] }, { b: [Getter/Setter] }, { b: [Getter/Setter] }
01-05 19:51:57.846 12358 13327 I ReactNativeJS: 'APEEK2', { b: [Getter/Setter] }, { b: [Getter/Setter] }, { b: [Getter/Setter] }
01-05 19:51:59.846 12358 13327 I ReactNativeJS: 'APEEK1', { b: [Getter/Setter] }, { b: [Getter/Setter] }, { b: [Getter/Setter] }
01-05 19:51:59.846 12358 13327 I ReactNativeJS: 'APEEK2', { b: [Getter/Setter] }, { b: [Getter/Setter] }, { b: [Getter/Setter] }
01-05 19:52:01.866 12358 13327 I ReactNativeJS: 'APEEK1', { b: [Getter/Setter] }, { b: [Getter/Setter] }, { b: [Getter/Setter] }
01-05 19:52:01.866 12358 13327 I ReactNativeJS: 'APEEK2', undefined, { b: [Getter/Setter] }, { b: [Getter/Setter] }
01-05 19:52:03.856 12358 13327 I ReactNativeJS: 'APEEK1', { b: [Getter/Setter] }, { b: [Getter/Setter] }, { b: [Getter/Setter] }
01-05 19:52:03.856 12358 13327 I ReactNativeJS: 'APEEK2', [], { b: [Getter/Setter] }, { b: [Getter/Setter] }
01-05 19:52:05.866 12358 13327 I ReactNativeJS: 'APEEK1', { b: [Getter/Setter] }, { b: [Getter/Setter] }, { b: [Getter/Setter] }
01-05 19:52:05.866 12358 13327 I ReactNativeJS: 'APEEK2', [], { b: [Getter/Setter] }, { b: [Getter/Setter] }
01-05 19:52:07.876 12358 13327 I ReactNativeJS: 'APEEK1', { b: [Getter/Setter] }, { b: [Getter/Setter] }, { b: [Getter/Setter] }
01-05 19:52:07.876 12358 13327 I ReactNativeJS: 'APEEK2', [], { b: [Getter/Setter] }, { b: [Getter/Setter] }

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)=> ... ) whereas foo 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.

zevmo commented 7 years ago

+1

stief510 commented 7 years ago

+1

mweststrate commented 7 years ago

Is the problem still present if you slice or peek before concat-ing or passing the array to lodash?

Nopik commented 7 years ago

1) I reproduced problem without lodash even installed 2) when on full app, slice delays the problem significantly

mweststrate commented 7 years ago

Is there any reproducible setup? Also curious: @observable foo = asFlat([]) does that make a difference? But given the irregularity of the output, (sometimes printing undefined or []), I suspect this is a VM bug indeed... :'(

Nopik commented 7 years ago

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 ;)

mweststrate commented 7 years ago

@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 node_modules/mobx/lib/types/observablearray.js directly)? That will allocate more memory by default, but if that works around this bug I think that is a very acceptable trade off.

Nopik commented 7 years ago

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 ;)

Nopik commented 7 years ago

I've been playing with this for several hours today on my sample hello world app, collected a number of information bits:

it seems that increasing reserveArrayBuffer to higher number does work in most contexts - not all, sadly
each getter created by reserveArrayBuffer seem to allocate ~34 bytes, so increasing them by 1million took 34MB of memory on my sample
I tried to prevent JITing of the relevant code, but didnt managed to do so. Some google results mentioned try/catch and/or argument polimorphism as good JIT preventers, but after trying all possible configurations, each of them caused problems
I exported reserveArrayBuffer outside MobX and called it from my root component constructor - and it helped on my sample app. Big array was created 2 seconds later via setInterval - but if I setTimeoute'd reserveArrayBuffer even by 1ms from the constructor (well ahead of that 2s interval), it didnt worked.
I tried to inline some of the functions, whenever possible, but it didnt help. Generally the problem was always that the getter was invoked with really wrong arguments (like some object instead of index)
safariPrototypeSetterInheritanceBug is set to true on my config
sometimes JS would crash. Here is some sample stacktrace:

01-09 10:01:02.264 27214 27256 F libc    : Fatal signal 11 (SIGSEGV), code 1, fault addr 0xbbadbeef in tid 27256 (mqt_js)
01-09 10:01:02.324  4894  4894 F DEBUG   : *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
01-09 10:01:02.324  4894  4894 F DEBUG   : Build fingerprint: 'samsung/hero2ltexx/hero2lte:6.0.1/MMB29K/G935FXXU1BPLB:user/release-keys'
01-09 10:01:02.324  4894  4894 F DEBUG   : Revision: '9'
01-09 10:01:02.324  4894  4894 F DEBUG   : ABI: 'arm'
01-09 10:01:02.324  4894  4894 F DEBUG   : pid: 27214, tid: 27256, name: mqt_js  >>> com.mobxtest <<<
01-09 10:01:02.324  4894  4894 F DEBUG   : signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0xbbadbeef
01-09 10:01:02.374  4894  4894 F DEBUG   :     r0 da2799ac  r1 fffffffe  r2 bbadbeef  r3 00000000
01-09 10:01:02.374  4894  4894 F DEBUG   :     r4 d7cab390  r5 fffffffb  r6 da279b48  r7 d7cab420
01-09 10:01:02.374  4894  4894 F DEBUG   :     r8 da279c08  r9 da279a58  sl d9f35000  fp da279ff4
01-09 10:01:02.374  4894  4894 F DEBUG   :     ip d9865900  sp da279a30  lr eeb28d67  pc eeb28d9c  cpsr 40070030
01-09 10:01:02.374  4894  4894 F DEBUG   :
01-09 10:01:02.374  4894  4894 F DEBUG   : backtrace:
01-09 10:01:02.374  4894  4894 F DEBUG   :     #00 pc 00182d9c  /data/app/com.mobxtest-1/lib/arm/libjsc.so (WTFCrash+19)
01-09 10:01:02.374  4894  4894 F DEBUG   :     #01 pc 0010770f  /data/app/com.mobxtest-1/lib/arm/libjsc.so
01-09 10:01:02.374  4894  4894 F DEBUG   :     #02 pc 0010777d  /data/app/com.mobxtest-1/lib/arm/libjsc.so
01-09 10:01:02.374  4894  4894 F DEBUG   :     #03 pc 0014274f  /data/app/com.mobxtest-1/lib/arm/libjsc.so (_ZNK3JSC12PropertySlot14functionGetterEPNS_9ExecStateE+32)
01-09 10:01:02.374  4894  4894 F DEBUG   :     #04 pc 000ac18b  /data/app/com.mobxtest-1/lib/arm/libjsc.so
01-09 10:01:02.384  4894  4894 F DEBUG   :     #05 pc 000ac62b  /data/app/com.mobxtest-1/lib/arm/libjsc.so
01-09 10:01:02.384  4894  4894 F DEBUG   :     #06 pc 000b0eb3  /data/app/com.mobxtest-1/lib/arm/libjsc.so
01-09 10:01:02.834  4894  4894 F DEBUG   :
01-09 10:01:02.834  4894  4894 F DEBUG   : Tombstone written to: /data/tombstones/tombstone_02
01-09 10:01:02.834  4894  4894 E DEBUG   : AM write failed: Broken pipe
01-09 10:01:02.834  4894  4894 E         : ro.product_ship = true
01-09 10:01:02.834  4894  4894 E         : ro.debug_level = 0x4f4c
01-09 10:01:02.834  4894  4894 E         : sys.mobilecare.preload = false
01-09 10:01:02.834  7783  7783 E audit   : type=1701 msg=audit(1483952462.834:1883): auid=4294967295 uid=10229 gid=10229 ses=4294967295 subj=u:r:untrusted_app:s0:c512,c768 pid=27256 comm="mqt_js" exe="/system/bin/app_process32" sig=11

Seeing WTFCrash and 0xbbadbeef doesnt look encouraging ;)

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 .slice() before indexing. Which probably improved the performance, too, btw.

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 reserveArrayBuffer with large value upfront is the only way to increase your luck (not always to 100%, sadly).

Any chances we can get reserveArrayBuffer exported by default? Just adding export is fine, then app can import it and call upon start.

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.

Nopik commented 7 years ago

Added sample app @ https://github.com/IDTLabs/RNCrashTest

mweststrate commented 7 years ago

Hey @Nopik thanks for the extensive investigation! Really nasty...

Let's devise a plan:

exposing the functions in the extras namespace in MobX is no issue, would you prefer that on MobX 2 or 3 (which is now available as well).
Is your current work around acceptable / the user base / changes are small enough etc etc we could call it a day until we hear something from the RN team
Otherwise we could check if introducing the properties on the observableArray instance instead of it's prototype makes a difference. That is a lot slower and memory expensive though (earlier mobx versions did this)
Otherwise what could be done is introduce and expose an observable collection in MobX, that doesn't use properties but explicit methods to read / write values (similar to observable maps). That is some work though.

Nopik commented 7 years ago

v3, yay ;) Congrats! Btw. reading through the changelog reminded me how poorly disposers are documented ;)

Now, answering your points in order: 1) currently our app is on v2, but since v3 is there, we'll try to upgrade first, so maybe no v2 changes will be necessary. 2) for the time being the problem is under control, I think. My confidence in MobX arrays got shattered, but fortunately we had pretty low amount of potentially long arrays. Their usage was pretty simple and they've been all patched with slices, so no crashes are observed now and we're in some kind of stable state. So, we'll see if RN team will react (no pun intended). 3) yeah, I considered that myself earlier, but it doesnt seem a good tradeoff for our app. Adding slices in 10 places were easy & did the job for the time being. So, moving getters to each instance seems to have too big cost over the profit. 4) well, actually you could expose some get( index ) method on your array pretty easily, that would give you observable collection with custom API, I guess. Most likely it wouldn't trigger JIT bug. Especially if you'd clean it and provide 2 classes, one with getters and no custom get, the other with get, but no automagic getters. Those classes would be so similar that all the common code should be in base class. I.e. reaching clean observable array should be pretty easy task. The biggest drawback obviously would be that you cant use it with lodash and other libs, user would always need to iterate 'manually' over that. Either way, currently I'm not in pressure to have such facility.

Nopik commented 7 years ago

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 reserveArrayBuffer, we dont need it in v2.

mweststrate commented 7 years ago

@Nopik could you try mobx@3.0.1-fix734? It exposes extras.reserveArrayBuffer and .get(index), .set(index, value) on every array, which hopefully avoids hititng this bug. (Also interesting to see if unmodified, with this update, you are also not hitting this bug)

Nopik commented 7 years ago

Thanks, I'll check it out and let you know, hopefully tomorrow.

Nopik commented 7 years ago

Did you pushed the branch already? I dont see it on the list

mweststrate commented 7 years ago

now I have :)

Op di 17 jan. 2017 om 17:22 schreef Kamil Burzynski < notifications@github.com>:

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 https://github.com/mobxjs/mobx/issues/734#issuecomment-273218003, or mute the thread https://github.com/notifications/unsubscribe-auth/ABvGhIFhUKlNDdeyXnknuhF1R9JueDMSks5rTOrQgaJpZM4LcBD8 .

Nopik commented 7 years ago

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.

mweststrate commented 7 years ago

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 < notifications@github.com>:

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 https://github.com/mobxjs/mobx/issues/734#issuecomment-273422192, or mute the thread https://github.com/notifications/unsubscribe-auth/ABvGhD3PczHiTMGpwSGr5uX6C-6dEZPbks5rTdesgaJpZM4LcBD8 .

mweststrate commented 7 years ago

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....

Nopik commented 7 years ago

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.

mweststrate commented 7 years ago

haha ok, let me refactor it first to be not dependent on the property descriptors then :)

mweststrate commented 7 years ago

@Nopik would you mind testing mobx@3.0.1-fix734-2?

Nopik commented 7 years ago

Not pushed yet? ;)

mweststrate commented 7 years ago

Eh no everything pushed, didn't npm install mobx@3.0.1-fix734-2 work?

Nopik commented 7 years ago

Sorry for late response. This version seem to behave identically like the previous one.

mweststrate commented 7 years ago

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 notifications@github.com:

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 https://github.com/mobxjs/mobx/issues/734#issuecomment-275692556, or mute the thread https://github.com/notifications/unsubscribe-auth/ABvGhHTWYo98saYHUDRArQLVnIg3QLyrks5rWg2-gaJpZM4LcBD8 .

mweststrate commented 7 years ago

@Nopik any updates around this one?

mweststrate commented 7 years ago

Released the changes for this issue as part of mobx 3.1.9. Feel free to ping if any further follow up is needed.

ragamufin commented 7 years ago

I've got my own custom code that was using Object.defineProperty on an array. Unlike mobx I don't preset the array with a certain length but increase the length as needed. I was getting random errors in various points of my app on android exactly as you were/are @Nopik. It would work sometimes with debug JS turned off but it would still crash quite often and on an array with 2 or 3 items in it. If I define all the keys from the start or redefine them when increasing the length of the array it would work.

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.

andtos90 commented 6 years ago

@ragamufin I encountered the same issue and your solution works great, thanks

wbercx commented 6 years ago

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 0xbbadbeef faults, and I did already use a custom JSC build.

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 0xbbadbeef fault address. This is in a Android 6.0 emulator, and to be completely honest, I have not tried to reproduce this in later versions.

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: I've since upgraded jsc-android to 225067.0.0, and I have yet to see it crash in my emulator. I also no longer get JVM garbage collection logs in adb nearly as much anymore.

lock[bot] commented 5 years ago

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.

mobxjs / mobx

Large observable arrays do not play well with React Native Android #734