-
-
Notifications
You must be signed in to change notification settings - Fork 248
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
[Flutter] startTransaction slows down grpc Requests on windows #2760
Comments
@Knupper thank you for the detailed report and the repro, that's really helpful and appreciated. We'll look into this and follow up here. |
@Knupper thx for all the details! i will check this out and get back to you 🙇 |
Think i got it. We are adding device info to every transaction. On windows/linux machines we run a process to get memory size in our |
@kahest What do you think would the best approach be here?
|
@denrase can you clarify "get memory size in our PlatformMemory class" - is this system RAM? if so, we can just cache it, it won't change on the fly too often :) |
memorySize: device?.memorySize ?? platformMemory.getTotalPhysicalMemory(),
freeMemory: device?.freeMemory ?? platformMemory.getFreePhysicalMemory(), Total is easy, only need to cache once. |
Ok, the command we are using on windows was deprecated and is not part of the system per default. https://learn.microsoft.com/en-us/windows/win32/wmisdk/wmic So we always call the process and get null for the value. Checking if there is an alternative. |
From the repo we got the code from in question: onepub-dev/system_info#12 |
@Knupper Would you be so kind and give us feedback for the fix/workaround? You can test it with a dependency override: dependency_overrides:
sentry_flutter:
git:
url: [email protected]:getsentry/sentry-dart.git
ref: fix/windows-paltform-memory |
@denrase IIUC the test runs 20 calls and the overhead of fetching memory info is ~3.3s, so one call takes ~167ms? that's awful 😅 I see GlobalMemoryStatusEx recommended and the win32 package also uses it - maybe a better alternative? not worth it adding win32 as dependency I think though |
@kahest Not quite, i update the sample to run more calls, but still significant when it should be close to no overhead. |
@denrase i was not able to override the dependency directly, as i get the following error:
then i cloned your repository and checkout your pull request branch and adjust the dependency override to go to the local version. then i was able to run it, but the performance was not better. as well i tried to implement the fix inside our main app, and there was no performance improvement as well. |
@Knupper can you check if this is an issue in older sentry flutter versions as well? |
@buenaflor with sentry_flutter: 8.0.0 and 8.1.0, 8.2.0, 8.3.0 the calls are much faster, there we have only a 4ms delay. after version 8.4.0 it takes much more time. i hope that helps? |
Yes, thank you! Helps us narrow it down |
What i found so far:
|
on desktop the frames tracking is disabled so this shouldn't be an issue there |
@denrase maybe what's interesting to check is if there is a difference if you use |
According to your changelog, it could be this change, as before there was no memory usage for windows. So i would agree with you, that it has to do with the memory allocation process. But i think caching is maybe not the best option if there are multiple requests that are startet at the same time, there will no benefit from caching. Add memory usage to contexts (#2133) The main question for me is, why does this async transaction from sentry delays the response from every call? On which time do you read the free memory? on transaction start or on transaction end? Maybe it could be a solution to move it to the finish as this has no impact on fetching the result? |
Yeah since this is an override of build in functionality, I'd assume that this is just the normal system calls to the method. |
I have now disabled memory collection on Windows/Linux per default. Even with caching, calling an external process even once was slow in my testing.
As for caching the platform memory, this is done in a class we hold throughout the lifecycle of the SDK. All transactions we process can access it.
We do processing/enriching of transactions after they are finished. Afterwards we send them to sentry. @Knupper would you be so kind and you try out how the performance impact is now with memory collection disabled by default? I can only test in a virtual machine, which is not really representative. 🙇 |
@denrase i was not able to test it with the performance app, as i get compile errors there. as i get following error: ../../../sentry-dart-main/flutter/lib/src/event_processor/flutter_enricher_event_processor.dart:47:14: Error: The setter 'runtimes' isn't defined for the class 'Contexts'.
i have checked out your branch and overrides it to my local version dependency_overrides: |
We have created a new PR where we remove free platform memory for now. #2798 |
We will release the fix soon in |
Platform
Flutter Desktop Windows
Obfuscation
Enabled
Debug Info
Enabled
Doctor
PS C:\Users\knupp\IdeaProjects\One.frontend> flutter doctor -v
[√] Flutter (Channel stable, 3.27.3, on Microsoft Windows [Version 10.0.22631.4830], locale de-DE)
• Flutter version 3.27.3 on channel stable at C:\development\flutter
• Upstream repository https://github.com/flutter/flutter.git
• Framework revision c519ee916e (5 weeks ago), 2025-01-21 10:32:23 -0800
• Engine revision e672b006cb
• Dart version 3.6.1
• DevTools version 2.40.2
[√] Windows Version (Installed version of Windows is version 10 or higher)
[√] Android toolchain - develop for Android devices (Android SDK version 35.0.1)
• Android SDK at C:\Users\knupp\AppData\Local\Android\Sdk
• Platform android-35, build-tools 35.0.1
• ANDROID_SDK_ROOT = C:\Users\knupp\AppData\Local\Android\Sdk
• Java binary at: C:\Program Files\Android\Android Studio1\jbr\bin\java
• Java version OpenJDK Runtime Environment (build 17.0.7+0-b2043.56-10550314)
• All Android licenses accepted.
[√] Chrome - develop for the web
• Chrome at C:\Program Files\Google\Chrome\Application\chrome.exe
[√] Visual Studio - develop Windows apps (Visual Studio Community 2022 17.12.5)
• Visual Studio at C:\Program Files\Microsoft Visual Studio\2022\Community
• Visual Studio Community 2022 version 17.12.35728.132
• Windows 10 SDK version 10.0.26100.0
[√] Android Studio (version 2023.1)
• Android Studio at C:\Program Files\Android\Android Studio1
• Flutter plugin can be installed from:
https://plugins.jetbrains.com/plugin/9212-flutter
• Dart plugin can be installed from:
https://plugins.jetbrains.com/plugin/6351-dart
• Java version OpenJDK Runtime Environment (build 17.0.7+0-b2043.56-10550314)
[!] Android Studio (version 4.1)
• Android Studio at C:\Program Files\Android\Android Studio
• Flutter plugin can be installed from:
https://plugins.jetbrains.com/plugin/9212-flutter
• Dart plugin can be installed from:
https://plugins.jetbrains.com/plugin/6351-dart
X Unable to determine bundled Java version.
• Try updating or re-installing Android Studio.
[√] IntelliJ IDEA Ultimate Edition (version 2024.3)
• IntelliJ at C:\Program Files\JetBrains\IntelliJ IDEA 2023.3.3
• Flutter plugin version 83.0.4
• Dart plugin version 243.23654.44
[√] Connected device (3 available)
• Windows (desktop) • windows • windows-x64 • Microsoft Windows [Version 10.0.22631.4830]
• Chrome (web) • chrome • web-javascript • Google Chrome 133.0.6943.142
• Edge (web) • edge • web-javascript • Microsoft Edge 132.0.2957.115
[√] Network resources
• All expected network resources are available.
! Doctor found issues in 1 category.
Version
8.13.0
Steps to Reproduce
Expected Result
The runtime of each call should be not longer then without the interceptor.
Actual Result
On Windows it takes much more time to complete all requests.
Here is a example app, that opens 200 connections to one service and meassures how long it takes that alle calls are completed. With the buttons we can decice if we want to use the interceptor with sentry or not.
Here are the results from windows (sampleRate: 1):
windows (SampleRate 0.2)
For mac:
For android:
For ios:
For web:
As we see the performance impact on all platforms is very small for enabling tracing. Only for windows it has a huge impact and that looks like a bug for me.
I hope you have enough information to reproduce it, if not let me know.
Minimal Example: https://github.com/Knupper/sentry-windows-mvp
Are you willing to submit a PR?
None
The text was updated successfully, but these errors were encountered: