-
-
Notifications
You must be signed in to change notification settings - Fork 458
[ANR] Reduced AndroidTransactionProfiler lock #4817
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
base: main
Are you sure you want to change the base?
Conversation
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d364ace | 382.77 ms | 443.21 ms | 60.44 ms |
| 3998a95 | 415.94 ms | 478.54 ms | 62.60 ms |
| 806307f | 357.85 ms | 424.64 ms | 66.79 ms |
| ee747ae | 357.79 ms | 421.84 ms | 64.05 ms |
| d217708 | 355.34 ms | 381.39 ms | 26.05 ms |
| ee747ae | 396.82 ms | 441.67 ms | 44.86 ms |
| 604a261 | 380.65 ms | 451.27 ms | 70.62 ms |
| 3699cd5 | 423.60 ms | 495.52 ms | 71.92 ms |
| b3d8889 | 371.84 ms | 447.49 ms | 75.65 ms |
| d5a29b6 | 298.62 ms | 391.78 ms | 93.16 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d364ace | 1.58 MiB | 2.11 MiB | 539.75 KiB |
| 3998a95 | 1.58 MiB | 2.10 MiB | 532.96 KiB |
| 806307f | 1.58 MiB | 2.10 MiB | 533.42 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| d217708 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| 604a261 | 1.58 MiB | 2.10 MiB | 533.42 KiB |
| 3699cd5 | 1.58 MiB | 2.10 MiB | 533.45 KiB |
| b3d8889 | 1.58 MiB | 2.10 MiB | 535.07 KiB |
| d5a29b6 | 1.58 MiB | 2.12 MiB | 549.37 KiB |
Previous results on branch: stefanosiano/enh/remove-profiling-lock
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 54a0e3d | 313.70 ms | 369.06 ms | 55.36 ms |
| 6329f7d | 302.88 ms | 357.80 ms | 54.92 ms |
| 7bf9f04 | 329.80 ms | 401.20 ms | 71.41 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 54a0e3d | 1.58 MiB | 2.12 MiB | 549.09 KiB |
| 6329f7d | 1.58 MiB | 2.11 MiB | 539.66 KiB |
| 7bf9f04 | 1.58 MiB | 2.12 MiB | 548.08 KiB |
sentry-android-core/src/main/java/io/sentry/android/core/AndroidProfiler.java
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java
Outdated
Show resolved
Hide resolved
| private int transactionsCounter = 0; | ||
| private final @NotNull AtomicBoolean isRunning = new AtomicBoolean(false); | ||
| private final @NotNull SentryFrameMetricsCollector frameMetricsCollector; | ||
| private @Nullable ProfilingTransactionData currentProfilingTransactionData; |
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 this be volatile?
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.
we access it only inside the lock, except in close() where we only read it to stop the profiler copying it into another variable
i guess it's fine to keep it as-is? wdyt?
| // When the first transaction is starting, we can start profiling | ||
| if (!isRunning.getAndSet(true)) { | ||
| // Let's initialize trace folder and profiling interval | ||
| init(); |
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 guess we still need to wrap profiler instantiation into a synchronized lock? And also where we're checking if (profiler == null) ?
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.
profiler instantiation is guarded by if (!isRunning.getAndSet(true)) {. Do you think it's not enough?
all other occurrences just read the variable, so volatile should be enough imho
mostly because we never nullify it, not even in close()
📜 Description
Removed lock from AndroidTransactionProfiler
💡 Motivation and Context
Trying to reduce ANRs. There are reports of ANRs happening when getting the lock on transaction start/end
resolves #4792
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps