-
-
Notifications
You must be signed in to change notification settings - Fork 353
fix(newArch): fixes how the availability of Turbo Modules is being detected #5064
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
Thank you for finding and fixing this! |
Android (legacy) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
534ba8c | 484.00 ms | 499.93 ms | 15.93 ms |
785ffb1 | 471.92 ms | 460.96 ms | -10.96 ms |
eb07ba3 | 470.04 ms | 473.35 ms | 3.31 ms |
7be1f99 | 454.83 ms | 461.36 ms | 6.53 ms |
d916aa3 | 425.37 ms | 427.02 ms | 1.65 ms |
3e0a5f9 | 401.72 ms | 394.98 ms | -6.74 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
534ba8c | 17.75 MiB | 20.15 MiB | 2.41 MiB |
785ffb1 | 17.75 MiB | 20.15 MiB | 2.41 MiB |
eb07ba3 | 17.75 MiB | 20.15 MiB | 2.41 MiB |
7be1f99 | 17.75 MiB | 20.15 MiB | 2.41 MiB |
d916aa3 | 17.75 MiB | 20.15 MiB | 2.41 MiB |
3e0a5f9 | 17.75 MiB | 20.15 MiB | 2.41 MiB |
The change should be mentioned on changelog, suggestion: ### Fixes
- Performance improvements when initializing the SDK with turbomodule ([#5064](https://github.com/getsentry/sentry-react-native/pull/5064)) |
Android (new) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d916aa3+dirty | 411.72 ms | 451.76 ms | 40.03 ms |
eb07ba3+dirty | 419.49 ms | 482.12 ms | 62.63 ms |
534ba8c+dirty | 472.35 ms | 537.31 ms | 64.96 ms |
3e0a5f9+dirty | 379.92 ms | 450.96 ms | 71.04 ms |
785ffb1+dirty | 380.65 ms | 451.83 ms | 71.18 ms |
7be1f99+dirty | 369.02 ms | 399.60 ms | 30.58 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d916aa3+dirty | 7.15 MiB | 8.42 MiB | 1.27 MiB |
eb07ba3+dirty | 7.15 MiB | 8.42 MiB | 1.27 MiB |
534ba8c+dirty | 7.15 MiB | 8.42 MiB | 1.27 MiB |
3e0a5f9+dirty | 7.15 MiB | 8.42 MiB | 1.27 MiB |
785ffb1+dirty | 7.15 MiB | 8.42 MiB | 1.27 MiB |
7be1f99+dirty | 7.15 MiB | 8.42 MiB | 1.27 MiB |
iOS (legacy) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
eb07ba3+dirty | 1222.46 ms | 1220.37 ms | -2.08 ms |
7be1f99+dirty | 1226.69 ms | 1217.76 ms | -8.93 ms |
534ba8c+dirty | 1230.22 ms | 1231.18 ms | 0.96 ms |
d916aa3+dirty | 1221.02 ms | 1228.98 ms | 7.96 ms |
3e0a5f9+dirty | 1226.94 ms | 1230.02 ms | 3.08 ms |
785ffb1+dirty | 1237.63 ms | 1240.50 ms | 2.87 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
eb07ba3+dirty | 2.63 MiB | 3.81 MiB | 1.18 MiB |
7be1f99+dirty | 2.63 MiB | 3.81 MiB | 1.18 MiB |
534ba8c+dirty | 2.63 MiB | 3.81 MiB | 1.18 MiB |
d916aa3+dirty | 2.63 MiB | 3.81 MiB | 1.18 MiB |
3e0a5f9+dirty | 2.63 MiB | 3.81 MiB | 1.18 MiB |
785ffb1+dirty | 2.63 MiB | 3.81 MiB | 1.18 MiB |
iOS (new) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
eb07ba3+dirty | 1214.49 ms | 1221.59 ms | 7.10 ms |
7be1f99+dirty | 1222.43 ms | 1217.15 ms | -5.28 ms |
534ba8c+dirty | 1225.00 ms | 1237.43 ms | 12.43 ms |
d916aa3+dirty | 1211.02 ms | 1221.33 ms | 10.31 ms |
3e0a5f9+dirty | 1233.65 ms | 1239.10 ms | 5.45 ms |
785ffb1+dirty | 1213.71 ms | 1213.37 ms | -0.35 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
eb07ba3+dirty | 3.19 MiB | 4.38 MiB | 1.19 MiB |
7be1f99+dirty | 3.19 MiB | 4.38 MiB | 1.19 MiB |
534ba8c+dirty | 3.19 MiB | 4.38 MiB | 1.19 MiB |
d916aa3+dirty | 3.19 MiB | 4.38 MiB | 1.19 MiB |
3e0a5f9+dirty | 3.19 MiB | 4.38 MiB | 1.19 MiB |
785ffb1+dirty | 3.19 MiB | 4.38 MiB | 1.19 MiB |
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.
LGTM! Thank you for finding this and fixing it so quickly @alwx 🏅
Let's add a changelog and 🚢
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.
LGTM 🚀
Thanks again for fixing this Alex 🙇
During an investigation of our support of the new architecture I accidentally found that the way we detect turbo modules is outdated since
__turboModuleProxy
was removed from React Native in bridgeless mode.Here is where I found that: facebook/react-native#48362
Here is more information about bridgeless mode in React Native: reactwg/react-native-new-architecture#154
And, finally, here is how React Native itself detects if Turbo Modules are available: https://github.com/facebook/react-native/blob/641a79dc5137b69a3c0813413b9fb82d0b9df783/packages/react-native/src/private/featureflags/ReactNativeFeatureFlagsBase.js#L110
📢 Type of change
📜 Description
Basically this change implements the same mechanism for detecting Turbo Modules that's already used in React Native (see link above).
💡 Motivation and Context
Solving how we detect the usage of the new architecture is crucial for correct usage of corresponding native modules.
📝 Checklist
🔮 Next steps