search

Shopify / react-native-skia

High-performance React Native Graphics using Skia
https://shopify.github.io/react-native-skia
MIT License
6.86k stars 441 forks source link

Some skia APIs deadlock on android app when using react-native-v8 #1705

Open evelant opened 1 year ago

evelant commented 1 year ago

Description

I'm attempting to switch my app from hermes to v8 on android since v8 is 4-5x faster than hermes for me. I must be tripping over some nasty hermes perf corner case. In doing so I discovered that some skia views cause the app to hang when running on v8.

Unfortunately I haven't been able to pinpoint exactly the conditions that create this yet. It seems that in some cases a component containing even an empty can hang the app. A lot of other uses of Canvas work perfectly fine however.

I'm not quite sure how to debug this. It seems likely to me that there's some difference between how hermes and v8 handle JSI libraries that's causing the issue, but I lack the knowledge to attempt to figure out what that could be.

I'm sorry this issue is vague and doesn't have a good reproduction yet. I'm attempting to narrow it down. Until then, can anybody offer some pointers on where I might start looking?

issue filed on react-native-v8: https://github.com/Kudo/react-native-v8/issues/186

Version

0.1.197

Steps to reproduce

  1. Install skia in an app alongside react-native-v8
  2. Observe that some skia views cause the app to hang. It's completely unresponsive, not even an ANR screen.

Snack, code example, screenshot, or link to a repository

Unfortunately I don't have one yet. I'm working on narrowing down the issue.

Environment: "@shopify/react-native-skia": "0.1.197", "expo": "~49.0.0", "react-native": "0.72.1", "react-native-v8": "2.2.2" "v8-android-jit": "11.1000.3"

If it's relevant, I do not have reanimated installed.

evelant commented 1 year ago

I've uncovered a little bit more info. I got the android studio debugger attached and when the app is hung it always pauses on

//in SkiaDomView.java:41

protected native void registerView(int nativeId);

Not sure if that's useful but at least it's a clue.

evelant commented 1 year ago

Further probing -- pausing the app native thread reveals it's frozen on the following stack. It appers there's a deadlock at rnv8::V8PointerValue::invalidate() image

evelant commented 1 year ago

mqt_js thread looks interesting too, it appears stuck at RNSkia::RNSkJsiViewApi::getEnsuredViewInfo

image

evelant commented 1 year ago

I'm at the end of my ability to debug this further I think. @chrfalch @wcandillon does anything obvious jump out at you as the cause of this deadlock?

chrfalch commented 1 year ago

Hi @evelant! Thanks for the insights about running RN Skia with RN V8. We've not yet done any explicit tests running react-native-v8 - and we don't have any direct requests for this meaning that we'll probl. not be able to prioritise this for now.

I've seen that @Kudo is already giving great feedback on this - we'll wait to see if he can reproduce and see the error before moving on on our side.

evelant commented 1 year ago

I figured that was the case 🙂 I'll hopefully have a bit of time tomorrow to reproduce this and will work with @Kudo on it. I filed this bug just for your awareness and for the off chance that the problem might immedidately jump out to you. Thanks for all your hard work on this awesome lib!

evelant commented 1 year ago

I think I found the root cause. Reproduction here: https://github.com/evelant/rnv8skiacrash

tl;dr -- v8 crashes or deadlocks when provided with invalid props such as NaN, hermes and JSC don't.

I don't really know how JSI works internally but I'm guessing this means it's likely a fix needed in react-native-v8, not skia.

evelant commented 1 year ago

It seems I may have been mistaken -- the issue actually appears to be something deeper involving useFont. My app on v8 seems to crash as soon as it renders skia text using a font loaded with useFont.

wcandillon commented 1 year ago

@evelant This may not be surprising. Did you switch your own loading to load fonts? useFont is just a convenience method, wrapper for

const data = await Skia.Data.fromURI(require(./font.ttf));
const font Skia.Font(Skia.Typeface.MakeFreeTypeFaceFromData(data), size)`;

I'd be curious if this works for you. If it does, we could document it and even look at improving our data loading hooks.

CyberCyclone commented 1 year ago

@chrfalch I haven't got anything to contribute to the ticket, but I just wanted to share that v8 is used for the project I'm working on because of the performance increase it gives. So while there might not be much vocal support behind v8, I think it would be something really worth while testing against as people who use Skia will have performance in mind and will probably want to run it in v8.

evelant commented 1 year ago

@wcandillon Sorry to take so long to get back to you, I was on vacation. I'm not quite understanding what you mean -- I already use useFont so if it's just a convenience wrapper for the snippet above how could that change the behavior?

wcandillon commented 1 year ago

@evelant because we have a bug in the useFont possibily

evelant commented 1 year ago

@wcandillon OK I tried loading the font manually like you specified above. Unfortunately the behavior is the same. My app still deadlocks on v8 in the same place as the stack traces above.