Skip to content
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

feat(internal): change collectProfilerForTrace result to mutable #3473

Merged
merged 1 commit into from
Dec 2, 2023

Conversation

vaind
Copy link
Collaborator

@vaind vaind commented Dec 1, 2023

When testing the native profiling integration for iOS in .NET, same way I've done it in Flutter, I've found I could use the collectProfileOutput directly without copying, which would, just if it was exposed as the NSMutableDictionary that it actually is.

See https://github.com/getsentry/sentry-dotnet/pull/2930/files#diff-1b71762e3c3bd42c9d55060467f9fcbc82b733f625ee6310ee76a6ff35a9e2d1R39

#skip-changelog - this probably doesn't make sense to mention to standard SDK consumers

Copy link

github-actions bot commented Dec 1, 2023

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 4244322

Copy link

codecov bot commented Dec 1, 2023

Codecov Report

Merging #3473 (4244322) into main (dfde86d) will decrease coverage by 0.027%.
Report is 1 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3473       +/-   ##
=============================================
- Coverage   89.031%   89.005%   -0.027%     
=============================================
  Files          525       525               
  Lines        56746     56765       +19     
  Branches     20416     20427       +11     
=============================================
+ Hits         50522     50524        +2     
- Misses        5308      5318       +10     
- Partials       916       923        +7     
Files Coverage Δ
Sources/Sentry/PrivateSentrySDKOnly.mm 87.155% <ø> (ø)

... and 19 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfde86d...4244322. Read the comment docs.

Copy link

github-actions bot commented Dec 1, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1230.67 ms 1238.04 ms 7.37 ms
Size 21.58 KiB 414.59 KiB 393.01 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
6604dbb 1248.35 ms 1256.14 ms 7.79 ms
7f691b5 1233.94 ms 1243.80 ms 9.86 ms
7bc3c0d 1212.35 ms 1228.94 ms 16.59 ms
b9b0f0a 1251.45 ms 1257.86 ms 6.41 ms
1d11695 1219.57 ms 1243.52 ms 23.95 ms
be2977c 1243.94 ms 1258.59 ms 14.65 ms
06548c0 1226.71 ms 1252.37 ms 25.66 ms
ddc9b9a 1201.71 ms 1226.70 ms 24.99 ms
ca91a5c 1234.53 ms 1249.86 ms 15.33 ms
5e66a38 1224.16 ms 1237.76 ms 13.60 ms

App size

Revision Plain With Sentry Diff
6604dbb 22.84 KiB 402.56 KiB 379.72 KiB
7f691b5 20.76 KiB 420.55 KiB 399.79 KiB
7bc3c0d 20.76 KiB 427.35 KiB 406.59 KiB
b9b0f0a 20.76 KiB 434.94 KiB 414.18 KiB
1d11695 20.76 KiB 401.60 KiB 380.84 KiB
be2977c 22.85 KiB 407.67 KiB 384.82 KiB
06548c0 20.76 KiB 427.36 KiB 406.59 KiB
ddc9b9a 20.76 KiB 420.40 KiB 399.65 KiB
ca91a5c 22.84 KiB 403.19 KiB 380.34 KiB
5e66a38 22.85 KiB 408.88 KiB 386.03 KiB

@vaind vaind marked this pull request as ready for review December 1, 2023 19:01
Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fine. Not sure about how C# works with it, but in ObjC, if you put a NSMutableDictionary inside a NSDictionary reference, you don't even have to call mutableCopy on it... you could just cast it back to a NSMutableDictionary reference... the underlying object type never changed, just the reference type you're using. (That's why it's so important to use the copy directive on ObjC @property declarations for types that have mutable counterparts; otherwise, you could internally store a NSMutableDictionary, and even if you only expose it as a NSDictionary, consumers could cast it back to the mutable type and mutate it.)

@vaind
Copy link
Collaborator Author

vaind commented Dec 1, 2023

Should be fine. Not sure about how C# works with it, but in ObjC, if you put a NSMutableDictionary inside a NSDictionary reference, you don't even have to call mutableCopy on it... you could just cast it back to a NSMutableDictionary reference... the underlying object type never changed, just the reference type you're using. (That's why it's so important to use the copy directive on ObjC @property declarations for types that have mutable counterparts; otherwise, you could internally store a NSMutableDictionary, and even if you only expose it as a NSDictionary, consumers could cast it back to the mutable type and mutate it.)

Yeah, I tried casting first but that didn't compile. Something to do with the bridging code I guess.

@vaind vaind merged commit e998fd0 into main Dec 2, 2023
69 of 71 checks passed
@vaind vaind deleted the feat/hybrid-sdk-profile-mutable branch December 2, 2023 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants