TraceSectionMetric optional custom name [301892382]

Fixed

Feature Request

Status Update

No update yet.

Description

ml...@google.com

created issue #1

Sep 25, 2023 07:58AM

TraceSectionMetric reports name as the sectionName + Ms or Count (in case of Mode.Sum).

Sometimes this might be super long and breaks standard horizontal size:

JIT compiling %com.google.composeviewbenchmarks.target%Ms      min      30.7,   median      97.7,   max     204.1

What do you think of adding optional metricName parameter to reduce this and give developers the option to define the name.

Pros:

shorter names easily glanceable
if we're improving metrics as in b/300887146 you can define the names yourself

Potential issues:

conflicting names
renaming something and forgetting what it was in reality

Comments

cc...@google.com <cc...@google.com> #2Sep 26, 2023 04:34PM

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

ml...@google.com <ml...@google.com> #3Sep 27, 2023 08:22AM

Yeah, probably yes. Or even simpler in this case, have the Mode not being an enum, but a interface where you'd pass the slices and it would calculate it? And even combine multiple ones so you don't have to write multiple TraceSectionMetric?

interface Mode {

  fun getMeasurements(slices: List<Slice>) : List<Measurement>

}

And we have the standard ones - first, sum (max), and it's easy to calculate it yourself.

object ModeMax : TraceSectionMetric.Mode {

  override fun getMeasurements(slices: List<Slice>) : List<Measurement> {
    val max = slices.maxByOrNull { it.dur } ?: return emptyList()

    return listOf(Measurement(name = sectionName + "MaxMs", data = max.dur / 1_000_000.0))
  }

}

Message last modified on Sep 27, 2023 08:31AM

ml...@google.com <ml...@google.com> #4Dec 4, 2023 09:22AM

The custom sectionName is required as well in cases when you're interested in sum + first at the same time

This would crash:

listOf(
  TraceSectionMetric("JetSnack:setContent", Mode.First),
  TraceSectionMetric("JetSnack:setContent", Mode.Sum)
)

Maybe we should add the Mode name to the metric name, which would make it unique (this would be breaking change for processing though)

Message last modified on Dec 4, 2023 09:22AM

ap...@google.com <ap...@google.com> #5Dec 5, 2023 05:50PM

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

commit 61c874f171271f9ddc01c0ee5ef8ce9ffaf4e055
Author: Chris Craik <ccraik@google.com>
Date: Mon Dec 04 11:02:36 2023

TraceSectionMetric API additions

Bug: 301892382
Fixes: 301955938
Relnote: """
Several changes to TraceSectionMetric API:
- Add Mode.Min, Mode.Max
- Add label argument to override section name as metric label
- Added mode name to output to clarify metric meaning
- Changed default to sum, as most usage of this metric is for repeated events
Be aware of this changes in CI usage, as it may create discontinuities
or break parsing."""

Test: TraceSectionMetricTest
Test: ./gradlew bench:b-m:cC

Change-Id: Ic1e821511d80afb2927b3cb1095336de85e82ba1

M benchmark/benchmark-macro/api/current.txt
M benchmark/benchmark-macro/api/restricted_current.txt
M benchmark/benchmark-macro/src/androidTest/java/androidx/benchmark/macro/TraceSectionMetricTest.kt
M benchmark/benchmark-macro/src/main/java/androidx/benchmark/macro/Metric.kt
M compose/integration-tests/macrobenchmark/src/main/java/androidx/compose/integration/macrobenchmark/SmallListStartupBenchmark.kt
M compose/integration-tests/macrobenchmark/src/main/java/androidx/compose/integration/macrobenchmark/TrivialPerfettoSdkBenchmark.kt
M compose/integration-tests/macrobenchmark/src/main/java/androidx/compose/integration/macrobenchmark/TrivialStartupPerfettoSdkBenchmark.kt
M testutils/testutils-macrobenchmark/src/main/java/androidx/testutils/MacrobenchUtils.kt

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

cc...@google.com <cc...@google.com> Jan 5, 2024 12:00AM

Marked as fixed.

pr...@google.com <pr...@google.com> #6Feb 21, 2024 07:32PM

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.benchmark:benchmark-macro:1.3.0-alpha01

Issue 301892382