diff --git a/core/java/com/android/internal/jank/FrameTracker.java b/core/java/com/android/internal/jank/FrameTracker.java index dbfa4d35974b5..48343803b10e9 100644 --- a/core/java/com/android/internal/jank/FrameTracker.java +++ b/core/java/com/android/internal/jank/FrameTracker.java @@ -66,6 +66,8 @@ public class FrameTracker extends SurfaceControl.OnJankDataListener private static final long INVALID_ID = -1; public static final int NANOS_IN_MILLISECOND = 1_000_000; + private static final int MAX_LENGTH_EVENT_DESC = 20; + static final int REASON_END_UNKNOWN = -1; static final int REASON_END_NORMAL = 0; static final int REASON_END_SURFACE_DESTROYED = 1; @@ -394,7 +396,18 @@ public class FrameTracker extends SurfaceControl.OnJankDataListener return true; } - private void markEvent(String desc) { + /** + * Mark the FrameTracker events in the trace. + * + * @param desc The description of the trace event, + * shouldn't exceed {@link #MAX_LENGTH_EVENT_DESC}. + */ + private void markEvent(@NonNull String desc) { + if (desc.length() > MAX_LENGTH_EVENT_DESC) { + throw new IllegalArgumentException(TextUtils.formatSimple( + "The length of the trace event description <%s> exceeds %d", + desc, MAX_LENGTH_EVENT_DESC)); + } Trace.beginSection(TextUtils.formatSimple("%s#%s", mSession.getName(), desc)); Trace.endSection(); } diff --git a/core/java/com/android/internal/jank/InteractionJankMonitor.java b/core/java/com/android/internal/jank/InteractionJankMonitor.java index e625b31f45d8e..72de78c148f8c 100644 --- a/core/java/com/android/internal/jank/InteractionJankMonitor.java +++ b/core/java/com/android/internal/jank/InteractionJankMonitor.java @@ -149,6 +149,10 @@ public class InteractionJankMonitor { private static final int DEFAULT_TRACE_THRESHOLD_MISSED_FRAMES = 3; private static final int DEFAULT_TRACE_THRESHOLD_FRAME_TIME_MILLIS = 64; + @VisibleForTesting + public static final int MAX_LENGTH_OF_CUJ_NAME = 80; + private static final int MAX_LENGTH_SESSION_NAME = 100; + public static final String ACTION_SESSION_END = ACTION_PREFIX + ".ACTION_SESSION_END"; public static final String ACTION_SESSION_CANCEL = ACTION_PREFIX + ".ACTION_SESSION_CANCEL"; @@ -732,6 +736,9 @@ public class InteractionJankMonitor { * @return the name of the cuj type */ public static String getNameOfCuj(int cujType) { + // Please note: + // 1. The length of the returned string shouldn't exceed MAX_LENGTH_OF_CUJ_NAME. + // 2. The returned string should be the same with the name defined in atoms.proto. switch (cujType) { case CUJ_NOTIFICATION_SHADE_EXPAND_COLLAPSE: return "SHADE_EXPAND_COLLAPSE"; @@ -1106,9 +1113,27 @@ public class InteractionJankMonitor { public Session(@CujType int cujType, @NonNull String postfix) { mCujType = cujType; mTimeStamp = System.nanoTime(); - mName = TextUtils.isEmpty(postfix) - ? String.format("J<%s>", getNameOfCuj(mCujType)) - : String.format("J<%s::%s>", getNameOfCuj(mCujType), postfix); + mName = generateSessionName(getNameOfCuj(cujType), postfix); + } + + private String generateSessionName(@NonNull String cujName, @NonNull String cujPostfix) { + final boolean hasPostfix = !TextUtils.isEmpty(cujPostfix); + // We assert that the cujName shouldn't exceed MAX_LENGTH_OF_CUJ_NAME. + if (cujName.length() > MAX_LENGTH_OF_CUJ_NAME) { + throw new IllegalArgumentException(TextUtils.formatSimple( + "The length of cuj name <%s> exceeds %d", cujName, MAX_LENGTH_OF_CUJ_NAME)); + } + if (hasPostfix) { + final int remaining = MAX_LENGTH_SESSION_NAME - cujName.length(); + if (cujPostfix.length() > remaining) { + cujPostfix = cujPostfix.substring(0, remaining - 3).concat("..."); + } + } + // The max length of the whole string should be: + // 105 with postfix, 83 without postfix + return hasPostfix + ? TextUtils.formatSimple("J<%s::%s>", cujName, cujPostfix) + : TextUtils.formatSimple("J<%s>", cujName); } @CujType diff --git a/core/tests/coretests/src/com/android/internal/jank/InteractionJankMonitorTest.java b/core/tests/coretests/src/com/android/internal/jank/InteractionJankMonitorTest.java index d96f041c13f8f..1519e48adbe0b 100644 --- a/core/tests/coretests/src/com/android/internal/jank/InteractionJankMonitorTest.java +++ b/core/tests/coretests/src/com/android/internal/jank/InteractionJankMonitorTest.java @@ -16,10 +16,26 @@ package com.android.internal.jank; +import static android.text.TextUtils.formatSimple; + import static com.android.internal.jank.FrameTracker.REASON_CANCEL_TIMEOUT; import static com.android.internal.jank.FrameTracker.REASON_END_NORMAL; +import static com.android.internal.jank.InteractionJankMonitor.CUJ_LOCKSCREEN_LAUNCH_CAMERA; +import static com.android.internal.jank.InteractionJankMonitor.CUJ_NOTIFICATION_ADD; +import static com.android.internal.jank.InteractionJankMonitor.CUJ_NOTIFICATION_APP_START; +import static com.android.internal.jank.InteractionJankMonitor.CUJ_NOTIFICATION_HEADS_UP_APPEAR; +import static com.android.internal.jank.InteractionJankMonitor.CUJ_NOTIFICATION_HEADS_UP_DISAPPEAR; +import static com.android.internal.jank.InteractionJankMonitor.CUJ_NOTIFICATION_REMOVE; import static com.android.internal.jank.InteractionJankMonitor.CUJ_NOTIFICATION_SHADE_EXPAND_COLLAPSE; +import static com.android.internal.jank.InteractionJankMonitor.CUJ_NOTIFICATION_SHADE_QS_EXPAND_COLLAPSE; +import static com.android.internal.jank.InteractionJankMonitor.CUJ_NOTIFICATION_SHADE_QS_SCROLL_SWIPE; +import static com.android.internal.jank.InteractionJankMonitor.CUJ_NOTIFICATION_SHADE_ROW_EXPAND; +import static com.android.internal.jank.InteractionJankMonitor.CUJ_NOTIFICATION_SHADE_ROW_SWIPE; +import static com.android.internal.jank.InteractionJankMonitor.CUJ_NOTIFICATION_SHADE_SCROLL_FLING; +import static com.android.internal.jank.InteractionJankMonitor.CUJ_SPLIT_SCREEN_RESIZE; import static com.android.internal.jank.InteractionJankMonitor.CUJ_TO_STATSD_INTERACTION_TYPE; +import static com.android.internal.jank.InteractionJankMonitor.MAX_LENGTH_OF_CUJ_NAME; +import static com.android.internal.jank.InteractionJankMonitor.getNameOfCuj; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; @@ -38,6 +54,7 @@ import android.os.Handler; import android.os.HandlerThread; import android.os.SystemClock; import android.provider.DeviceConfig; +import android.util.SparseArray; import android.view.View; import android.view.ViewAttachTestActivity; @@ -52,8 +69,12 @@ import com.android.internal.jank.FrameTracker.ThreadedRendererWrapper; import com.android.internal.jank.FrameTracker.ViewRootWrapper; import com.android.internal.jank.InteractionJankMonitor.Configuration; import com.android.internal.jank.InteractionJankMonitor.Session; +import com.android.internal.util.FrameworkStatsLog; + +import com.google.common.truth.Expect; import org.junit.Before; +import org.junit.BeforeClass; import org.junit.Rule; import org.junit.Test; import org.mockito.ArgumentCaptor; @@ -64,11 +85,17 @@ import java.util.Arrays; import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.stream.Collectors; @SmallTest public class InteractionJankMonitorTest { private static final String CUJ_POSTFIX = ""; + private static final SparseArray ENUM_NAME_EXCEPTION_MAP = new SparseArray<>(); + private static final SparseArray CUJ_NAME_EXCEPTION_MAP = new SparseArray<>(); + private static final String ENUM_NAME_PREFIX = + "UIINTERACTION_FRAME_INFO_REPORTED__INTERACTION_TYPE__"; + private ViewAttachTestActivity mActivity; private View mView; private HandlerThread mWorker; @@ -77,6 +104,53 @@ public class InteractionJankMonitorTest { public ActivityTestRule mRule = new ActivityTestRule<>(ViewAttachTestActivity.class); + @Rule + public final Expect mExpect = Expect.create(); + + @BeforeClass + public static void initialize() { + ENUM_NAME_EXCEPTION_MAP.put(CUJ_NOTIFICATION_ADD, getEnumName("SHADE_NOTIFICATION_ADD")); + ENUM_NAME_EXCEPTION_MAP.put(CUJ_NOTIFICATION_APP_START, getEnumName("SHADE_APP_LAUNCH")); + ENUM_NAME_EXCEPTION_MAP.put( + CUJ_NOTIFICATION_HEADS_UP_APPEAR, getEnumName("SHADE_HEADS_UP_APPEAR")); + ENUM_NAME_EXCEPTION_MAP.put( + CUJ_NOTIFICATION_HEADS_UP_DISAPPEAR, getEnumName("SHADE_HEADS_UP_DISAPPEAR")); + ENUM_NAME_EXCEPTION_MAP.put( + CUJ_NOTIFICATION_REMOVE, getEnumName("SHADE_NOTIFICATION_REMOVE")); + ENUM_NAME_EXCEPTION_MAP.put( + CUJ_NOTIFICATION_SHADE_EXPAND_COLLAPSE, getEnumName("NOTIFICATION_SHADE_SWIPE")); + ENUM_NAME_EXCEPTION_MAP.put( + CUJ_NOTIFICATION_SHADE_QS_EXPAND_COLLAPSE, getEnumName("SHADE_QS_EXPAND_COLLAPSE")); + ENUM_NAME_EXCEPTION_MAP.put( + CUJ_NOTIFICATION_SHADE_QS_SCROLL_SWIPE, getEnumName("SHADE_QS_SCROLL_SWIPE")); + ENUM_NAME_EXCEPTION_MAP.put( + CUJ_NOTIFICATION_SHADE_ROW_EXPAND, getEnumName("SHADE_ROW_EXPAND")); + ENUM_NAME_EXCEPTION_MAP.put( + CUJ_NOTIFICATION_SHADE_ROW_SWIPE, getEnumName("SHADE_ROW_SWIPE")); + ENUM_NAME_EXCEPTION_MAP.put( + CUJ_NOTIFICATION_SHADE_SCROLL_FLING, getEnumName("SHADE_SCROLL_FLING")); + + CUJ_NAME_EXCEPTION_MAP.put( + CUJ_NOTIFICATION_SHADE_EXPAND_COLLAPSE, "CUJ_NOTIFICATION_SHADE_EXPAND_COLLAPSE"); + CUJ_NAME_EXCEPTION_MAP.put( + CUJ_NOTIFICATION_SHADE_QS_EXPAND_COLLAPSE, + "CUJ_NOTIFICATION_SHADE_QS_EXPAND_COLLAPSE"); + CUJ_NAME_EXCEPTION_MAP.put( + CUJ_NOTIFICATION_SHADE_QS_SCROLL_SWIPE, "CUJ_NOTIFICATION_SHADE_QS_SCROLL_SWIPE"); + CUJ_NAME_EXCEPTION_MAP.put( + CUJ_NOTIFICATION_SHADE_ROW_EXPAND, "CUJ_NOTIFICATION_SHADE_ROW_EXPAND"); + CUJ_NAME_EXCEPTION_MAP.put( + CUJ_NOTIFICATION_SHADE_ROW_SWIPE, "CUJ_NOTIFICATION_SHADE_ROW_SWIPE"); + CUJ_NAME_EXCEPTION_MAP.put( + CUJ_NOTIFICATION_SHADE_SCROLL_FLING, "CUJ_NOTIFICATION_SHADE_SCROLL_FLING"); + CUJ_NAME_EXCEPTION_MAP.put(CUJ_LOCKSCREEN_LAUNCH_CAMERA, "CUJ_LOCKSCREEN_LAUNCH_CAMERA"); + CUJ_NAME_EXCEPTION_MAP.put(CUJ_SPLIT_SCREEN_RESIZE, "CUJ_SPLIT_SCREEN_RESIZE"); + } + + private static String getEnumName(String name) { + return formatSimple("%s%s", ENUM_NAME_PREFIX, name); + } + @Before public void setup() { // Prepare an activity for getting ThreadedRenderer later. @@ -174,6 +248,82 @@ public class InteractionJankMonitorTest { } } + @Test + public void testCujsMapToEnumsCorrectly() { + List cujs = Arrays.stream(InteractionJankMonitor.class.getDeclaredFields()) + .filter(f -> f.getName().startsWith("CUJ_") + && Modifier.isStatic(f.getModifiers()) + && f.getType() == int.class) + .collect(Collectors.toList()); + + Map enumsMap = Arrays.stream(FrameworkStatsLog.class.getDeclaredFields()) + .filter(f -> f.getName().startsWith(ENUM_NAME_PREFIX) + && Modifier.isStatic(f.getModifiers()) + && f.getType() == int.class) + .collect(Collectors.toMap(this::getIntFieldChecked, Field::getName)); + + cujs.forEach(f -> { + final int cuj = getIntFieldChecked(f); + final String cujName = f.getName(); + final String expectedEnumName = ENUM_NAME_EXCEPTION_MAP.contains(cuj) + ? ENUM_NAME_EXCEPTION_MAP.get(cuj) + : formatSimple("%s%s", ENUM_NAME_PREFIX, cujName.substring(4)); + final int enumKey = CUJ_TO_STATSD_INTERACTION_TYPE[cuj]; + final String enumName = enumsMap.get(enumKey); + final String expectedNameOfCuj = CUJ_NAME_EXCEPTION_MAP.contains(cuj) + ? CUJ_NAME_EXCEPTION_MAP.get(cuj) + : formatSimple("CUJ_%s", getNameOfCuj(cuj)); + mExpect + .withMessage(formatSimple( + "%s (%d) not matches %s (%d)", cujName, cuj, enumName, enumKey)) + .that(expectedEnumName.equals(enumName)) + .isTrue(); + mExpect + .withMessage(formatSimple("getNameOfCuj(%d) not matches %s", cuj, cujName)) + .that(cujName.equals(expectedNameOfCuj)) + .isTrue(); + }); + } + + @Test + public void testCujNameLimit() { + Arrays.stream(InteractionJankMonitor.class.getDeclaredFields()) + .filter(f -> f.getName().startsWith("CUJ_") + && Modifier.isStatic(f.getModifiers()) + && f.getType() == int.class) + .forEach(f -> { + final int cuj = getIntFieldChecked(f); + mExpect + .withMessage(formatSimple( + "Too long CUJ(%d) name: %s", cuj, getNameOfCuj(cuj))) + .that(getNameOfCuj(cuj).length()) + .isAtMost(MAX_LENGTH_OF_CUJ_NAME); + }); + } + + @Test + public void testSessionNameLengthLimit() { + final int cujType = CUJ_NOTIFICATION_SHADE_EXPAND_COLLAPSE; + final String cujName = getNameOfCuj(cujType); + final String cujTag = "ThisIsTheCujTag"; + final String tooLongTag = cujTag.repeat(10); + + // Normal case, no postfix. + Session noPostfix = new Session(cujType, ""); + assertThat(noPostfix.getName()).isEqualTo(formatSimple("J<%s>", cujName)); + + // Normal case, with postfix. + Session withPostfix = new Session(cujType, cujTag); + assertThat(withPostfix.getName()).isEqualTo(formatSimple("J<%s::%s>", cujName, cujTag)); + + // Since the length of the cuj name is tested in another test, no need to test it here. + // Too long postfix case, should trim the postfix and keep the cuj name completed. + final String expectedTrimmedName = formatSimple("J<%s::%s>", cujName, + "ThisIsTheCujTagThisIsTheCujTagThisIsTheCujTagThisIsTheCujTagThisIsTheCujTagT..."); + Session longPostfix = new Session(cujType, tooLongTag); + assertThat(longPostfix.getName()).isEqualTo(expectedTrimmedName); + } + private InteractionJankMonitor createMockedInteractionJankMonitor() { InteractionJankMonitor monitor = spy(new InteractionJankMonitor(mWorker)); doReturn(true).when(monitor).shouldMonitor(anyInt()); @@ -217,4 +367,12 @@ public class InteractionJankMonitorTest { return tracker; } + + private int getIntFieldChecked(Field field) { + try { + return field.getInt(null); + } catch (IllegalAccessException ex) { + throw new RuntimeException(ex); + } + } }