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 c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

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

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

Also, fix public docs for PerformanceMetricsState.Companion (and elsewhere?) which still refer to the now-renamed PerformanceMetricsStateHolder object (now just "Holder")

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

ok, had a think, here's what we'll do:
- getForHierarchy() will be renamed to getHolderForHierarchy() since Holder, as you point out, is what it returns
- addState() will be changed to putState(). This should be closer to what's actually happening, since internally the structure acts like a map. if you call addState("A", "1") then addState("A", "2"), there will only (on a given frame) be either "1" or "2" for A, not both. So now you would call putState(...)
- stateName and stateValue are indeed confusing, and a bit redundant, since you are already setting them on a state object. key/value are more helpful (and are, in fact, what they are), so I will change it to be key/value (with no 'state' at all, since that is implicit by being in the 'state' object to begin with).

Meanwhile, I think the coupling is looser than you are imagining. You can call Holder.state and it will be non-null if it was gotten on a valid view hierarchy. this was intentional because you should be able to put/remove state at any time, whether or not that state is being tracked when you do that operation.
So I believe the current API and functionality here is correct, let me know if I'm missing something here.
Meanwhile, I'll make the other fixes above.

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