Skip to content

Fix: Improve platform memory collection on windows/linux#2774

Closed
denrase wants to merge 17 commits intomainfrom
fix/windows-paltform-memory
Closed

Fix: Improve platform memory collection on windows/linux#2774
denrase wants to merge 17 commits intomainfrom
fix/windows-paltform-memory

Conversation

@denrase
Copy link
Copy Markdown
Collaborator

@denrase denrase commented Mar 5, 2025 •
edited
Loading

📜 Description

A user reported an issue where his http calls are way slower if wrapped in sentry transactions. Turns out this is because we call external binaries with exec for every transaction to get physical and free memory size. Turns out this has a large overhead. Also the binary used on windows was deprecated and may therefore not be present. So we have the large overhead without the benefit.

This PR introduces the following to mitigate/fix all of the above:

  • Disable platform memory collection per default per options.
  • If enabled:
    • Cache physical memory indefinitely
    • Cache free memory for one minute
    • On windows, fallback to powershell if wmci is not available.
  • Avoid copying of contexts.

💡 Motivation and Context

Relates to #2760
Relates to onepub-dev/system_info#12

💚 How did you test it?

Ran locally in a win 11 vm.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 5, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 65.51724% with 20 lines in your changes missing coverage. Please review.

Project coverage is 88.55%. Comparing base (98f7055) to head (3df766c).

Files with missing lines Patch % Lines
...c/event_processor/enricher/io_platform_memory.dart 35.48% 20 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2774      +/-   ##
==========================================
- Coverage   88.60%   88.55%   -0.05%     
==========================================
  Files         263      263              
  Lines        8766     8784      +18     
==========================================
+ Hits         7767     7779      +12     
- Misses        999     1005       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 5, 2025 •
edited
Loading

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 417.76 ms 478.36 ms 60.60 ms
Size 6.44 MiB 7.56 MiB 1.12 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
68677de 364.41 ms 415.61 ms 51.20 ms
8e133ad 360.08 ms 402.82 ms 42.74 ms
9a5040f 490.15 ms 563.98 ms 73.83 ms
6aab859 306.27 ms 377.92 ms 71.65 ms
752e1cb 472.85 ms 511.60 ms 38.76 ms
2e1e4ae 413.34 ms 509.24 ms 95.90 ms
0db91cc 327.85 ms 387.31 ms 59.46 ms
84bc635 395.57 ms 464.23 ms 68.66 ms
134c9f8 301.40 ms 352.65 ms 51.26 ms
c328ffc 394.35 ms 480.94 ms 86.59 ms

App size

Revision Plain With Sentry Diff
68677de 6.06 MiB 7.10 MiB 1.04 MiB
8e133ad 6.06 MiB 7.03 MiB 990.29 KiB
9a5040f 6.46 MiB 7.48 MiB 1.01 MiB
6aab859 6.15 MiB 7.13 MiB 999.97 KiB
752e1cb 6.49 MiB 7.57 MiB 1.08 MiB
2e1e4ae 6.35 MiB 7.42 MiB 1.06 MiB
0db91cc 5.94 MiB 6.95 MiB 1.01 MiB
84bc635 6.34 MiB 7.28 MiB 968.41 KiB
134c9f8 5.94 MiB 6.95 MiB 1.01 MiB
c328ffc 6.35 MiB 7.42 MiB 1.07 MiB

Previous results on branch: fix/windows-paltform-memory

Startup times

Revision Plain With Sentry Diff
77ae740 390.55 ms 508.32 ms 117.77 ms
4211fff 437.53 ms 516.20 ms 78.67 ms
1e0e22b 399.46 ms 494.48 ms 95.02 ms

App size

Revision Plain With Sentry Diff
77ae740 6.44 MiB 7.50 MiB 1.06 MiB
4211fff 6.46 MiB 7.48 MiB 1.02 MiB
1e0e22b 6.46 MiB 7.48 MiB 1.02 MiB

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 5, 2025 •
edited
Loading

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1251.04 ms 1274.17 ms 23.13 ms
Size 8.43 MiB 9.98 MiB 1.55 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
abfcdb5 1230.87 ms 1244.94 ms 14.06 ms
4d046e0 1243.08 ms 1255.98 ms 12.90 ms
0ceb89c 1252.02 ms 1271.78 ms 19.75 ms
f4cc744 1274.57 ms 1290.79 ms 16.22 ms
c73ab67 1267.73 ms 1279.36 ms 11.63 ms
dd25e43 1261.59 ms 1275.81 ms 14.22 ms
2e93bab 1237.08 ms 1258.41 ms 21.33 ms
ee0ca56 1236.44 ms 1258.89 ms 22.45 ms
bd75526 1252.62 ms 1287.00 ms 34.38 ms
178baee 1230.61 ms 1249.94 ms 19.33 ms

App size

Revision Plain With Sentry Diff
abfcdb5 8.33 MiB 9.64 MiB 1.31 MiB
4d046e0 8.42 MiB 9.83 MiB 1.40 MiB
0ceb89c 8.15 MiB 9.12 MiB 989.78 KiB
f4cc744 8.16 MiB 9.16 MiB 1.01 MiB
c73ab67 8.29 MiB 9.36 MiB 1.07 MiB
dd25e43 8.42 MiB 9.87 MiB 1.44 MiB
2e93bab 8.38 MiB 9.73 MiB 1.36 MiB
ee0ca56 8.32 MiB 9.52 MiB 1.20 MiB
bd75526 8.38 MiB 9.76 MiB 1.39 MiB
178baee 8.34 MiB 9.67 MiB 1.33 MiB

Previous results on branch: fix/windows-paltform-memory

Startup times

Revision Plain With Sentry Diff
eab117b 1255.10 ms 1280.49 ms 25.39 ms
4211fff 1242.37 ms 1247.50 ms 5.13 ms
77ae740 1255.69 ms 1276.79 ms 21.09 ms
1e0e22b 1241.89 ms 1260.23 ms 18.34 ms

App size

Revision Plain With Sentry Diff
eab117b 8.43 MiB 9.98 MiB 1.55 MiB
4211fff 8.42 MiB 9.97 MiB 1.55 MiB
77ae740 8.43 MiB 9.97 MiB 1.54 MiB
1e0e22b 8.42 MiB 9.97 MiB 1.55 MiB

@denrase denrase marked this pull request as ready for review March 5, 2025 12:52
@buenaflor
Copy link
Copy Markdown
Contributor

buenaflor commented Mar 11, 2025

judging by the users comment it doesn't fix the issue: #2760 (comment)

maybe the issue is elsewhere?

@denrase
Copy link
Copy Markdown
Collaborator Author

denrase commented Mar 11, 2025

@buenaflor Will verify again and continue searching.

@denrase denrase changed the title Fix: Cache platform memory on windows/linux Fix: Improve platform memory collection on windows/linux Mar 12, 2025
buenaflor
buenaflor reviewed Mar 12, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@buenaflor buenaflor left a comment

Choose a reason for hiding this comment

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

note: now that we merged v9 into main, we need to merge this PR into a separate branch. perhaps temp/8.14.1

buenaflor
buenaflor reviewed Mar 13, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@buenaflor buenaflor left a comment •
edited
Loading

Choose a reason for hiding this comment

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

overall I wonder whether we should just ditch fetching free memory for now and only keep total memory. total memory should be fine to keep as it is only grabbed once and cached indefinitely.

introducing another flag and making it opt-out means people most likely won't use this feature at all even if we document it properly and tbh I'd rather not add another flag for this as it doesn't have so much impact

In the future we could even use ffi to access the GlobalMemoryStatusEx on windows. see this or this, most likely has far better performance than the current impl

denrase added 4 commits March 14, 2025 10:35
@denrase
Merge branch 'main' into fix/windows-paltform-memory
fa95c97 
# Conflicts:
#	dart/lib/src/event_processor/enricher/io_platform_memory.dart
#	flutter/lib/src/event_processor/flutter_enricher_event_processor.dart
@denrase
fix cl
8781a39
@denrase
add test to set runtimes
eab117b
@denrase
remove free memory collection, make total memory collection async
6568c55
buenaflor
buenaflor reviewed Mar 14, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@buenaflor buenaflor left a comment

Choose a reason for hiding this comment

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

I prepared the branch temp/8.14.1 where we should merge this into, right now it's using main which is v9 as base branch

@denrase denrase changed the base branch from main to temp/8.14.1 March 17, 2025 12:20
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 17, 2025

🚨 Detected changes in high risk code 🚨

High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:

  • flutter/android/src/main/kotlin/io/sentry/flutter/SentryFlutterPlugin.kt
  • flutter/lib/src/integrations/native_app_start_integration.dart
  • flutter/lib/src/native/java/android_replay_recorder.dart
  • flutter/lib/src/screenshot/recorder.dart

@denrase denrase changed the base branch from temp/8.14.1 to main March 17, 2025 12:48
@denrase
Copy link
Copy Markdown
Collaborator Author

denrase commented Mar 17, 2025

Clsed in favor of #2798

@denrase denrase closed this Mar 17, 2025
@denrase denrase mentioned this pull request Mar 17, 2025
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@buenaflor buenaflor buenaflor left review comments

@krystofwoldrich krystofwoldrich Awaiting requested review from krystofwoldrich

@stefanosiano stefanosiano Awaiting requested review from stefanosiano

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@denrase @buenaflor