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

The issue is reproducible with core-ktx 1.2.0 and 1.3.0-rc01.

ch...@google.com <ch...@google.com> #3Jan 13, 2022 11:35PM

The Typeface.weight is not a weight of the underlying font file. It is a display style. On older APIs, the display style is adjusted if the Typeface is created from single font. However, after moving to CustomFallbackBuilder, that adjustment is removed since it can crate Typeface from multiple style font files.

Looks like it is good to set display style by ResourcesCompat.getFont for backward compatibility.

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

Hi Nona,

Can you please schedule a release after you merge the fix?

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

Project: platform/frameworks/support
Branch: androidx-master-dev

commit 3d6aa2e9b3243dcc4de1f54bd8d40339bd69cb05
Author: Seigo Nonaka <nona@google.com>
Date: Wed May 27 17:38:05 2020

Adjust the Typeface display style with the style of given font

This behavir is implicitly done by Typeface.Builder and
Typeface.createXXX function but not to be done by
Typeface.CustomFallbackBuilder since it is designed to be working
with multiple font files which has different style.

Looks like the style argument is ignored on older API implementation.

Bug: 156853883
Bug: 152023266
Test: ResourcesCompatTest#testGetFont_adjustDisplayStyle passes on 29
Test: ./gradlew core:core:connectedAndroidTest on API 29, 28, 23
Change-Id: I3a377c339a7aed50973cf11df86ddf0069f4ec25

A core/core/src/androidTest/assets/fonts/thin_italic.ttf
A core/core/src/androidTest/assets/fonts/thin_italic.ttx
M core/core/src/androidTest/java/androidx/core/content/res/ResourcesCompatTest.java
A core/core/src/androidTest/res/font/thin_italic.ttf
M core/core/src/main/java/androidx/core/graphics/TypefaceCompatApi29Impl.java

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

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

The following changes were cherrypicked through Coastguard Release Track: (https://android-build.googleplex.com/coastguard/dashboard/6222780589080576/#/history/all) Changes: aosp/1318947

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

Any way I can tell what version this will land in?

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

Reassigned to cc...@google.com.

should be in 1.5.0-alpha1 https://developer.android.com/jetpack/androidx/releases/core#1.5.0-alpha01

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

Reassigned to ch...@google.com.

Great—works as expected, thanks!

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.