Several issues in PerformanceMetricsState [213499234]

Fixed

Bug

Status Update

No update yet.

Description

cc...@google.com

created issue #1

Jan 6, 2022 10:27PM

PerformanceMetricsStates doesn't appear to support arbitrarily long delays between writing and reading.

While it holds time ranges for for each state, that's isn't sufficient if, for example, there's a 3 frame delay for (not impossible for a frame that gets blocked on GPU composition, for example). You could have frames 1,2,3 where 1 & 3 have a single frame state key/value, but not 2.

Additionally, a couple other problems:

PerformanceMetricsStates#getIntervalStates uses nanoTime (via markSingleFrameStateForRemoval) to affect single frame states, which seems incorrect. It should be only using data from the times passed in.

It should instead be driven by the values passed in getIntervalStates(), since the time getIntervalStates is called may be some arbitrary point in the future. The behavior for single frame states should work even if they're not

getIntervalStates is destructive, so multiple JankStats instances will interfere (only the first will get single frame states). Instead, consider separating into two apis:

getIntervalStates() (as is, but just a getter, non destructive)
discardDataBeforeTimestamp() (discards data that happened before the end UI thread timestamp of last frame)

Comments

ch...@google.com <ch...@google.com> #2Jan 6, 2022 10:43PM

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> #3Jan 13, 2022 11:35PM

Going with the one-per-window approach.
There's not an easy way to query existing listeners on a Window, so I'll use a view tag on DecorView to stash the single OnFrameMetricsListener, as well as the list of delegate listeners that it is sending out events to.

ap...@google.com <ap...@google.com> #4Jan 24, 2022 05:20PM

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

commit 4ed809aeb388ef6c5ac327c1328724e2053a2bbe
Author: Chet Haase <chet@google.com>
Date: Thu Jan 13 15:54:46 2022

Fix problem with multiple JankStats per window

Previously, creating multiple instances of JankStats on a particular
window would potentially create multiple OnFrameMetricsAvailableListener
objects on that window, causing both unnecessary overhead as well as
an issue where one Jankstats instance exclusively consumes data that
should be passed to multiple listeners (SingleFrameStates).

This fix creates only one listener per window, along with a list
of listeners that it delegates to. Creating a new JankStats instance
will only create/add the core window listener if none yet exists.
Otherwise, it will simply add its listener to the delegate list, which
will be called by the single listener when it is called by Window.

A similar change was made to the JankStatsApi16Impl to have a single
OnPreDrawListener which delegates to multiple instances (if they exist).

Test: ran manual integration tests and added new automated test
Bug: 213499234
Change-Id: I72b7e703bdd476b8bd8fc32f73d6932b9086478a

M metrics/metrics-performance/src/main/java/androidx/metrics/performance/JankStatsApi24Impl.kt
M metrics/metrics-performance/src/main/java/androidx/metrics/performance/JankStatsApi22Impl.kt
M metrics/metrics-performance/src/main/java/androidx/metrics/performance/JankStats.kt
M metrics/metrics-performance/src/main/java/androidx/metrics/performance/JankStatsApi16Impl.kt
M metrics/metrics-performance/src/main/java/androidx/metrics/performance/PerformanceMetricsState.kt
M metrics/metrics-performance/src/androidTest/java/androidx/metrics/performance/test/JankStatsTest.kt
M metrics/metrics-performance/src/main/res/values/ids.xml
M metrics/metrics-performance/src/androidTest/java/androidx/metrics/performance/test/DelayedActivity.kt
M metrics/metrics-performance/src/androidTest/java/androidx/metrics/performance/test/DelayedView.kt

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

ch...@google.com <ch...@google.com> #5Jan 24, 2022 05:24PM

alpha problem (multiple instances) fixed, leaving bug open for other issues raised in #1

ch...@google.com <ch...@google.com> #6Mar 15, 2022 05:15PM

Specifically, these comments from #1 still need to be addressed:

1) PerformanceMetricsStates doesn't appear to support arbitrarily long delays between writing and reading.
While it holds time ranges for for each state, that's isn't sufficient if, for example, there's a 3 frame delay for (not impossible for a frame that gets blocked on GPU composition, for example). You could have frames 1,2,3 where 1 & 3 have a single frame state key/value, but not 2.
2) PerformanceMetricsStates#getIntervalStates uses nanoTime (via markSingleFrameStateForRemoval) to affect single frame states, which seems incorrect. It should be only using data from the times passed in.
It should instead be driven by the values passed in getIntervalStates(), since the time getIntervalStates is called may be some arbitrary point in the future. The behavior for single frame states should work even if they're not

For (2), that function is no longer called internally (and should be removed). The only other place that single frame states are removed is during a call to addSingleFrameState(). We use nanoTime there, because that is when the caller called the function, so it's the best time to use. That same time is used for both adding the state and removing any previous state, so it is consistent.
So I think the 'fix' here is just to remove markSingleFrameStateForRemoval(); the other logic seems okay.

For (1) I'm going to need clarification to understand the problem and proposed solution. When addSingleFrameState() is called, we log the time that it was added (and, later, when it is marked for removal). In processing a frame in the future, we check whether that state existed (was added but not yet marked for removal) within the timeframe of that frame. It's not clear what the problem is that's being described; whether there are delays in the system or not, we're still going to process/report single frame states exactly once, and only for frames which are in the correct envelope of time for those states.

ap...@google.com <ap...@google.com> #7Mar 15, 2022 08:28PM

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

commit 8aad27001a81138ac1aea014cc22793fbb976a6b
Author: Chet Haase <chet@google.com>
Date: Tue Mar 15 10:16:02 2022

Remove obsolete function for processing single frame states

markSingleFrameStateForRemoval() was historical and no longer used.
Removing to clean up the code and clarify the logic around the timing
of state removal.

Bug: 213499234
Test: JankStatsTest tests all pass
Change-Id: I8ffff839ed767ed564505962b2eeb0494be79396

M metrics/metrics-performance/src/main/java/androidx/metrics/performance/PerformanceMetricsState.kt

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

ch...@google.com <ch...@google.com> #8Mar 15, 2022 08:29PM

Reassigned to cc...@google.com.

CL in #7 fixes issue (2) in

comment #6

.
For issue (1) in #6, passing back to submitter for more info.

cc...@google.com <cc...@google.com> #9Mar 22, 2022 05:21PM

Reassigned to ch...@google.com.

Imagine you have the following events in order:

t=100 set state A=B
t=110 frame 1 occurs
t=120 set state A=C
t=130 set state A=D
t=140 frame 2 occurs
t=150 set state A=E
t=160 frame 3 occurs
t=170 callback from FrameMetricsListener fires with data from frame 1
t=180 callback from FrameMetricsListener fires with data from frame 2
t=190 callback from FrameMetricsListener fires with data from frame 3

Whether these are single frame states or not, you should be sending the following events:

frame1 = state{A=B}
frame2 = state{A=D} // C was overwritten
frame3 = state{A=E}

Currently though, you don't have enough data to reconstruct this information when the callbacks start firing. You're tracking add/removed for each state, but thats not enough to remember old values, once the values have changed. You want an event queue, something like the following:

[
   set state A=B (ts = 100)
   set state A=C (ts = 120)
   set state A=D (ts = 130)
   set state A=E (ts = 150)
]

Then, when the data for frame 1 occurs you know end of that frame is ts110, so it sends out frame1 = state{A=B} and drop the first item in the queue (take current data, and apply/drop recorded events up until ts=110).

When frame 2 occurs, 2nd and 3rd events are applied, so `frame2 = state{A=D} since C is overridden.

This concept can extend to add/remove state (for persistent state) and single frame states. With that queue, and the current callback state (most recent state issued to callback) you basically have two ops:

on add/remove/setFrameState (ui thread):
    synchronized(eventQueue) {
        eventQueue.push(event(..., System.nanoTime()))
    }

on framemetrics listener (handler thread):
    val eventsToApply = synchronized(eventQueue) {
        eventQueue.removeUntil(frame.end)
    }
    currentState = currentState.applyAll(eventsToApply)
    dispatchCallback(currentState, frame.end)

ap...@google.com <ap...@google.com> #10May 9, 2022 10:16PM

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

commit be9c1aa2cfb1295dbe2dc852f31f8a1e7ab39168
Author: Chet Haase <chet@google.com>
Date: Mon Apr 11 17:10:50 2022

Fixing frame timing issue, plus minor API change

Previously, state setting could potentially clobber existing states
which would have been valid for frames currently being processed.
The new approach stores all states without replacing existing ones
until they are done being processed.
More complex tests were added to ensure that these situations work.

Also, minor API change to switch MetricsStateHolder to simply Holder,
since it is already inside the PerformanceMetricsState class/namespace.

Bug: 226565716, 213499234
Test: Added new TestComplexFrameStateData, tested on API 32, 24, 16
Relnote: Previous Holder name was too verbose and unnecessary

Change-Id: I5a4d9095520399a146e6fd78eb50c86a7051738b

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/JankStats.kt
M metrics/metrics-performance/src/androidTest/java/androidx/metrics/performance/test/JankStatsTest.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/MessageListFragment.kt
M metrics/metrics-performance/src/androidTest/java/androidx/metrics/performance/test/DelayedActivity.kt
M metrics/metrics-performance/src/androidTest/java/androidx/metrics/performance/test/DelayedView.kt
M metrics/metrics-performance/api/current.txt

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

ch...@google.com <ch...@google.com> May 9, 2022 10:21PM

Marked as fixed.