-
-
Notifications
You must be signed in to change notification settings - Fork 277
Revert FFI usage on iOS/macOS #3379
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
Android Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 73a3c38 | 478.18 ms | 526.62 ms | 48.44 ms |
| 40c8f93 | 417.10 ms | 482.60 ms | 65.50 ms |
| 8541716 | 437.14 ms | 443.65 ms | 6.51 ms |
| d3fb366 | 391.49 ms | 385.85 ms | -5.64 ms |
| 2cf9161 | 454.12 ms | 512.67 ms | 58.55 ms |
| 1f639ee | 429.98 ms | 476.60 ms | 46.62 ms |
| b6c8720 | 457.41 ms | 519.04 ms | 61.63 ms |
| a69a51f | 437.18 ms | 450.60 ms | 13.42 ms |
| 6bcdc99 | 440.23 ms | 435.77 ms | -4.46 ms |
| 393f8ec | 360.07 ms | 362.70 ms | 2.64 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 73a3c38 | 6.54 MiB | 7.69 MiB | 1.15 MiB |
| 40c8f93 | 13.93 MiB | 15.00 MiB | 1.06 MiB |
| 8541716 | 13.93 MiB | 15.00 MiB | 1.06 MiB |
| d3fb366 | 13.93 MiB | 15.06 MiB | 1.13 MiB |
| 2cf9161 | 6.54 MiB | 7.70 MiB | 1.16 MiB |
| 1f639ee | 13.93 MiB | 15.00 MiB | 1.06 MiB |
| b6c8720 | 6.54 MiB | 7.69 MiB | 1.15 MiB |
| a69a51f | 13.93 MiB | 15.06 MiB | 1.13 MiB |
| 6bcdc99 | 13.93 MiB | 15.00 MiB | 1.06 MiB |
| 393f8ec | 13.93 MiB | 15.06 MiB | 1.13 MiB |
Previous results on branch: revert/ios-ffi
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 3a5d08f | 463.49 ms | 509.22 ms | 45.73 ms |
| 913a00b | 367.70 ms | 353.00 ms | -14.70 ms |
| 1adceb6 | 418.08 ms | 449.76 ms | 31.67 ms |
| ab2ec2e | 363.49 ms | 378.42 ms | 14.93 ms |
| e929274 | 368.77 ms | 385.67 ms | 16.90 ms |
| 670268c | 384.94 ms | 393.43 ms | 8.49 ms |
| ead6989 | 364.43 ms | 394.60 ms | 30.16 ms |
| a0d6a57 | 448.85 ms | 468.39 ms | 19.53 ms |
| 6b90375 | 367.81 ms | 378.20 ms | 10.40 ms |
| f50f35f | 403.15 ms | 440.33 ms | 37.17 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 3a5d08f | 13.93 MiB | 15.18 MiB | 1.25 MiB |
| 913a00b | 13.93 MiB | 15.18 MiB | 1.25 MiB |
| 1adceb6 | 13.93 MiB | 15.18 MiB | 1.25 MiB |
| ab2ec2e | 13.93 MiB | 15.18 MiB | 1.25 MiB |
| e929274 | 13.93 MiB | 15.18 MiB | 1.25 MiB |
| 670268c | 13.93 MiB | 15.18 MiB | 1.25 MiB |
| ead6989 | 13.93 MiB | 15.18 MiB | 1.25 MiB |
| a0d6a57 | 13.93 MiB | 15.18 MiB | 1.25 MiB |
| 6b90375 | 13.93 MiB | 15.18 MiB | 1.25 MiB |
| f50f35f | 13.93 MiB | 15.18 MiB | 1.25 MiB |
iOS Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| c0dde50 | 1268.90 ms | 1275.61 ms | 6.71 ms |
| 1f639ee | 1252.43 ms | 1257.82 ms | 5.38 ms |
| 7cfee3b | 1260.90 ms | 1273.14 ms | 12.24 ms |
| 192b44c | 1269.08 ms | 1275.52 ms | 6.44 ms |
| 7b21e8b | 1256.79 ms | 1267.12 ms | 10.33 ms |
| 6f47800 | 1247.52 ms | 1259.37 ms | 11.85 ms |
| ad121c0 | 1275.04 ms | 1280.59 ms | 5.55 ms |
| e04b24b | 1230.22 ms | 1233.90 ms | 3.67 ms |
| 819c1e7 | 1250.59 ms | 1249.08 ms | -1.51 ms |
| d0aa4b6 | 1268.23 ms | 1268.39 ms | 0.15 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| c0dde50 | 5.53 MiB | 6.01 MiB | 488.14 KiB |
| 1f639ee | 5.53 MiB | 6.00 MiB | 479.95 KiB |
| 7cfee3b | 20.70 MiB | 22.46 MiB | 1.75 MiB |
| 192b44c | 5.53 MiB | 5.96 MiB | 444.33 KiB |
| 7b21e8b | 5.53 MiB | 6.00 MiB | 479.96 KiB |
| 6f47800 | 7.86 MiB | 9.44 MiB | 1.58 MiB |
| ad121c0 | 5.53 MiB | 6.01 MiB | 488.11 KiB |
| e04b24b | 5.53 MiB | 6.00 MiB | 480.00 KiB |
| 819c1e7 | 5.53 MiB | 6.00 MiB | 479.96 KiB |
| d0aa4b6 | 5.53 MiB | 6.02 MiB | 502.04 KiB |
Previous results on branch: revert/ios-ffi
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1edd51f | 1237.02 ms | 1242.67 ms | 5.65 ms |
| a7204e6 | 1255.06 ms | 1261.69 ms | 6.63 ms |
| 6f1acd0 | 1273.73 ms | 1277.47 ms | 3.73 ms |
| 4e98d39 | 1253.35 ms | 1256.76 ms | 3.41 ms |
| ab2ec2e | 1232.69 ms | 1248.23 ms | 15.55 ms |
| 913a00b | 1268.16 ms | 1263.40 ms | -4.76 ms |
| f50f35f | 1272.48 ms | 1279.51 ms | 7.03 ms |
| ead6989 | 1275.31 ms | 1275.08 ms | -0.22 ms |
| 6b90375 | 1251.27 ms | 1251.98 ms | 0.71 ms |
| 75a5e32 | 1251.67 ms | 1257.08 ms | 5.41 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1edd51f | 5.53 MiB | 5.96 MiB | 444.85 KiB |
| a7204e6 | 5.53 MiB | 5.96 MiB | 444.50 KiB |
| 6f1acd0 | 5.53 MiB | 5.96 MiB | 444.86 KiB |
| 4e98d39 | 5.53 MiB | 5.96 MiB | 444.50 KiB |
| ab2ec2e | 5.53 MiB | 5.96 MiB | 444.49 KiB |
| 913a00b | 5.53 MiB | 5.96 MiB | 444.49 KiB |
| f50f35f | 5.53 MiB | 5.96 MiB | 444.86 KiB |
| ead6989 | 5.53 MiB | 5.96 MiB | 444.49 KiB |
| 6b90375 | 5.53 MiB | 5.96 MiB | 444.86 KiB |
| 75a5e32 | 5.53 MiB | 5.96 MiB | 444.86 KiB |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3379 +/- ##
==========================================
+ Coverage 88.31% 91.05% +2.73%
==========================================
Files 291 95 -196
Lines 9957 3198 -6759
==========================================
- Hits 8794 2912 -5882
+ Misses 1163 286 -877 ☔ View full report in Codecov by Sentry. |
packages/flutter/ios/sentry_flutter/Sources/sentry_flutter/SentryFlutter.swift
Outdated
Show resolved
Hide resolved
packages/flutter/ios/sentry_flutter/Sources/sentry_flutter/SentryFlutter.swift
Outdated
Show resolved
Hide resolved
...utter/ios/sentry_flutter/Sources/sentry_flutter_objc/SentryFlutterReplayScreenshotProvider.m
Show resolved
Hide resolved
packages/flutter/ios/sentry_flutter/Sources/sentry_flutter/SentryFlutter.swift
Outdated
Show resolved
Hide resolved
denrase
left a comment
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.
Should check why ci fails.
packages/flutter/example/integration_test/platform_integrations_test.dart
Outdated
Show resolved
Hide resolved
packages/flutter/ios/sentry_flutter/Sources/sentry_flutter/SentryFlutterPlugin.swift
Show resolved
Hide resolved
packages/flutter/test/user_interaction/sentry_user_interaction_widget_test.dart
Show resolved
Hide resolved
packages/flutter/ios/sentry_flutter/Sources/sentry_flutter/SentryFlutterPlugin.swift
Outdated
Show resolved
Hide resolved
...s/sentry_flutter/Sources/sentry_flutter_objc/include/SentryFlutterReplayScreenshotProvider.h
Show resolved
Hide resolved
packages/flutter/ios/sentry_flutter/Sources/sentry_flutter/SentryFlutterPlugin.swift
Show resolved
Hide resolved
| : NSObject <SentryViewScreenshotProvider> | ||
|
|
||
| - (instancetype)initWithCallback:(SentryReplayCaptureCallback _Nonnull)callback; | ||
| - (instancetype)initWithChannel:(id)FlutterMethodChannel; |
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.
Bug: Header-implementation signature mismatch for initWithChannel method
The header declares - (instancetype)initWithChannel:(id)FlutterMethodChannel; where FlutterMethodChannel appears as the parameter name with id as the type. The implementation in SentryFlutterReplayScreenshotProvider.m uses (FlutterMethodChannel *_Nonnull)channel. This inconsistency between the header and implementation signatures may cause unexpected behavior. The header should use (FlutterMethodChannel *)channel to match the implementation.
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.
@denrase is this valid? I don't know much about objc so I'm not sure. I just reverted the header and implementation file to a previous commit and this was the state for a long time.
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.
My best guess is that we are using id here so we can pass an object that is not of type FlutterMethodChannel , but a subclass or object implementing the same methods. Right now you could call this intiializer with any object. Maybe we can check the PR where this was implemented why it was done in this way. Ideally the header and implementation files should have the same signatures.
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.
I can follow up on this, perhaps using @class FlutterMethodChannel can work here as well
packages/flutter/ios/sentry_flutter/Sources/sentry_flutter/SentryFlutter.swift
Show resolved
Hide resolved
...s/sentry_flutter/Sources/sentry_flutter_objc/include/SentryFlutterReplayScreenshotProvider.h
Show resolved
Hide resolved
packages/flutter/ios/sentry_flutter/Sources/sentry_flutter/SentryFlutter.swift
Show resolved
Hide resolved
...s/sentry_flutter/Sources/sentry_flutter_objc/include/SentryFlutterReplayScreenshotProvider.h
Show resolved
Hide resolved
denrase
left a comment
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.
Looks good, we can check the header/implementation issue, but seeing as this worked before, we can also leave this for now and get back to it.
📜 Description
Revert FFI usage on iOS/macOS
💡 Motivation and Context
Due to issues with the
objective_cpackage in statically linked frameworks we will revert this as it's a breaking change.Note: profiling will still use FFI (it has been using FFI for a year now) so we are just reverting to the state before we worked on updating the FFI impl
💚 How did you test it?
Existing tests
📝 Checklist
sendDefaultPiiis enabled🔮 Next steps