-
Notifications
You must be signed in to change notification settings - Fork 100
Disable DFG_JIT to fix possibly crash issue. #105
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, lets get this in, I'll publish this on NPM today or tomorrow the latests and lets see if people out there see any issues with this build
cpirich
commented
May 30, 2019
@Kudo thanks for your work here, we've also been bitten by SEGV_MAPPER crashes and corruption within the JavaScript side. We will try this out.
A point of clarification: in your chart above, you mention "Stock RN59" using 174650.0.2, but perhaps you mean "Stock RN58"? I think RN59 by default is now using 236355
ItsNoHax
commented
May 31, 2019
Have you published it already @kmagiera ?
Sorry it's me ask for pending new release for a while.
As there still some user report crash happens even after disable DFG JIT.
I would like to make sure we publish a new version that will truly solve the crash.
@cpirich Oops, it's my wrong for RN59. As you said, it's stock RN058. Thanks for clarification.
ItsNoHax
commented
May 31, 2019
Understandable @Kudo , thanks again for all the effort you have been putting into fixing this asap.
Uh oh!
There was an error while loading. Please reload this page.
Summary
Disable DFG JIT in JavaScriptCore to fix crash issues.
As far as I know, there are two kinds of crash.
#84 (comment)
And
facebook/react-native#24261 (comment)
Both of them are caused from DFG JIT.
To prevent further crash issues, I am proposing to disable DFG.
Some notes:
old JSC disabled DFG JIT
jsc-android disabled DFG_JIT until 236355.0.0
Test Plans
null pointer dereference on jsc-android: 236355.1.1 #84 (comment)
See detail comments in App Crash on Android OS 6 Samsung Galaxy S7 SM-G930FD (JSC Crash) 64 bit support A/libc: Fatal signal 11 (SIGSEGV) facebook/react-native#24261
Measurement
I am not going to compare with original measurement result since I don't have Pixel phone.
Instead to test on Samsung Galaxy Note5.
Four test runs: original 241213.1.0, old JSC, no_DFG_JIT, and no_JIT.
Note that I've updated render deep scale to 500.
Otherwise, I will have JNI local reference table overflow crash (JNI maximum references is 512)
chromiumICUCommit:b34251f
i18n:false
chromiumICUCommit:b34251f
i18n:false
DFG_JIT:false
chromiumICUCommit:b34251f
i18n:false
DFG_JIT:false
JIT:false
I prepared a graph to show the result visually.
https://docs.google.com/spreadsheets/d/1hqX3ai-NCpN_J6YQDTKnKNBctWnMFA6EyOdVhPvwUas/edit#gid=445596593
Screen Shot 2019年05月23日 at 6 50 52 PM
From the graph, you could see there's no much benefits from DFG JIT except the jetstream hash-map test case.
And disable DFG JIT also save about 1.3MB for libjsc.so (about 17%)
From my point of view, to disable DFG JIT we keep the performance baseline as old JSC.
However, bug free and small binary size are more meaningful to me.