Fixed
Status Update
No update yet.
Comments
ra...@google.com
ap...@google.com
Yigit, do you have time to fix it?
reemission of the same liveData is racy
reemission of the same liveData is racy
cc...@google.com
yea i'll take it.
ra...@google.com
Thanks for the detailed analysis. This may not be an issue anymore since we've started using Main.immediate there but I' not sure; I'll try to create a test case.
cc...@google.com
just emitting same live data reproduces the issue.
@Test
fun raceTest() {
val subLiveData = MutableLiveData(1)
val subject = liveData(testScope.coroutineContext) {
emitSource(subLiveData)
emitSource(subLiveData) //crashes
}
subject.addObserver().apply {
testScope.advanceUntilIdle()
}
}
@Test
fun raceTest() {
val subLiveData = MutableLiveData(1)
val subject = liveData(testScope.coroutineContext) {
emitSource(subLiveData)
emitSource(subLiveData) //crashes
}
subject.addObserver().apply {
testScope.advanceUntilIdle()
}
}
ap...@google.com
With 2.2.0-alpha04 (that use Main.immediate), the issue seems to be still there (I tested it by calling emitSource() twice, like your test case)
ap...@google.com
yea sorry immediate does not fix it.
I actually have a WIP fix for it:
https://android-review.googlesource.com/c/platform/frameworks/support/+/1112186
if your case is the one i found (emitting same LiveData multiple times, as shown in #5) you can work around it by adding a dummy transformation.
val subLiveData = MutableLiveData(1)
val subject = liveData(testScope.coroutineContext) {
emitSource(subLiveData.map {it })
emitSource(subLiveData.map {it} )
}
I actually have a WIP fix for it:
if your case is the one i found (emitting same LiveData multiple times, as shown in #5) you can work around it by adding a dummy transformation.
val subLiveData = MutableLiveData(1)
val subject = liveData(testScope.coroutineContext) {
emitSource(subLiveData.map {it })
emitSource(subLiveData.map {it} )
}
an...@google.com
Project: platform/frameworks/support
Branch: androidx-master-dev
commit af12e75e6b4110f48e44ca121466943909de8f06
Author: Yigit Boyar <yboyar@google.com>
Date: Tue Sep 03 12:58:11 2019
Fix coroutine livedata race condition
This CL fixes a bug in liveData builder where emitting same
LiveData source twice would make it crash because the second
emission registry could possibly happen before first one is
removed as source.
We fix it by using a suspending dispose function. It does feel
a bit hacky but we cannot make DisposableHandle.dispose async
and we do not want to block there. This does not mean that there
is a problem if developer disposes it manually since our emit
functions take care of making sure it disposes (and there is
no other way to add source to the underlying MediatorLiveData)
Bug: 140249349
Test: BuildLiveDataTest#raceTest_*
Change-Id: I0b464c242a583da4669af195cf2504e2adc4de40
M lifecycle/lifecycle-livedata-ktx/api/2.2.0-alpha05.txt
M lifecycle/lifecycle-livedata-ktx/api/current.txt
M lifecycle/lifecycle-livedata-ktx/api/public_plus_experimental_2.2.0-alpha05.txt
M lifecycle/lifecycle-livedata-ktx/api/public_plus_experimental_current.txt
M lifecycle/lifecycle-livedata-ktx/api/restricted_2.2.0-alpha05.txt
M lifecycle/lifecycle-livedata-ktx/api/restricted_current.txt
M lifecycle/lifecycle-livedata-ktx/src/main/java/androidx/lifecycle/CoroutineLiveData.kt
M lifecycle/lifecycle-livedata-ktx/src/test/java/androidx/lifecycle/BuildLiveDataTest.kt
https://android-review.googlesource.com/1112186
https://goto.google.com/android-sha1/af12e75e6b4110f48e44ca121466943909de8f06
Branch: androidx-master-dev
commit af12e75e6b4110f48e44ca121466943909de8f06
Author: Yigit Boyar <yboyar@google.com>
Date: Tue Sep 03 12:58:11 2019
Fix coroutine livedata race condition
This CL fixes a bug in liveData builder where emitting same
LiveData source twice would make it crash because the second
emission registry could possibly happen before first one is
removed as source.
We fix it by using a suspending dispose function. It does feel
a bit hacky but we cannot make DisposableHandle.dispose async
and we do not want to block there. This does not mean that there
is a problem if developer disposes it manually since our emit
functions take care of making sure it disposes (and there is
no other way to add source to the underlying MediatorLiveData)
Bug: 140249349
Test: BuildLiveDataTest#raceTest_*
Change-Id: I0b464c242a583da4669af195cf2504e2adc4de40
M lifecycle/lifecycle-livedata-ktx/api/2.2.0-alpha05.txt
M lifecycle/lifecycle-livedata-ktx/api/current.txt
M lifecycle/lifecycle-livedata-ktx/api/public_plus_experimental_2.2.0-alpha05.txt
M lifecycle/lifecycle-livedata-ktx/api/public_plus_experimental_current.txt
M lifecycle/lifecycle-livedata-ktx/api/restricted_2.2.0-alpha05.txt
M lifecycle/lifecycle-livedata-ktx/api/restricted_current.txt
M lifecycle/lifecycle-livedata-ktx/src/main/java/androidx/lifecycle/CoroutineLiveData.kt
M lifecycle/lifecycle-livedata-ktx/src/test/java/androidx/lifecycle/BuildLiveDataTest.kt
ap...@google.com
Project: platform/frameworks/support
Branch: androidx-main
commit b545c369ec7d8d6a019616c2dff67a4819c655c8
Author: Rahul Ravikumar <rahulrav@google.com>
Date: Wed Aug 23 13:24:01 2023
Make method tracing in Macrobenchmark more resilient.
* Use `--streaming`. Otherwise method traces stop at ~ the 1 sec mark.
* Use `--start-profiler` only when the process is not already alive.
* Tracing is always stopped at an iteration boundary to avoid confusion.
* Only iterations that involve `cold` starts are now accompanied with a method trace and therefore we avoid `0 byte` file captures.
Test: Tested locally.
Bug:
Change-Id: I66670cc981f371359767588c0374b1a909972b6c
M benchmark/benchmark-macro/src/androidTest/java/androidx/benchmark/macro/MacrobenchmarkScopeTest.kt
M benchmark/benchmark-macro/src/main/java/androidx/benchmark/macro/Macrobenchmark.kt
M benchmark/benchmark-macro/src/main/java/androidx/benchmark/macro/MacrobenchmarkScope.kt
https://android-review.googlesource.com/2724235
Branch: androidx-main
commit b545c369ec7d8d6a019616c2dff67a4819c655c8
Author: Rahul Ravikumar <rahulrav@google.com>
Date: Wed Aug 23 13:24:01 2023
Make method tracing in Macrobenchmark more resilient.
* Use `--streaming`. Otherwise method traces stop at ~ the 1 sec mark.
* Use `--start-profiler` only when the process is not already alive.
* Tracing is always stopped at an iteration boundary to avoid confusion.
* Only iterations that involve `cold` starts are now accompanied with a method trace and therefore we avoid `0 byte` file captures.
Test: Tested locally.
Bug:
Change-Id: I66670cc981f371359767588c0374b1a909972b6c
M benchmark/benchmark-macro/src/androidTest/java/androidx/benchmark/macro/MacrobenchmarkScopeTest.kt
M benchmark/benchmark-macro/src/main/java/androidx/benchmark/macro/Macrobenchmark.kt
M benchmark/benchmark-macro/src/main/java/androidx/benchmark/macro/MacrobenchmarkScope.kt
ra...@google.com
I landed this change to make this more consistent.
--
In general, MethodTracing when enabled captures the time between a cold start to the point where the iteration is complete.
We only turn on method tracing for iterations that involve cold starts. You will therefore not see a 1-1 mapping between the number of iterations and the number of method traces given you might be measuring HOT starts for e.g.
I fixed the issue where you were seeing 0 byte method trace files as well. Let me know if you have other questions.
cc...@google.com
I'm going to do one more pass on the behavior here to make it match micro more closely for the final 1.2 version:
- One method tracing pass at the end, after measurements, with measurements disabled (may want to still throw if no metrics captured, though)
- Document behavior surprises for micro vs macro
- Starts in first startActivityAndWait for now
- Add warning to output about why missing in living process case
ap...@google.com
Project: platform/frameworks/support
Branch: androidx-main
commit 98cd86539257998d458a86d2b689e3911687a4fd
Author: Chris Craik <ccraik@google.com>
Date: Mon Sep 11 15:50:57 2023
Fix method tracing file labels/paths
Test: TrivialStartupBenchmark # w/ method tracing enabled
Bug: 285912360
Relnote: "(Macrobenchmark) Clarified method tracing label in Studio
display, and fixed method tracing filenames to be unique on
device/host, so they won't be overwritten when more than one benchmark
is run."
Method tracing still has some issues, such as only capturing one
iteration when process isn't killed each iteration, and not running as
a separate phase, but those are larger changes, for a later release.
Also, removes unused MethodTracing.kt
Change-Id: I08e65d3fe8ab77614b5a3cbc241fb7901ba57135
M benchmark/benchmark-macro/src/androidTest/java/androidx/benchmark/macro/MacrobenchmarkScopeTest.kt
M benchmark/benchmark-macro/src/main/java/androidx/benchmark/macro/Macrobenchmark.kt
M benchmark/benchmark-macro/src/main/java/androidx/benchmark/macro/MacrobenchmarkScope.kt
D benchmark/benchmark-macro/src/main/java/androidx/benchmark/macro/MethodTracing.kt
https://android-review.googlesource.com/2747799
Branch: androidx-main
commit 98cd86539257998d458a86d2b689e3911687a4fd
Author: Chris Craik <ccraik@google.com>
Date: Mon Sep 11 15:50:57 2023
Fix method tracing file labels/paths
Test: TrivialStartupBenchmark # w/ method tracing enabled
Bug: 285912360
Relnote: "(Macrobenchmark) Clarified method tracing label in Studio
display, and fixed method tracing filenames to be unique on
device/host, so they won't be overwritten when more than one benchmark
is run."
Method tracing still has some issues, such as only capturing one
iteration when process isn't killed each iteration, and not running as
a separate phase, but those are larger changes, for a later release.
Also, removes unused MethodTracing.kt
Change-Id: I08e65d3fe8ab77614b5a3cbc241fb7901ba57135
M benchmark/benchmark-macro/src/androidTest/java/androidx/benchmark/macro/MacrobenchmarkScopeTest.kt
M benchmark/benchmark-macro/src/main/java/androidx/benchmark/macro/Macrobenchmark.kt
M benchmark/benchmark-macro/src/main/java/androidx/benchmark/macro/MacrobenchmarkScope.kt
D benchmark/benchmark-macro/src/main/java/androidx/benchmark/macro/MethodTracing.kt
cc...@google.com
With the above fix, macrobench method tracing is in a good enough/experimental state to ship with 1.2, so punting further work to 1.3
Remaining issues / gaps between method tracing in micro vs macro:
- Method trace should be done in separate measurement phase, more like microbench
- Method tracing can only start for an iteration if process is launched cold, so only one occurs frequently, e.g. when measuring warm start (CL in
#comment12 - Method trace not embedded into perfetto trace
ap...@google.com
Project: platform/frameworks/support
Branch: androidx-main
commit 7a0dc4138dd47a207c94dead3653e4db2c36b1d9
Author: Rahul Ravikumar <rahulrav@google.com>
Date: Mon Nov 06 15:12:10 2023
Use a consistent naming scheme for generated method traces in Macrobenchmarks.
Test: Updated existing tests.
Bug: 285912360
Change-Id: I5367d4fad0ec62a761ed9272340638d0020cfd75
M benchmark/benchmark-macro/src/androidTest/java/androidx/benchmark/macro/MacrobenchmarkScopeTest.kt
M benchmark/benchmark-macro/src/main/java/androidx/benchmark/macro/MacrobenchmarkScope.kt
https://android-review.googlesource.com/2816632
Branch: androidx-main
commit 7a0dc4138dd47a207c94dead3653e4db2c36b1d9
Author: Rahul Ravikumar <rahulrav@google.com>
Date: Mon Nov 06 15:12:10 2023
Use a consistent naming scheme for generated method traces in Macrobenchmarks.
Test: Updated existing tests.
Bug: 285912360
Change-Id: I5367d4fad0ec62a761ed9272340638d0020cfd75
M benchmark/benchmark-macro/src/androidTest/java/androidx/benchmark/macro/MacrobenchmarkScopeTest.kt
M benchmark/benchmark-macro/src/main/java/androidx/benchmark/macro/MacrobenchmarkScope.kt
cc...@google.com
bu...@google.com
Hi. I've received your bug and will wait until 2024-04-19 10:55 -0700 PDT and then assign the bug to rahulrav@google.com.
bu...@google.com
ra...@google.com
cc...@google.com
Last thing here should be to not capture metrics (or at least label metrics separately) for method tracing iterations.
ra...@google.com
ap...@google.com
Project: platform/frameworks/support
Branch: androidx-main
commit 2d1785e9837b97c6d64878413efb96d873895cd0
Author: Rahul Ravikumar <rahulrav@google.com>
Date: Wed Apr 24 16:57:02 2024
Move method tracing to a separate phase.
* Method tracing now runs as a separate phase, without interfering with metrics.
Fixes: 285912360
Fixes: 336588271
Test: Existing unit tests.
Change-Id: If9a504a7bce7228840339b34294ba3fdf98aceeb
Relnote: Method tracing runs as a seperate phase during a Macrobenchmark, and it no longer affects measurements.
M benchmark/benchmark-macro/src/main/java/androidx/benchmark/macro/Macrobenchmark.kt
A benchmark/benchmark-macro/src/main/java/androidx/benchmark/macro/MacrobenchmarkPhase.kt
https://android-review.googlesource.com/3059411
Branch: androidx-main
commit 2d1785e9837b97c6d64878413efb96d873895cd0
Author: Rahul Ravikumar <rahulrav@google.com>
Date: Wed Apr 24 16:57:02 2024
Move method tracing to a separate phase.
* Method tracing now runs as a separate phase, without interfering with metrics.
Fixes: 285912360
Fixes: 336588271
Test: Existing unit tests.
Change-Id: If9a504a7bce7228840339b34294ba3fdf98aceeb
Relnote: Method tracing runs as a seperate phase during a Macrobenchmark, and it no longer affects measurements.
M benchmark/benchmark-macro/src/main/java/androidx/benchmark/macro/Macrobenchmark.kt
A benchmark/benchmark-macro/src/main/java/androidx/benchmark/macro/MacrobenchmarkPhase.kt
pr...@google.com
The following release(s) address this bug.It is possible this bug has only been partially addressed:
androidx.benchmark:benchmark-macro:1.3.0-alpha05
Description
Filing a bug to track (late) comments on macrobench tracing CL:https://android-review.git.corp.google.com/c/platform/frameworks/support/+/2594692
Can punt this out of 1.2, but put there for minimal fix of config options