From 95e32f0f9a6c65b4e9089608d4ff7e468200cbeb Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Mon, 17 Aug 2020 15:49:46 -0600 Subject: [PATCH 1/2] Hand-rolled linked list for Parcel performance. While recently looking at pprof data, we noticed that Parcel.obtain() and recycle() were some of the most heavily used methods inside the system process. On the surface, this makes sense because these methods are typically invoked at least twice for every Binder call. Internally these methods had been maintaining a pool of cached Parcel objects to avoid GC pressure, but unfortunately it was using an array which resulted in O(n) scanning under heavy load, increasing lock contention encountered by all Binder calls. This change greatly reduces that contention by using a hand-rolled linked list approach, mirroring android.os.Message. For a 1-thread benchmark, this new approach has almost 2x throughput, and for a 16-thread benchmark it has almost 8x throughput. As part of making this change we evaluated several approaches, including using pure-GC with no pooling, a single AtomicReference, and a pool of several AtomicReferences. To measure these we wrote ParcelPoolBenchmark which simulates various levels of Binder load using 1, 4 and 16 threads. Below are the relative benchmark results compared to the previous approach before this CL: 1 thread 4 threads 16 threads Pure GC 131.74% 32.76% 43.90% Single AR 95.22% 25.54% 13.66% Pooled AR 57.65% 16.21% 11.55% Linked list 52.66% 18.06% 12.55% On balance, the linked list approach performs well across the board, and we bias towards it over the two AtomicReference approaches since it performs slightly better on the single-threaded case, which is the most representative of typical Binder load across all processes. Bug: 165032569 Test: ./frameworks/base/libs/hwui/tests/scripts/prep_generic.sh little && atest CorePerfTests:android.os.ParcelObtainPerfTest Change-Id: I190b1c8f7fd59855c3c2d36032512279691e2c04 --- .../src/android/os/ParcelObtainPerfTest.java | 88 +++++++++++++ .../core/src/android/os/ParcelPerfTest.java | 15 --- core/java/android/os/Parcel.java | 118 ++++++++++++------ 3 files changed, 165 insertions(+), 56 deletions(-) create mode 100644 apct-tests/perftests/core/src/android/os/ParcelObtainPerfTest.java diff --git a/apct-tests/perftests/core/src/android/os/ParcelObtainPerfTest.java b/apct-tests/perftests/core/src/android/os/ParcelObtainPerfTest.java new file mode 100644 index 0000000000000..760ae12bcc07c --- /dev/null +++ b/apct-tests/perftests/core/src/android/os/ParcelObtainPerfTest.java @@ -0,0 +1,88 @@ +/* + * Copyright (C) 2016 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +package android.os; + +import androidx.test.InstrumentationRegistry; +import androidx.test.filters.LargeTest; +import androidx.test.runner.AndroidJUnit4; + +import org.junit.Test; +import org.junit.runner.RunWith; + +@RunWith(AndroidJUnit4.class) +@LargeTest +public class ParcelObtainPerfTest { + private static final int ITERATIONS = 1_000_000; + + @Test + public void timeContention_01() throws Exception { + timeContention(1); + } + + @Test + public void timeContention_04() throws Exception { + timeContention(4); + } + + @Test + public void timeContention_16() throws Exception { + timeContention(16); + } + + private static void timeContention(int numThreads) throws Exception { + final long start = SystemClock.elapsedRealtime(); + { + final ObtainThread[] threads = new ObtainThread[numThreads]; + for (int i = 0; i < numThreads; i++) { + final ObtainThread thread = new ObtainThread(ITERATIONS / numThreads); + thread.start(); + threads[i] = thread; + } + for (int i = 0; i < numThreads; i++) { + threads[i].join(); + } + } + final long duration = SystemClock.elapsedRealtime() - start; + + final Bundle results = new Bundle(); + results.putLong("duration", duration); + InstrumentationRegistry.getInstrumentation().sendStatus(0, results); + } + + public static class ObtainThread extends Thread { + public int iterations; + + public ObtainThread(int iterations) { + this.iterations = iterations; + } + + @Override + public void run() { + while (iterations-- > 0) { + final Parcel data = Parcel.obtain(); + final Parcel reply = Parcel.obtain(); + try { + data.writeInt(32); + reply.writeInt(32); + } finally { + reply.recycle(); + data.recycle(); + } + } + } + } +} diff --git a/apct-tests/perftests/core/src/android/os/ParcelPerfTest.java b/apct-tests/perftests/core/src/android/os/ParcelPerfTest.java index 4db9262f7fe36..be2f9d72663ee 100644 --- a/apct-tests/perftests/core/src/android/os/ParcelPerfTest.java +++ b/apct-tests/perftests/core/src/android/os/ParcelPerfTest.java @@ -158,21 +158,6 @@ public class ParcelPerfTest { } } - @Test - public void timeObtainRecycle() { - // Use up the pooled instances. - // A lot bigger than the actual size but in case someone increased it. - final int POOL_SIZE = 100; - for (int i = 0; i < POOL_SIZE; i++) { - Parcel.obtain(); - } - - final BenchmarkState state = mPerfStatusReporter.getBenchmarkState(); - while (state.keepRunning()) { - Parcel.obtain().recycle(); - } - } - @Test public void timeWriteException() { timeWriteException(false); diff --git a/core/java/android/os/Parcel.java b/core/java/android/os/Parcel.java index 415e5a60a1406..dce474fd8654a 100644 --- a/core/java/android/os/Parcel.java +++ b/core/java/android/os/Parcel.java @@ -33,6 +33,8 @@ import android.util.SparseArray; import android.util.SparseBooleanArray; import android.util.SparseIntArray; +import com.android.internal.annotations.GuardedBy; + import dalvik.annotation.optimization.CriticalNative; import dalvik.annotation.optimization.FastNative; import dalvik.system.VMRuntime; @@ -222,9 +224,31 @@ public final class Parcel { */ private static boolean sParcelExceptionStackTrace; - private static final int POOL_SIZE = 6; - private static final Parcel[] sOwnedPool = new Parcel[POOL_SIZE]; - private static final Parcel[] sHolderPool = new Parcel[POOL_SIZE]; + private static final Object sPoolSync = new Object(); + + /** Next item in the linked list pool, if any */ + @GuardedBy("sPoolSync") + private Parcel mPoolNext; + + /** Head of a linked list pool of {@link Parcel} objects */ + @GuardedBy("sPoolSync") + private static Parcel sOwnedPool; + /** Head of a linked list pool of {@link Parcel} objects */ + @GuardedBy("sPoolSync") + private static Parcel sHolderPool; + + /** Total size of pool with head at {@link #sOwnedPool} */ + @GuardedBy("sPoolSync") + private static int sOwnedPoolSize = 0; + /** Total size of pool with head at {@link #sHolderPool} */ + @GuardedBy("sPoolSync") + private static int sHolderPoolSize = 0; + + /** + * We're willing to pool up to 32 objects, which is sized to accommodate + * both a data and reply Parcel for the maximum of 16 Binder threads. + */ + private static final int POOL_SIZE = 32; // Keep in sync with frameworks/native/include/private/binder/ParcelValTypes.h. private static final int VAL_NULL = -1; @@ -420,22 +444,27 @@ public final class Parcel { */ @NonNull public static Parcel obtain() { - final Parcel[] pool = sOwnedPool; - synchronized (pool) { - Parcel p; - for (int i=0; i Date: Fri, 11 Sep 2020 11:18:28 -0600 Subject: [PATCH 2/2] Remove advisory registerNativeAllocation(). After consulting with the ART team, we learned that sending them detailed advisory native allocation sizes isn't as useful when the overall lifecycle of the object is tightly managed, which is the case with Parcel objects. (Because Parcel users use explicit obtain() and recycle() methods, any variable-sized native allocations have already been freed by the time a Parcel instance is considered for GC.) The Parcel benchmarks referenced below are showing a uniform ~3% performance improvement across 1, 4, and 16 thread cases. Note that this is in addition to the improvements recently made with the shift to a linked-list pooling design. Bug: 165032569 Test: ./frameworks/base/libs/hwui/tests/scripts/prep_generic.sh little && atest CorePerfTests:android.os.ParcelObtainPerfTest Change-Id: Id0ce9b3bff1d0ffb426a9f105c7a54eb00060f85 --- core/java/android/os/Parcel.java | 39 ++++++++------------------------ core/jni/android_os_Parcel.cpp | 35 ++++++++++++---------------- 2 files changed, 24 insertions(+), 50 deletions(-) diff --git a/core/java/android/os/Parcel.java b/core/java/android/os/Parcel.java index dce474fd8654a..1bddc49f839ce 100644 --- a/core/java/android/os/Parcel.java +++ b/core/java/android/os/Parcel.java @@ -37,7 +37,6 @@ import com.android.internal.annotations.GuardedBy; import dalvik.annotation.optimization.CriticalNative; import dalvik.annotation.optimization.FastNative; -import dalvik.system.VMRuntime; import libcore.util.ArrayUtils; import libcore.util.SneakyThrow; @@ -309,7 +308,7 @@ public final class Parcel { @CriticalNative private static native int nativeDataCapacity(long nativePtr); @FastNative - private static native long nativeSetDataSize(long nativePtr, int size); + private static native void nativeSetDataSize(long nativePtr, int size); @CriticalNative private static native void nativeSetDataPosition(long nativePtr, int pos); @FastNative @@ -338,7 +337,7 @@ public final class Parcel { @FastNative private static native void nativeWriteStrongBinder(long nativePtr, IBinder val); @FastNative - private static native long nativeWriteFileDescriptor(long nativePtr, FileDescriptor val); + private static native void nativeWriteFileDescriptor(long nativePtr, FileDescriptor val); private static native byte[] nativeCreateByteArray(long nativePtr); private static native boolean nativeReadByteArray(long nativePtr, byte[] dest, int destLen); @@ -361,14 +360,14 @@ public final class Parcel { private static native FileDescriptor nativeReadFileDescriptor(long nativePtr); private static native long nativeCreate(); - private static native long nativeFreeBuffer(long nativePtr); + private static native void nativeFreeBuffer(long nativePtr); private static native void nativeDestroy(long nativePtr); private static native byte[] nativeMarshall(long nativePtr); - private static native long nativeUnmarshall( + private static native void nativeUnmarshall( long nativePtr, byte[] data, int offset, int length); private static native int nativeCompareData(long thisNativePtr, long otherNativePtr); - private static native long nativeAppendFrom( + private static native void nativeAppendFrom( long thisNativePtr, long otherNativePtr, int offset, int length); @CriticalNative private static native boolean nativeHasFileDescriptors(long nativePtr); @@ -563,7 +562,7 @@ public final class Parcel { * @param size The new number of bytes in the Parcel. */ public final void setDataSize(int size) { - updateNativeSize(nativeSetDataSize(mNativePtr, size)); + nativeSetDataSize(mNativePtr, size); } /** @@ -615,11 +614,11 @@ public final class Parcel { * Set the bytes in data to be the raw bytes of this Parcel. */ public final void unmarshall(@NonNull byte[] data, int offset, int length) { - updateNativeSize(nativeUnmarshall(mNativePtr, data, offset, length)); + nativeUnmarshall(mNativePtr, data, offset, length); } public final void appendFrom(Parcel parcel, int offset, int length) { - updateNativeSize(nativeAppendFrom(mNativePtr, parcel.mNativePtr, offset, length)); + nativeAppendFrom(mNativePtr, parcel.mNativePtr, offset, length); } /** @hide */ @@ -902,24 +901,7 @@ public final class Parcel { * if {@link Parcelable#PARCELABLE_WRITE_RETURN_VALUE} is set.

*/ public final void writeFileDescriptor(@NonNull FileDescriptor val) { - updateNativeSize(nativeWriteFileDescriptor(mNativePtr, val)); - } - - private void updateNativeSize(long newNativeSize) { - if (mOwnsNativeParcelObject) { - if (newNativeSize > Integer.MAX_VALUE) { - newNativeSize = Integer.MAX_VALUE; - } - if (newNativeSize != mNativeSize) { - int delta = (int) (newNativeSize - mNativeSize); - if (delta > 0) { - VMRuntime.getRuntime().registerNativeAllocation(delta); - } else { - VMRuntime.getRuntime().registerNativeFree(-delta); - } - mNativeSize = newNativeSize; - } - } + nativeWriteFileDescriptor(mNativePtr, val); } /** @@ -3571,7 +3553,7 @@ public final class Parcel { private void freeBuffer() { resetSqaushingState(); if (mOwnsNativeParcelObject) { - updateNativeSize(nativeFreeBuffer(mNativePtr)); + nativeFreeBuffer(mNativePtr); } mReadWriteHelper = ReadWriteHelper.DEFAULT; } @@ -3581,7 +3563,6 @@ public final class Parcel { if (mNativePtr != 0) { if (mOwnsNativeParcelObject) { nativeDestroy(mNativePtr); - updateNativeSize(0); } mNativePtr = 0; } diff --git a/core/jni/android_os_Parcel.cpp b/core/jni/android_os_Parcel.cpp index 7cfe3bc1020ab..0892b70b9651b 100644 --- a/core/jni/android_os_Parcel.cpp +++ b/core/jni/android_os_Parcel.cpp @@ -114,7 +114,7 @@ static jint android_os_Parcel_dataCapacity(jlong nativePtr) return parcel ? parcel->dataCapacity() : 0; } -static jlong android_os_Parcel_setDataSize(JNIEnv* env, jclass clazz, jlong nativePtr, jint size) +static void android_os_Parcel_setDataSize(JNIEnv* env, jclass clazz, jlong nativePtr, jint size) { Parcel* parcel = reinterpret_cast(nativePtr); if (parcel != NULL) { @@ -122,9 +122,7 @@ static jlong android_os_Parcel_setDataSize(JNIEnv* env, jclass clazz, jlong nati if (err != NO_ERROR) { signalExceptionForError(env, clazz, err); } - return parcel->getOpenAshmemSize(); } - return 0; } static void android_os_Parcel_setDataPosition(jlong nativePtr, jint pos) @@ -308,7 +306,7 @@ static void android_os_Parcel_writeStrongBinder(JNIEnv* env, jclass clazz, jlong } } -static jlong android_os_Parcel_writeFileDescriptor(JNIEnv* env, jclass clazz, jlong nativePtr, jobject object) +static void android_os_Parcel_writeFileDescriptor(JNIEnv* env, jclass clazz, jlong nativePtr, jobject object) { Parcel* parcel = reinterpret_cast(nativePtr); if (parcel != NULL) { @@ -317,9 +315,7 @@ static jlong android_os_Parcel_writeFileDescriptor(JNIEnv* env, jclass clazz, jl if (err != NO_ERROR) { signalExceptionForError(env, clazz, err); } - return parcel->getOpenAshmemSize(); } - return 0; } static jbyteArray android_os_Parcel_createByteArray(JNIEnv* env, jclass clazz, jlong nativePtr) @@ -506,14 +502,12 @@ static jlong android_os_Parcel_create(JNIEnv* env, jclass clazz) return reinterpret_cast(parcel); } -static jlong android_os_Parcel_freeBuffer(JNIEnv* env, jclass clazz, jlong nativePtr) +static void android_os_Parcel_freeBuffer(JNIEnv* env, jclass clazz, jlong nativePtr) { Parcel* parcel = reinterpret_cast(nativePtr); if (parcel != NULL) { parcel->freeData(); - return parcel->getOpenAshmemSize(); } - return 0; } static void android_os_Parcel_destroy(JNIEnv* env, jclass clazz, jlong nativePtr) @@ -551,12 +545,12 @@ static jbyteArray android_os_Parcel_marshall(JNIEnv* env, jclass clazz, jlong na return ret; } -static jlong android_os_Parcel_unmarshall(JNIEnv* env, jclass clazz, jlong nativePtr, +static void android_os_Parcel_unmarshall(JNIEnv* env, jclass clazz, jlong nativePtr, jbyteArray data, jint offset, jint length) { Parcel* parcel = reinterpret_cast(nativePtr); if (parcel == NULL || length < 0) { - return 0; + return; } jbyte* array = (jbyte*)env->GetPrimitiveArrayCritical(data, 0); @@ -570,7 +564,6 @@ static jlong android_os_Parcel_unmarshall(JNIEnv* env, jclass clazz, jlong nativ env->ReleasePrimitiveArrayCritical(data, array, 0); } - return parcel->getOpenAshmemSize(); } static jint android_os_Parcel_compareData(JNIEnv* env, jclass clazz, jlong thisNativePtr, @@ -588,23 +581,23 @@ static jint android_os_Parcel_compareData(JNIEnv* env, jclass clazz, jlong thisN return thisParcel->compareData(*otherParcel); } -static jlong android_os_Parcel_appendFrom(JNIEnv* env, jclass clazz, jlong thisNativePtr, +static void android_os_Parcel_appendFrom(JNIEnv* env, jclass clazz, jlong thisNativePtr, jlong otherNativePtr, jint offset, jint length) { Parcel* thisParcel = reinterpret_cast(thisNativePtr); if (thisParcel == NULL) { - return 0; + return; } Parcel* otherParcel = reinterpret_cast(otherNativePtr); if (otherParcel == NULL) { - return thisParcel->getOpenAshmemSize(); + return; } status_t err = thisParcel->appendFrom(otherParcel, offset, length); if (err != NO_ERROR) { signalExceptionForError(env, clazz, err); } - return thisParcel->getOpenAshmemSize(); + return; } static jboolean android_os_Parcel_hasFileDescriptors(jlong nativePtr) @@ -720,7 +713,7 @@ static const JNINativeMethod gParcelMethods[] = { // @CriticalNative {"nativeDataCapacity", "(J)I", (void*)android_os_Parcel_dataCapacity}, // @FastNative - {"nativeSetDataSize", "(JI)J", (void*)android_os_Parcel_setDataSize}, + {"nativeSetDataSize", "(JI)V", (void*)android_os_Parcel_setDataSize}, // @CriticalNative {"nativeSetDataPosition", "(JI)V", (void*)android_os_Parcel_setDataPosition}, // @FastNative @@ -749,7 +742,7 @@ static const JNINativeMethod gParcelMethods[] = { // @FastNative {"nativeWriteStrongBinder", "(JLandroid/os/IBinder;)V", (void*)android_os_Parcel_writeStrongBinder}, // @FastNative - {"nativeWriteFileDescriptor", "(JLjava/io/FileDescriptor;)J", (void*)android_os_Parcel_writeFileDescriptor}, + {"nativeWriteFileDescriptor", "(JLjava/io/FileDescriptor;)V", (void*)android_os_Parcel_writeFileDescriptor}, {"nativeCreateByteArray", "(J)[B", (void*)android_os_Parcel_createByteArray}, {"nativeReadByteArray", "(J[BI)Z", (void*)android_os_Parcel_readByteArray}, @@ -772,13 +765,13 @@ static const JNINativeMethod gParcelMethods[] = { {"nativeReadFileDescriptor", "(J)Ljava/io/FileDescriptor;", (void*)android_os_Parcel_readFileDescriptor}, {"nativeCreate", "()J", (void*)android_os_Parcel_create}, - {"nativeFreeBuffer", "(J)J", (void*)android_os_Parcel_freeBuffer}, + {"nativeFreeBuffer", "(J)V", (void*)android_os_Parcel_freeBuffer}, {"nativeDestroy", "(J)V", (void*)android_os_Parcel_destroy}, {"nativeMarshall", "(J)[B", (void*)android_os_Parcel_marshall}, - {"nativeUnmarshall", "(J[BII)J", (void*)android_os_Parcel_unmarshall}, + {"nativeUnmarshall", "(J[BII)V", (void*)android_os_Parcel_unmarshall}, {"nativeCompareData", "(JJ)I", (void*)android_os_Parcel_compareData}, - {"nativeAppendFrom", "(JJII)J", (void*)android_os_Parcel_appendFrom}, + {"nativeAppendFrom", "(JJII)V", (void*)android_os_Parcel_appendFrom}, // @CriticalNative {"nativeHasFileDescriptors", "(J)Z", (void*)android_os_Parcel_hasFileDescriptors}, {"nativeWriteInterfaceToken", "(JLjava/lang/String;)V", (void*)android_os_Parcel_writeInterfaceToken},