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

impr: Speed up getBinaryImages V2 #4539

Merged
merged 8 commits into from
Nov 21, 2024

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Nov 15, 2024

📜 Description

Follow up on GH-4435. Use binary image cache to get debug meta when finishing the tracer and capturing profiles. Mark all methods to get debug meta not using the cache as deprecated to ensure we migrate to the methods using the cache.

💡 Motivation and Context

Our customer reported that even with GH-4435, they still see app hangs.

💚 How did you test it?

Unit tests and simulator SwiftUI transactions.

Profiles

Main branch

CleanShot 2024-11-21 at 09 44 48@2x

Feature Branch
CleanShot 2024-11-21 at 09 45 24@2x

All debug images for app start transactions

I randomly picked two images

Main branch

{
  "code_file": "/Library/Developer/CoreSimulator/Volumes/iOS_22A3351/Library/Developer/CoreSimulator/Profiles/Runtimes/iOS 18.0.simruntime/Contents/Resources/RuntimeRoot/System/Library/PrivateFrameworks/WebCore.framework/WebCore",
  "debug_id": "af2661eb-019f-3f23-a03a-b12bb4f9186a",
  "image_addr": "0x18cc65000",
  "image_size": 47603712,
  "image_vmaddr": "0x18cc65000",
  "type": "macho"
},
{
"code_file": "/Users/philipp.hofmann/Library/Developer/CoreSimulator/Devices/479CAA0E-3CC9-4B32-BBA2-B6A9E85F1C65/data/Containers/Bundle/Application/46935A91-04AD-4D90-8F90-406F97CF1996/iOS-SwiftUI.app/iOS-SwiftUI",
"debug_id": "930c0860-1359-373c-9138-1bc884df22ab",
"image_addr": "0x104550000",
"image_size": 16384,
"image_vmaddr": "0x100000000",
"type": "macho"
},

This feature branch

{
  "code_file": "/Library/Developer/CoreSimulator/Volumes/iOS_22A3351/Library/Developer/CoreSimulator/Profiles/Runtimes/iOS 18.0.simruntime/Contents/Resources/RuntimeRoot/System/Library/PrivateFrameworks/WebCore.framework/WebCore",
  "debug_id": "af2661eb-019f-3f23-a03a-b12bb4f9186a",
  "image_addr": "0x18cc65000",
  "image_size": 47603712,
  "image_vmaddr": "0x18cc65000",
  "type": "macho"
},
{
  "code_file": "/Users/philipp.hofmann/Library/Developer/CoreSimulator/Devices/479CAA0E-3CC9-4B32-BBA2-B6A9E85F1C65/data/Containers/Bundle/Application/5D77FF49-A7D7-426D-A5E8-B66D0C2AE829/iOS-SwiftUI.app/iOS-SwiftUI",
  "debug_id": "930c0860-1359-373c-9138-1bc884df22ab",
  "image_addr": "0x102810000",
  "image_size": 16384,
  "image_vmaddr": "0x100000000",
  "type": "macho"
},

Release build profile on main branch

Release build profile on feature branch

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Follow up on GH-4435. Use binary image cache to get debug meta when
finishing the tracer and capturing profiles. Mark all methods to get
debug meta not using the cache as deprecated to ensure we migrate to the
methods using the cache.
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

codecov bot commented Nov 15, 2024

Codecov Report

Attention: Patch coverage is 96.47059% with 6 lines in your changes missing coverage. Please review.

Project coverage is 90.917%. Comparing base (4d22351) to head (3d34d27).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
SentryTestUtils/TestDebugImageProvider.swift 72.727% 6 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4539       +/-   ##
=============================================
- Coverage   90.925%   90.917%   -0.009%     
=============================================
  Files          617       617               
  Lines        70439     70585      +146     
  Branches     25161     25218       +57     
=============================================
+ Hits         64047     64174      +127     
- Misses        6299      6319       +20     
+ Partials        93        92        -1     
Files with missing lines Coverage Δ
Sources/Sentry/PrivateSentrySDKOnly.mm 89.285% <100.000%> (+0.166%) ⬆️
...es/Sentry/Profiling/SentryProfilerSerialization.mm 91.538% <100.000%> (+0.065%) ⬆️
Sources/Sentry/SentryBinaryImageCache.m 100.000% <100.000%> (ø)
Sources/Sentry/SentryDebugImageProvider.m 88.000% <100.000%> (+1.636%) ⬆️
Sources/Sentry/SentryTracer.m 97.377% <100.000%> (ø)
...SentryProfilerTests/SentryProfileTestFixture.swift 99.703% <100.000%> (+0.011%) ⬆️
Tests/SentryTests/PrivateSentrySDKOnlyTests.swift 99.752% <100.000%> (+0.016%) ⬆️
...ests/SentryTests/SentryBinaryImageCacheTests.swift 100.000% <100.000%> (ø)
...ts/SentryCrash/SentryDebugImageProviderTests.swift 100.000% <100.000%> (ø)
...ts/SentryTests/Transaction/SentryTracerTests.swift 98.466% <100.000%> (+0.002%) ⬆️
... and 1 more

... and 13 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 4d22351...3d34d27. Read the comment docs.

---- 🚨 Try these New Features:

Copy link

github-actions bot commented Nov 15, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1237.78 ms 1248.94 ms 11.16 ms
Size 22.30 KiB 747.50 KiB 725.19 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
fc350a4 1240.47 ms 1263.45 ms 22.98 ms
3437454 1225.64 ms 1234.31 ms 8.67 ms
3b4110a 1228.90 ms 1247.65 ms 18.76 ms
8123181 1223.53 ms 1241.73 ms 18.20 ms
dd0557f 1242.04 ms 1250.86 ms 8.82 ms
e681215 6240.94 ms 6254.36 ms 13.42 ms
2af280d 1219.33 ms 1239.98 ms 20.65 ms
11ccc16 1203.82 ms 1237.06 ms 33.24 ms
98a8c16 1243.33 ms 1257.86 ms 14.53 ms
aeec206 1229.27 ms 1253.70 ms 24.43 ms

App size

Revision Plain With Sentry Diff
fc350a4 21.90 KiB 708.34 KiB 686.44 KiB
3437454 22.85 KiB 408.87 KiB 386.02 KiB
3b4110a 21.58 KiB 625.82 KiB 604.24 KiB
8123181 21.58 KiB 675.79 KiB 654.21 KiB
dd0557f 22.85 KiB 411.75 KiB 388.90 KiB
e681215 20.76 KiB 431.91 KiB 411.15 KiB
2af280d 20.76 KiB 435.22 KiB 414.46 KiB
11ccc16 20.76 KiB 431.71 KiB 410.95 KiB
98a8c16 20.76 KiB 431.00 KiB 410.24 KiB
aeec206 20.76 KiB 434.88 KiB 414.12 KiB

Previous results on branch: impr/speed-up-get-debug-images-v2

Startup times

Revision Plain With Sentry Diff
321ca4f 1225.15 ms 1240.20 ms 15.06 ms
74f1006 1240.00 ms 1249.88 ms 9.88 ms
da9ddd8 1233.22 ms 1249.96 ms 16.73 ms
d2daa72 1222.51 ms 1249.30 ms 26.79 ms

App size

Revision Plain With Sentry Diff
321ca4f 22.30 KiB 747.50 KiB 725.20 KiB
74f1006 22.30 KiB 730.96 KiB 708.66 KiB
da9ddd8 22.30 KiB 730.78 KiB 708.47 KiB
d2daa72 22.30 KiB 747.79 KiB 725.48 KiB

@philipphofmann
Copy link
Member Author

I can't validate this right now, because I can't check sample transactions in our sentry-sdks as something is broken with view traces. I posted a message in Slack #discuss-performance, and will retry once the problem is fixed.

@philipphofmann philipphofmann merged commit d70a7e8 into main Nov 21, 2024
64 of 65 checks passed
@philipphofmann philipphofmann deleted the impr/speed-up-get-debug-images-v2 branch November 21, 2024 10:01
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