From 94dc620f10405596d2182f5cabcce08906bab729 Mon Sep 17 00:00:00 2001 From: Michael Thiessen Date: Wed, 7 Sep 2022 20:32:30 +0000 Subject: [PATCH] Add trace events to View#dispatchTouchEvent and RecyclerView#scrollStep In local testing, View#dispatchTouchEvent and RecyclerView#scrollStep take up a large chunk of time under Choreographer callbacks and are not currently captured in traces for the scroll jank dashboard. Hopefully, adding these trace events will clarify at least some of the jank currently attributed to "Choreographer(java_views=)" RecyclerView#scrollStep is a package-private method in AndroidX that isn't part of the View API, so required some minor changes to the Bytecode rewriter to support methods missing from some class hierarchies. Bug: 1357024 Change-Id: I73f8a7c733501e69b744e3315bba9360819fe3a7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3876627 Reviewed-by: Mohamed Heikal Commit-Queue: Michael Thiessen Cr-Commit-Position: refs/heads/main@{#1044158} NOKEYCHECK=True GitOrigin-RevId: 85144e544dcecdc6928a86bc381ac08970953e2f --- .../EmptyOverrideGeneratorClassAdapter.java | 1 + .../ParentMethodCheckerClassAdapter.java | 19 +++++++++++++++++-- .../chromium/bytecode/TraceEventAdder.java | 5 ++++- 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/android/bytecode/java/org/chromium/bytecode/EmptyOverrideGeneratorClassAdapter.java b/android/bytecode/java/org/chromium/bytecode/EmptyOverrideGeneratorClassAdapter.java index d0957625d7..63ad6148b0 100644 --- a/android/bytecode/java/org/chromium/bytecode/EmptyOverrideGeneratorClassAdapter.java +++ b/android/bytecode/java/org/chromium/bytecode/EmptyOverrideGeneratorClassAdapter.java @@ -75,6 +75,7 @@ public void visitEnd() { */ private void writeOverrideCode( MethodVisitor mv, final int access, final String name, final String descriptor) { + assert access != 0; Type[] argTypes = Type.getArgumentTypes(descriptor); Type returnType = Type.getReturnType(descriptor); diff --git a/android/bytecode/java/org/chromium/bytecode/ParentMethodCheckerClassAdapter.java b/android/bytecode/java/org/chromium/bytecode/ParentMethodCheckerClassAdapter.java index d913f1a73e..6d399460c1 100644 --- a/android/bytecode/java/org/chromium/bytecode/ParentMethodCheckerClassAdapter.java +++ b/android/bytecode/java/org/chromium/bytecode/ParentMethodCheckerClassAdapter.java @@ -6,6 +6,8 @@ import static org.objectweb.asm.Opcodes.ACC_FINAL; import static org.objectweb.asm.Opcodes.ACC_PRIVATE; +import static org.objectweb.asm.Opcodes.ACC_PROTECTED; +import static org.objectweb.asm.Opcodes.ACC_PUBLIC; import static org.objectweb.asm.Opcodes.ASM7; import org.objectweb.asm.ClassVisitor; @@ -19,7 +21,7 @@ * methods remain then we recurse on the class's superclass. */ class ParentMethodCheckerClassAdapter extends ClassVisitor { - private static final String OBJECT_CLASS_DESCRIPTOR = "java.lang.Object"; + private static final String OBJECT_CLASS_DESCRIPTOR = "java/lang/Object"; private final ArrayList mMethodsToCheck; private final ClassLoader mJarClassLoader; @@ -62,8 +64,12 @@ public MethodVisitor visitMethod( // This class contains methodToCheck. boolean isMethodPrivate = (access & ACC_PRIVATE) == ACC_PRIVATE; boolean isMethodFinal = (access & ACC_FINAL) == ACC_FINAL; + boolean isMethodPackagePrivate = + (access & (ACC_PUBLIC | ACC_PROTECTED | ACC_PRIVATE)) == 0; + // If the method is private or final then don't create an override. - methodToCheck.shouldCreateOverride = !isMethodPrivate && !isMethodFinal; + methodToCheck.shouldCreateOverride = + !isMethodPrivate && !isMethodFinal && !isMethodPackagePrivate; } return super.visitMethod(access, name, descriptor, signature, exceptions); @@ -72,6 +78,15 @@ public MethodVisitor visitMethod( @Override public void visitEnd() { if (mIsCheckingObjectClass) { + // We support tracing methods that are defined in classes that are derived from View, + // but are not defined in View itself. If we've reached the Object class in the + // hierarchy, it means the method doesn't exist in this hierarchy, so don't override it, + // and stop looking for it. + for (MethodDescription method : mMethodsToCheck) { + if (method.shouldCreateOverride == null) { + method.shouldCreateOverride = false; + } + } return; } diff --git a/android/bytecode/java/org/chromium/bytecode/TraceEventAdder.java b/android/bytecode/java/org/chromium/bytecode/TraceEventAdder.java index 515758deac..dd1c70f193 100644 --- a/android/bytecode/java/org/chromium/bytecode/TraceEventAdder.java +++ b/android/bytecode/java/org/chromium/bytecode/TraceEventAdder.java @@ -71,9 +71,12 @@ protected boolean shouldRewriteClass(String classPath) { @Override protected boolean shouldRewriteClass(ClassReader classReader) { mMethodsToTrace = new ArrayList<>(Arrays.asList( + new MethodDescription( + "dispatchTouchEvent", "(Landroid/view/MotionEvent;)Z", Opcodes.ACC_PUBLIC), new MethodDescription("draw", "(Landroid/graphics/Canvas;)V", Opcodes.ACC_PUBLIC), new MethodDescription("onMeasure", "(II)V", Opcodes.ACC_PROTECTED), - new MethodDescription("onLayout", "(ZIIII)V", Opcodes.ACC_PROTECTED))); + new MethodDescription("onLayout", "(ZIIII)V", Opcodes.ACC_PROTECTED), + new MethodDescription("scrollStep", "(II[I)V", 0))); // This adapter will modify mMethodsToTrace to indicate which methods already exist in the // class and which ones need to be overridden. In case the class is not an Android view