PerformanceMetricsState.getForHierarchy API is a bit weird [233421985]

Fixed

Bug

Status Update

No update yet.

Description

py...@gmail.com

created issue #1

May 21, 2022 10:05PM

PerformanceMetricsState.getForHierarchy returns a MetricsStateHolder which is just a wrapper around PerformanceMetricsState?.

As I started using it, I did this:

// Named it "state" because I'm calling "get" on something a class which as a name ending in State.
val state = PerformanceMetricsState.getForHierarchy(view)
state.state?.let { state ->
 state.addState("foo", "bar")
}

So.. I'm getting a state, then getting a state from it or maybe not, and if I do have a state then I can add a state to it!

The other thing that's a bit weird about this is how PerformanceMetricsState is a separate class but it's directly coupled to JankState, i.e. MetricsStateHolder.state will always be set if I already called JankState.createAndTrack() for that window, but it'll be null if I didn't. This conditional nullability isn't super clear.

Suggestions:

Consider hiding MetricsStateHolder and having getForHierarchy directly return PerformanceMetricsState?
Consider a name changes so that we're not adding "state" (addState) to an object already named "state" (PerformanceMetricsState).
Consider either breaking the coupling entirely or making the coupling more explicit. Breaking the coupling would mean deleting the PerformanceMetricsState.create() function that JankStat is calling, and having MetricsStateHolder.getForHierarchy() automatically create it on first call. Making the coupling more explicit might mean making a PerformanceMetricsState instance directly available on a JankState instance.

Comments

py...@gmail.com <py...@gmail.com> #2May 21, 2022 10:15PM

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

ch...@google.com <ch...@google.com> #3Jul 11, 2022 10:15PM

The following changes were cherrypicked through Coastguard (

https://android-build.googleplex.com/coastguard/)
Release Track: (

https://android-build.googleplex.com/coastguard/dashboard/6093116828680192/#/cherrypickHistory/all)
Created: 2019-10-14 17:23:42.610596+00:00
Changes: aosp/1123258

ch...@google.com <ch...@google.com> #4Jul 11, 2022 10:56PM

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

ch...@google.com <ch...@google.com> #5Jul 11, 2022 11:47PM

Correction - you were right on the coupling between the state and stats objects. From the class docs for PerformanceMetricsState:
* Note that the function will not create a PerformanceMetricsState object if the
* Holder's `state` is null; that object is created when a [JankStats]
* object is created. This is done to avoid recording performance state if it is
* not being tracked.

However, I'm inclined to leave this loose-but-not coupling as-is. The intention is to enable any caller to get the state holder and popular it if appropriate, but not to bother wasting space and time doing so if nobody is tracking that data.
For example, we might add code to AndroidX libraries to populate state for JankStats in general. But it only makes sense doing that if someone wants that data, otherwise we are wasting time, allocations, and space on data that is unnecessary. So it makes sense to have the loose coupling that allows, say, RecyclerView to get the state object for the hierarchy and call (if state?) addState() with relevant data. But it also makes sense to have that state object only exist and be valid if someone is listening on the other end.

ap...@google.com <ap...@google.com> #6Jul 13, 2022 08:24PM

Project: platform/frameworks/support
Branch: androidx-main

commit 9a8c7dd45518e4f8c8e04776c0b7487de62ce0dc
Author: Chet Haase <chet@google.com>
Date: Mon Jul 11 17:24:18 2022

Minor changes in PerformanceMetricsState APIs for clarity

There was some confusion in some method and parameter names
in PerformanceMetricsState which have been changed for clarity.
For example, stateName/state were being set on a 'state' object,
which seemed redundant. Since they are acting as a key/value pair,
they have been changed to simply key and value. Also, getForHierarchy()
on PerformanceMetricsState returned a Holder object, not (as one might
assume) a State object, so the method name was updated to
getHolderForHierarchy(). Finally, add[SingleFrame]State was actually
replacing state, acting more like a Map than a List or Set, so the method
names were updated to 'put' instead.

Bug: 233421985
Test: Ran JankStatsTest, benchmark, unit test apps
RelNote: "Updated method and parameter names in PerformanceMetricsState
to make the results of those calls clearer."

Change-Id: I56da57b13818bf4077a64ab144222ce255f4539a

M metrics/metrics-performance/api/public_plus_experimental_current.txt
M metrics/metrics-performance/src/main/java/androidx/metrics/performance/FrameData.kt
M metrics/metrics-performance/api/restricted_current.txt
M metrics/metrics-performance/src/main/java/androidx/metrics/performance/JankStatsApi24Impl.kt
M metrics/metrics-performance/src/main/java/androidx/metrics/performance/JankStatsApi16Impl.kt
M metrics/metrics-performance/src/androidTest/java/androidx/metrics/performance/test/JankStatsTest.kt
M metrics/integration-tests/janktest/src/main/java/androidx/metrics/performance/janktest/JankAggregatorActivity.kt
M metrics/integration-tests/janktest/src/main/java/androidx/metrics/performance/janktest/MessageListFragment.kt
M metrics/integration-tests/janktest/src/main/java/androidx/metrics/performance/janktest/JankLoggingActivity.kt
M metrics/metrics-performance/src/androidTest/java/androidx/metrics/performance/test/DelayedView.kt
M metrics/metrics-benchmark/src/androidTest/java/androidx/metrics/performance/benchmark/JankStatsBenchmark.kt
M metrics/metrics-performance/src/main/java/androidx/metrics/performance/FrameDataApi31.kt
M metrics/metrics-performance/src/main/java/androidx/metrics/performance/PerformanceMetricsState.kt
M metrics/integration-tests/janktest/src/main/java/androidx/metrics/performance/janktest/SimpleLoggingActivity.kt
M metrics/metrics-performance/src/main/java/androidx/metrics/performance/FrameDataApi24.kt
M metrics/metrics-performance/api/current.txt

https://android-review.googlesource.com/2150522

ch...@google.com <ch...@google.com> Jul 13, 2022 09:21PM

Marked as fixed.

py...@gmail.com <py...@gmail.com> #7Jul 14, 2022 08:44AM

Thanks for the fixes!

I think the loose-but-not coupling is fine, it's just that I'm assuming at some point you might want to release other jetpack perf metric libs and then you'll have more places that care about tracking state. So maybe at some point there should be a "this window should track state" API.. and these pieces can be released as a tiny standalone lib. but maybe YAGNI. I'm just wondering if I'd be interested in leveraging this as well in 3rd party libs.

Regarding the holder, I'm still confused why we need to call PerformanceMetricsState.getHolderForHierarchy(view)?.state, i.e. the holder isn't useful ever from a public API standpoint, so I why not have a PerformanceMetricsState.getForHierarchy(view) that returns PerformanceMetricsState? directly and keep the Holder class internal?

ch...@google.com <ch...@google.com> #8Jul 14, 2022 03:59PM

At this point (through 1.0) I'd rather keep the API very minimal and add stuff later as it makes sense. I consider this version as the fundamental-plumbing release, with more capabilities added later, on top, as they make sense.
As for the Holder, it is possible that it will become null later, if the app stops tracking. The reason for the Holder is to have relevant information at the time when you want to inject state, not to cache something that might be obsolete later. Or, for another example, it may be initially null when you first get the Holder, but the next time that code is run, there is an attached JankStats, thus it is active. If you had cached the initial result, then you might never populate state, whereas if you get it at the time when you should inject it, you know whether someone is listening or not.

Message last modified on Jul 14, 2022 04:00PM