Fixed
Status Update
Comments
li...@pinterest.com <li...@pinterest.com> #2
Project: platform/frameworks/support
Branch: androidx-master-dev
commit b90079595f33f58fece04026a97faa0d243acdb1
Author: Yuichi Araki <yaraki@google.com>
Date: Wed Sep 18 16:55:49 2019
Change the way to detect mismatch between POJO and query
This fixes cursor mismatch warnings with expandProjection.
Bug: 140759491
Test: QueryMethodProcessorTest
Change-Id: I7659002e5e0d1ef60fc1af2a625c0c36da0664d8
M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt
M room/compiler/src/test/kotlin/androidx/room/processor/QueryMethodProcessorTest.kt
M room/compiler/src/test/kotlin/androidx/room/testing/TestProcessor.kt
https://android-review.googlesource.com/1123258
https://goto.google.com/android-sha1/b90079595f33f58fece04026a97faa0d243acdb1
Branch: androidx-master-dev
commit b90079595f33f58fece04026a97faa0d243acdb1
Author: Yuichi Araki <yaraki@google.com>
Date: Wed Sep 18 16:55:49 2019
Change the way to detect mismatch between POJO and query
This fixes cursor mismatch warnings with expandProjection.
Bug: 140759491
Test: QueryMethodProcessorTest
Change-Id: I7659002e5e0d1ef60fc1af2a625c0c36da0664d8
M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt
M room/compiler/src/test/kotlin/androidx/room/processor/QueryMethodProcessorTest.kt
M room/compiler/src/test/kotlin/androidx/room/testing/TestProcessor.kt
ch...@google.com <ch...@google.com> #3
ch...@google.com <ch...@google.com> #4
Project: platform/frameworks/support
Branch: androidx-master-dev
commit bdde5a1a970ddc9007b28de4aa29d60ffa588f08
Author: Yigit Boyar <yboyar@google.com>
Date: Thu Apr 16 16:47:05 2020
Re-factor how errors are dismissed when query is re-written
This CL changes how we handle errors/warnings if query is
re-written.
There was a bug in expandProjection where we would report warnings
for things that Room already fixes automatically ( b/140759491 ).
The solution to that problem (I7659002e5e0d1ef60fc1af2a625c0c36da0664d8)
solved it by deferring validating of columns until after re-write
decision is made. Unfortunately, this required changing PojoRowAdapter
to have a dummy mapping until it is validating, make it hard to use
as it does have a non-null mapping which is not useful.
This CL partially reverts that change and instead rely on the log
deferring logic we have in Context. This way, we don't need to break
the stability of PojoRowAdapter while still having the ability to
drop warnings that room fixes. This will also play nicer when we
have different query re-writing options that can use more information
about the query results.
Bug: 153387066
Bug: 140759491
Test: existing tests pass
Change-Id: I2ec967c763d33d7a3ff02c1a13c6953b460d1e5f
M room/compiler/src/main/kotlin/androidx/room/log/RLog.kt
M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt
https://android-review.googlesource.com/1288456
Branch: androidx-master-dev
commit bdde5a1a970ddc9007b28de4aa29d60ffa588f08
Author: Yigit Boyar <yboyar@google.com>
Date: Thu Apr 16 16:47:05 2020
Re-factor how errors are dismissed when query is re-written
This CL changes how we handle errors/warnings if query is
re-written.
There was a bug in expandProjection where we would report warnings
for things that Room already fixes automatically (
The solution to that problem (I7659002e5e0d1ef60fc1af2a625c0c36da0664d8)
solved it by deferring validating of columns until after re-write
decision is made. Unfortunately, this required changing PojoRowAdapter
to have a dummy mapping until it is validating, make it hard to use
as it does have a non-null mapping which is not useful.
This CL partially reverts that change and instead rely on the log
deferring logic we have in Context. This way, we don't need to break
the stability of PojoRowAdapter while still having the ability to
drop warnings that room fixes. This will also play nicer when we
have different query re-writing options that can use more information
about the query results.
Bug: 153387066
Bug: 140759491
Test: existing tests pass
Change-Id: I2ec967c763d33d7a3ff02c1a13c6953b460d1e5f
M room/compiler/src/main/kotlin/androidx/room/log/RLog.kt
M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt
ap...@google.com <ap...@google.com> #5
Project: platform/frameworks/support
Branch: androidx-main
commit 1de6215c6bd9e887e3d94556e9ac55cfb7b8c797
Author: Chet Haase <chet@google.com>
Date: Fri Aug 26 16:56:05 2022
Fix cpuDuration calculation and expose totalDuration
A missing metric (the time taken to queue the data sync to RenderThread)
in FrameMetrics made the cpuDuration data in JankStats less accurate than
it could be, and made it harder to determine the equivalent of
"totalDuration" in comparison to data one could get from FrameMetrics.
Further complicating the problem was the overlap of SWAP duration
in the calculation of both GPU and CPU durations on API31, making it
even trickier to align all of these similar-yet-different metrics.
There was a request (see the bug) to expose totalDuration for JankStats
overall, but this is problematic since it is not possible to expose it
consistently across different releases (on 31 it includes GPU, on 24 it
does not, and on 16 it has even less data behind it). Instead of
exposing it overall, we now expose it just in API31, in the
FrameDataApi31 structure, where iut has all of the data it needs to
provide an accurate measure of total duration.
Bug: 243694893
Test: Ran JankStats on API24 and 31
RelNote: cpuDuration now more accurate, also new totalDuration on API31
Change-Id: I59ce8c67f06a168f96893375c8aeca5516a55d81
M metrics/metrics-performance/src/main/java/androidx/metrics/performance/JankStatsApi31Impl.kt
M metrics/metrics-performance/api/public_plus_experimental_current.txt
M metrics/metrics-performance/api/restricted_current.txt
M metrics/metrics-performance/src/main/java/androidx/metrics/performance/FrameDataApi31.kt
M metrics/metrics-performance/src/main/java/androidx/metrics/performance/JankStatsApi24Impl.kt
M metrics/metrics-performance/src/androidTest/java/androidx/metrics/performance/test/JankStatsTest.kt
M metrics/metrics-performance/api/current.txt
https://android-review.googlesource.com/2199726
Branch: androidx-main
commit 1de6215c6bd9e887e3d94556e9ac55cfb7b8c797
Author: Chet Haase <chet@google.com>
Date: Fri Aug 26 16:56:05 2022
Fix cpuDuration calculation and expose totalDuration
A missing metric (the time taken to queue the data sync to RenderThread)
in FrameMetrics made the cpuDuration data in JankStats less accurate than
it could be, and made it harder to determine the equivalent of
"totalDuration" in comparison to data one could get from FrameMetrics.
Further complicating the problem was the overlap of SWAP duration
in the calculation of both GPU and CPU durations on API31, making it
even trickier to align all of these similar-yet-different metrics.
There was a request (see the bug) to expose totalDuration for JankStats
overall, but this is problematic since it is not possible to expose it
consistently across different releases (on 31 it includes GPU, on 24 it
does not, and on 16 it has even less data behind it). Instead of
exposing it overall, we now expose it just in API31, in the
FrameDataApi31 structure, where iut has all of the data it needs to
provide an accurate measure of total duration.
Bug: 243694893
Test: Ran JankStats on API24 and 31
RelNote: cpuDuration now more accurate, also new totalDuration on API31
Change-Id: I59ce8c67f06a168f96893375c8aeca5516a55d81
M metrics/metrics-performance/src/main/java/androidx/metrics/performance/JankStatsApi31Impl.kt
M metrics/metrics-performance/api/public_plus_experimental_current.txt
M metrics/metrics-performance/api/restricted_current.txt
M metrics/metrics-performance/src/main/java/androidx/metrics/performance/FrameDataApi31.kt
M metrics/metrics-performance/src/main/java/androidx/metrics/performance/JankStatsApi24Impl.kt
M metrics/metrics-performance/src/androidTest/java/androidx/metrics/performance/test/JankStatsTest.kt
M metrics/metrics-performance/api/current.txt
ch...@google.com <ch...@google.com>
li...@pinterest.com <li...@pinterest.com> #6
Thanks for the quick fix!!! We believe the take away is the following:
> use frameDurationCpuNanos to replace FrameMetrics.TOTAL_DURATION on API versions 24-30, then use frameDurationTotalNanos on version 31+
I do have a follow-up question: while looking at the following trace (https://developer.android.com/static/studio/images/profile/jank_detection-frame-lifecycle-threads.png ), I am wondering if "the sheer delta between draws in main thread" is a good rough representation of how much the user perceived as a frame's rendering time or the delta still misses some execution time on the render thread in parallel to the main thread.
> use frameDurationCpuNanos to replace FrameMetrics.TOTAL_DURATION on API versions 24-30, then use frameDurationTotalNanos on version 31+
I do have a follow-up question: while looking at the following trace (
ch...@google.com <ch...@google.com> #7
I don't know off-hand from looking at that trace (since it's just a big blank area), but yeah, we don't have visibility pre-FrameMetrics in where the time was going. The perceived time between draws is still a decent overall metric, because that will end up being visible to the user (jank == longer delays between draws). So it's not like a huge delay in GPU rendering (for example) would end up being masked by the fact that we couldn't detect it due to the lack of FrameMetrics data. I'm not certain it would show up on that exact frame, however; I wonder if some GPU delays would show up on later frames, when that later frame went to send data to the GPU and had to wait to send it down, or was able to send it, but it just queued up waiting for the GPU to finish doing all of those earlier operations. Regardless, it's all decent data (I believe) in terms of knowing when there are general frame performance problems, it's just a little harder to diagnose where in the pipeline those problems occurred.
li...@pinterest.com <li...@pinterest.com> #8
That's great to hear since we are taking a high level experimental approach to test frame render optimizations or investigate regressions. We often look at the aggregated frame drop or render data between the experimental groups. The "draw interval" measurement so far fits our needs and is indeed quite sensitive to the changes too.
na...@google.com <na...@google.com> #9
The following release(s) address this bug.It is possible this bug has only been partially addressed:
androidx.metrics:metrics-performance:1.0.0-alpha04
Description
androidx.metrics
module used: androidx.metrics:metrics-performanceandroidx.metrics
version used: 1.0.0-alpha02 Devices/Android versions reproduced on: All versionsWhen you encountered this bug:
Request for including total frame duration field in
FrameData
for API 24.It would have been helpful if JankStats library could provide total frame duration (for API >= 24). We were trying to replace our legacy implementation with the JankStats library. In our legacy implementation, we use total frame duration; but the data class
FrameData
does not contain total frame duration. Providing total frame duration for API 24 & above would give more flexibility to use this library (if someone wants to migrate from the legacy implementation to the JankStats).More Details: Could you include a field (totalFrameDuration) in the next alpha release?
It would be great if you could include the total frame duration as one of the data class values.
I saw you included
frameOverrunNanos
(which is derived from total frame duration) for API 31