From fcc32aa4f7ec724749cd6653a2a3898d97e1891f Mon Sep 17 00:00:00 2001 From: Zim Date: Fri, 14 Jul 2023 17:55:03 +0100 Subject: [PATCH] Enable perfetto tracing for non-autogenerated AIDL names Since we added AIDL name tracing globally, it's been a very powerfull tool to root cause issues in the field. One gap is non-autogenerated AIDL interfaces like ContentProvider and Cursor. This was explicitly disabled in aosp/2254719 to workaround some unrealistic perf startup issue. Given the gap in identifying more important issues in the field it's worth enabling this so we can fix 'real' problems. Enabled caching of the non-autogenerated AIDLs and also added the 'AIDL::java' prefix to the name of the non-autogenerated AIDL names to be consistent with the autogenerated ones. Bug: 288578300 Bug: 270044534 Test: atest AidlTest Change-Id: I96633ec976db01991620b43d7a7e9528ea22de1f --- core/java/android/os/Binder.java | 25 +++++--------- .../coretests/src/android/os/AidlTest.java | 33 ++++++++++++++----- 2 files changed, 34 insertions(+), 24 deletions(-) diff --git a/core/java/android/os/Binder.java b/core/java/android/os/Binder.java index 5d0a723241b0e..3f04ca480daff 100644 --- a/core/java/android/os/Binder.java +++ b/core/java/android/os/Binder.java @@ -943,16 +943,19 @@ public class Binder implements IBinder { * @hide */ @VisibleForTesting - public final @NonNull String getTransactionTraceName(int transactionCode) { + public final @Nullable String getTransactionTraceName(int transactionCode) { + final boolean isInterfaceUserDefined = getMaxTransactionId() == 0; if (mTransactionTraceNames == null) { - final int highestId = Math.min(getMaxTransactionId(), TRANSACTION_TRACE_NAME_ID_LIMIT); + final int highestId = isInterfaceUserDefined ? TRANSACTION_TRACE_NAME_ID_LIMIT + : Math.min(getMaxTransactionId(), TRANSACTION_TRACE_NAME_ID_LIMIT); mSimpleDescriptor = getSimpleDescriptor(); mTransactionTraceNames = new AtomicReferenceArray(highestId + 1); } - final int index = transactionCode - FIRST_CALL_TRANSACTION; - if (index < 0 || index >= mTransactionTraceNames.length()) { - return mSimpleDescriptor + "#" + transactionCode; + final int index = isInterfaceUserDefined + ? transactionCode : transactionCode - FIRST_CALL_TRANSACTION; + if (index >= mTransactionTraceNames.length() || index < 0) { + return null; } String transactionTraceName = mTransactionTraceNames.getAcquire(index); @@ -1317,19 +1320,9 @@ public class Binder implements IBinder { final boolean hasFullyQualifiedName = getMaxTransactionId() > 0; final String transactionTraceName; - if (tagEnabled && hasFullyQualifiedName) { + if (tagEnabled) { // If tracing enabled and we have a fully qualified name, fetch the name transactionTraceName = getTransactionTraceName(code); - } else if (tagEnabled && isStackTrackingEnabled()) { - // If tracing is enabled and we *don't* have a fully qualified name, fetch the - // 'best effort' name only for stack tracking. This works around noticeable perf impact - // on low latency binder calls (<100us). The tracing call itself is between (1-10us) and - // the perf impact can be quite noticeable while benchmarking such binder calls. - // The primary culprits are ContentProviders and Cursors which convenienty don't - // autogenerate their AIDL and hence will not have a fully qualified name. - // - // TODO(b/253426478): Relax this constraint after a more robust fix - transactionTraceName = getTransactionTraceName(code); } else { transactionTraceName = null; } diff --git a/core/tests/coretests/src/android/os/AidlTest.java b/core/tests/coretests/src/android/os/AidlTest.java index 5f54b093e5e5f..d0c3470c4c1f6 100644 --- a/core/tests/coretests/src/android/os/AidlTest.java +++ b/core/tests/coretests/src/android/os/AidlTest.java @@ -28,12 +28,14 @@ public class AidlTest extends TestCase { private IAidlTest mRemote; private AidlObject mLocal; + private NonAutoGeneratedObject mNonAutoGenerated; @Override protected void setUp() throws Exception { super.setUp(); mLocal = new AidlObject(); mRemote = IAidlTest.Stub.asInterface(mLocal); + mNonAutoGenerated = new NonAutoGeneratedObject("NonAutoGeneratedObject"); } private static boolean check(TestParcelable p, int n, String s) { @@ -84,6 +86,12 @@ public class AidlTest extends TestCase { } } + private static class NonAutoGeneratedObject extends Binder { + NonAutoGeneratedObject(String descriptor) { + super(descriptor); + } + } + private static class AidlObject extends IAidlTest.Stub { public IInterface queryLocalInterface(String descriptor) { // overriding this to return null makes asInterface always @@ -194,7 +202,7 @@ public class AidlTest extends TestCase { TestParcelable[] a1, TestParcelable[] a2) { return null; } - + public void voidSecurityException() { throw new SecurityException("gotcha!"); } @@ -396,7 +404,7 @@ public class AidlTest extends TestCase { assertEquals("s2[1]", s2[1]); assertEquals("s2[2]", s2[2]); } - + @SmallTest public void testVoidSecurityException() throws Exception { boolean good = false; @@ -407,7 +415,7 @@ public class AidlTest extends TestCase { } assertEquals(good, true); } - + @SmallTest public void testIntSecurityException() throws Exception { boolean good = false; @@ -420,7 +428,7 @@ public class AidlTest extends TestCase { } @SmallTest - public void testGetTransactionName() throws Exception { + public void testGetTransactionNameAutoGenerated() throws Exception { assertEquals(15, mLocal.getMaxTransactionId()); assertEquals("booleanArray", @@ -430,12 +438,21 @@ public class AidlTest extends TestCase { assertEquals("parcelableIn", mLocal.getTransactionName(IAidlTest.Stub.TRANSACTION_parcelableIn)); - assertEquals("IAidlTest:booleanArray", + assertEquals("AIDL::java::IAidlTest::booleanArray::server", mLocal.getTransactionTraceName(IAidlTest.Stub.TRANSACTION_booleanArray)); - assertEquals("IAidlTest:voidSecurityException", + assertEquals("AIDL::java::IAidlTest::voidSecurityException::server", mLocal.getTransactionTraceName(IAidlTest.Stub.TRANSACTION_voidSecurityException)); - assertEquals("IAidlTest:parcelableIn", + assertEquals("AIDL::java::IAidlTest::parcelableIn::server", mLocal.getTransactionTraceName(IAidlTest.Stub.TRANSACTION_parcelableIn)); } -} + @SmallTest + public void testGetTransactionNameNonAutoGenerated() throws Exception { + assertEquals(0, mNonAutoGenerated.getMaxTransactionId()); + + assertEquals("AIDL::java::NonAutoGeneratedObject::#0::server", + mNonAutoGenerated.getTransactionTraceName(0)); + assertEquals("AIDL::java::NonAutoGeneratedObject::#1::server", + mNonAutoGenerated.getTransactionTraceName(1)); + } +}