v2.2.2 deadlocks with react-native-skia on expo 49 and RN 0.72.1
evelant opened this issue · 23 comments
I've got a large app that's been performing badly on Hermes so I decided to give JSC or v8 a try. JSC won't run due to missing BigInt support. V8 runs and is 5x or more faster than Hermes with my app!
The downside is that the v8 version randomly locks up. It appears it could be related to images or maybe animations, but it isn't consistent. There's no further output from the app or even in logcat -- everything just stops dead with no indication of failure. Suspect packages could be react-native-mmkv or react-native-skia. The hang happens in debug or release builds.
Any ideas on how I might try to debug this? I know this issue is vague, sorry, I know it's not actionable, I'm just hoping to get some tips on how I might get some more debug info to find a root cause. Given than v8 is literally a 5x perf improvement over hermes on my app I'd love to get it running reliably.
Ah I narrowed it down by trial and error and discovered the hang came from react-native-skia. Specifically rendering text elements with skia. I'm not sure exactly which props might be causing the issue or if it's all text rendering, but at least it's greatly narrowed down.
@Kudo Hmm I'm still getting hangs on some devices when using some parts of react-native-skia. I haven't been able to pinpoint exactly what is causing it. Do you have any ideas about what might be going wrong there since skia is a JSI lib? It does not freeze on hermes so I'm thinking there could be some difference/bug in the JSI bindings for v8 causing this problem.
@Kudo I found some more info by pausing the app thread with the debugger. It appers there's a deadlock at rnv8::V8PointerValue::invalidate(). I'm not sure if that's a problem coming from skia or a bug in rnv8 though. My knowledge of c++ is very limited.
ohohoh that's awesome! i had locks in rnv8 and was thinking about to remove the locks for the performance issue. will try to investigate that. thanks for the great investigation.
@Kudo Glad that info is helpful! Hopefully it's simple to fix. I discovered that my app is 4-5x faster with v8 than hermes! Unfortunately this deadlock prevents production use.
FYI I found a little more info in mqt_js
@Kudo Any thoughts on how this might be fixed? I'd love to re-enable v8 in my app but unfortunately I have basically no idea about how to debug this.
@Kudo Unfortunately it does not work. It doesn't deadlock anymore but it crashes instead.
The crashing thread -- mqt_js
mqt_native_modules
I ran it again and the stack is slightly different on the crash
Again the crashing thread, mqt_js
For whatever reason there's more than one thread labeled mqt_js... this one has some frames from skia
Tried a couple more times and one time the app ran for a little while before crashing. Different stack captured on this one. Hopefully these are helpful!
From the main thread
oh no feels like a threading issue between react-native-skia & react-native-v8. could you help to create a minimal reproducible example?
ps. sorry i would response a little late from my vacation 😅
No worries, thanks so much for looking into this and for making this awesome library! I'll see if I can put together a minimal reproduction and get back to you here.
@Kudo pretty sure I figured out the root cause, reproduction here: https://github.com/evelant/rnv8skiacrash
If you pass an invalid prop to a JSI method on v8 it deadlocks or crashes. On hermes or JSC it results in an error. In this case I pass NaN to a skia API. I verified that this deadlocks v8 but not the other engines.
I'm guessing (since I don't really know how JSI works internally) that react-native-v8 might be missing some safety checks on prop values?
@Kudo I narrowed it down even further. It is most definitely a case of invalid prop types being unchecked by react-native-v8. I found out that my app was passing a value to SkiaText that was a number not a string. That's fine in JS, it should be implicitly coerced (so typescript didn't pick it up), but with react-native-v8 it actually passes the number to the JSI function which results in the crash/deadlock!
Hmm perhaps I spoke too soon. There seems to be something else at play here. Even if I make sure everything is of the correct type I can still observe a deadlock that seems to happen as soon as the font is loaded.
sorry for late response. now i figured out the problem is actually coming from my problem to build libv8android.so by ndk r21. it will cause crash because inconsistent ndk version between react-native. will try to start new libv8android.so.
upgrade v8-android-jit@11.1000.4 should resolve the problem. let me know if there are still other problems. thanks for the great support!
@Kudo Unfortunately even after upgrading to that version I'm still seeing the same deadlock
I also tried the new v8-android-jit version with your patch above to remove locks -- same outcome unfortunately, instead of deadlocking the app just crashes.
oh no 🙈
based on your repro, only set the frontWidth to NaN is enough to trigger the deadlock?
Unfortunately it seems to be something deeper than that. Setting props to NaN does seem to trigger the issue, but it still happens even when I've ensured that the props are always valid. It's not entirely consistent either -- sometimes it will deadlock right away but other times it will work for a short period of time before deadlocking.