Merge "Address static cache access issues." (2a6df07a) · Commits · e / os / android_frameworks_base

core/java/android/util/ArrayMap.java

+58 −20

Original line number	Diff line number	Diff line
		@@ -48,6 +48,8 @@ import java.util.Set;
		* you have no control over this shrinking -- if you set a capacity and then remove an
		* item, it may reduce the capacity to better match the current size. In the future an
		* explicit call to set the capacity should turn off this aggressive shrinking behavior.</p>
		*
		* <p>This structure is <b>NOT</b> thread-safe.</p>
		*/
		public final class ArrayMap<K, V> implements Map<K, V> {
		private static final boolean DEBUG = false;
		@@ -103,15 +105,21 @@ public final class ArrayMap<K, V> implements Map<K, V> {
		static Object[] mTwiceBaseCache;
		@UnsupportedAppUsage(maxTargetSdk = 28) // Allocations are an implementation detail.
		static int mTwiceBaseCacheSize;
		/**
		* Separate locks for each cache since each can be accessed independently of the other without
		* risk of a deadlock.
		*/
		private static final Object sBaseCacheLock = new Object();
		private static final Object sTwiceBaseCacheLock = new Object();

		final boolean mIdentityHashCode;
		private final boolean mIdentityHashCode;
		@UnsupportedAppUsage(maxTargetSdk = 28) // Hashes are an implementation detail. Use public key/value API.
		int[] mHashes;
		@UnsupportedAppUsage(maxTargetSdk = 28) // Storage is an implementation detail. Use public key/value API.
		Object[] mArray;
		@UnsupportedAppUsage(maxTargetSdk = 28) // Use size()
		int mSize;
		MapCollections<K, V> mCollections;
		private MapCollections<K, V> mCollections;

		private static int binarySearchHashes(int[] hashes, int N, int hash) {
		try {
		@@ -209,32 +217,58 @@ public final class ArrayMap<K, V> implements Map<K, V> {
		throw new UnsupportedOperationException("ArrayMap is immutable");
		}
		if (size == (BASE_SIZE*2)) {
		synchronized (ArrayMap.class) {
		synchronized (sTwiceBaseCacheLock) {
		if (mTwiceBaseCache != null) {
		final Object[] array = mTwiceBaseCache;
		mArray = array;
		try {
		mTwiceBaseCache = (Object[]) array[0];
		mHashes = (int[]) array[1];
		if (mHashes != null) {
		array[0] = array[1] = null;
		mTwiceBaseCacheSize--;
		if (DEBUG) Log.d(TAG, "Retrieving 2x cache " + mHashes
		if (DEBUG) {
		Log.d(TAG, "Retrieving 2x cache " + mHashes
		+ " now have " + mTwiceBaseCacheSize + " entries");
		}
		return;
		}
		} catch (ClassCastException e) {
		}
		// Whoops! Someone trampled the array (probably due to not protecting
		// their access with a lock). Our cache is corrupt; report and give up.
		Slog.wtf(TAG, "Found corrupt ArrayMap cache: [0]=" + array[0]
		+ " [1]=" + array[1]);
		mTwiceBaseCache = null;
		mTwiceBaseCacheSize = 0;
		}
		}
		} else if (size == BASE_SIZE) {
		synchronized (ArrayMap.class) {
		synchronized (sBaseCacheLock) {
		if (mBaseCache != null) {
		final Object[] array = mBaseCache;
		mArray = array;
		try {
		mBaseCache = (Object[]) array[0];
		mHashes = (int[]) array[1];
		if (mHashes != null) {
		array[0] = array[1] = null;
		mBaseCacheSize--;
		if (DEBUG) Log.d(TAG, "Retrieving 1x cache " + mHashes
		if (DEBUG) {
		Log.d(TAG, "Retrieving 1x cache " + mHashes
		+ " now have " + mBaseCacheSize + " entries");
		}
		return;
		}
		} catch (ClassCastException e) {
		}
		// Whoops! Someone trampled the array (probably due to not protecting
		// their access with a lock). Our cache is corrupt; report and give up.
		Slog.wtf(TAG, "Found corrupt ArrayMap cache: [0]=" + array[0]
		+ " [1]=" + array[1]);
		mBaseCache = null;
		mBaseCacheSize = 0;
		}
		}
		}

		@@ -242,10 +276,14 @@ public final class ArrayMap<K, V> implements Map<K, V> {
		mArray = new Object[size<<1];
		}

		/**
		* Make sure <b>NOT</b> to call this method with arrays that can still be modified. In other
		* words, don't pass mHashes or mArray in directly.
		*/
		@UnsupportedAppUsage(maxTargetSdk = 28) // Allocations are an implementation detail.
		private static void freeArrays(final int[] hashes, final Object[] array, final int size) {
		if (hashes.length == (BASE_SIZE*2)) {
		synchronized (ArrayMap.class) {
		synchronized (sTwiceBaseCacheLock) {
		if (mTwiceBaseCacheSize < CACHE_SIZE) {
		array[0] = mTwiceBaseCache;
		array[1] = hashes;
		@@ -259,7 +297,7 @@ public final class ArrayMap<K, V> implements Map<K, V> {
		}
		}
		} else if (hashes.length == BASE_SIZE) {
		synchronized (ArrayMap.class) {
		synchronized (sBaseCacheLock) {
		if (mBaseCacheSize < CACHE_SIZE) {
		array[0] = mBaseCache;
		array[1] = hashes;

core/java/android/util/ArraySet.java

+101 −45

Original line number	Diff line number	Diff line
		@@ -23,6 +23,7 @@ import libcore.util.EmptyArray;

		import java.lang.reflect.Array;
		import java.util.Collection;
		import java.util.ConcurrentModificationException;
		import java.util.Iterator;
		import java.util.Map;
		import java.util.Set;
		@@ -46,6 +47,8 @@ import java.util.function.Predicate;
		* you have no control over this shrinking -- if you set a capacity and then remove an
		* item, it may reduce the capacity to better match the current size. In the future an
		* explicit call to set the capacity should turn off this aggressive shrinking behavior.</p>
		*
		* <p>This structure is <b>NOT</b> thread-safe.</p>
		*/
		public final class ArraySet<E> implements Collection<E>, Set<E> {
		private static final boolean DEBUG = false;
		@@ -72,15 +75,30 @@ public final class ArraySet<E> implements Collection<E>, Set<E> {
		static int sBaseCacheSize;
		static Object[] sTwiceBaseCache;
		static int sTwiceBaseCacheSize;
		/**
		* Separate locks for each cache since each can be accessed independently of the other without
		* risk of a deadlock.
		*/
		private static final Object sBaseCacheLock = new Object();
		private static final Object sTwiceBaseCacheLock = new Object();

		final boolean mIdentityHashCode;
		private final boolean mIdentityHashCode;
		@UnsupportedAppUsage(maxTargetSdk = 28) // Hashes are an implementation detail. Use public API.
		int[] mHashes;
		@UnsupportedAppUsage(maxTargetSdk = 28) // Storage is an implementation detail. Use public API.
		Object[] mArray;
		@UnsupportedAppUsage(maxTargetSdk = 28) // Use size()
		int mSize;
		MapCollections<E, E> mCollections;
		private MapCollections<E, E> mCollections;

		private int binarySearch(int[] hashes, int hash) {
		try {
		return ContainerHelpers.binarySearch(hashes, mSize, hash);
		} catch (ArrayIndexOutOfBoundsException e) {
		throw new ConcurrentModificationException();
		}
		}


		@UnsupportedAppUsage(maxTargetSdk = 28) // Hashes are an implementation detail. Use indexOfKey(Object).
		private int indexOf(Object key, int hash) {
		@@ -91,7 +109,7 @@ public final class ArraySet<E> implements Collection<E>, Set<E> {
		return ~0;
		}

		int index = ContainerHelpers.binarySearch(mHashes, N, hash);
		int index = binarySearch(mHashes, hash);

		// If the hash code wasn't found, then we have no entry for this key.
		if (index < 0) {
		@@ -130,7 +148,7 @@ public final class ArraySet<E> implements Collection<E>, Set<E> {
		return ~0;
		}

		int index = ContainerHelpers.binarySearch(mHashes, N, 0);
		int index = binarySearch(mHashes, 0);

		// If the hash code wasn't found, then we have no entry for this key.
		if (index < 0) {
		@@ -163,13 +181,14 @@ public final class ArraySet<E> implements Collection<E>, Set<E> {
		@UnsupportedAppUsage(maxTargetSdk = 28) // Allocations are an implementation detail.
		private void allocArrays(final int size) {
		if (size == (BASE_SIZE * 2)) {
		synchronized (ArraySet.class) {
		synchronized (sTwiceBaseCacheLock) {
		if (sTwiceBaseCache != null) {
		final Object[] array = sTwiceBaseCache;
		try {
		mArray = array;
		sTwiceBaseCache = (Object[]) array[0];
		mHashes = (int[]) array[1];
		if (mHashes != null) {
		array[0] = array[1] = null;
		sTwiceBaseCacheSize--;
		if (DEBUG) {
		@@ -177,6 +196,7 @@ public final class ArraySet<E> implements Collection<E>, Set<E> {
		+ sTwiceBaseCacheSize + " entries");
		}
		return;
		}
		} catch (ClassCastException e) {
		}
		// Whoops! Someone trampled the array (probably due to not protecting
		@@ -188,20 +208,22 @@ public final class ArraySet<E> implements Collection<E>, Set<E> {
		}
		}
		} else if (size == BASE_SIZE) {
		synchronized (ArraySet.class) {
		synchronized (sBaseCacheLock) {
		if (sBaseCache != null) {
		final Object[] array = sBaseCache;
		try {
		mArray = array;
		sBaseCache = (Object[]) array[0];
		mHashes = (int[]) array[1];
		if (mHashes != null) {
		array[0] = array[1] = null;
		sBaseCacheSize--;
		if (DEBUG) {
		Log.d(TAG, "Retrieving 1x cache " + mHashes + " now have " + sBaseCacheSize
		+ " entries");
		Log.d(TAG, "Retrieving 1x cache " + mHashes + " now have "
		+ sBaseCacheSize + " entries");
		}
		return;
		}
		} catch (ClassCastException e) {
		}
		// Whoops! Someone trampled the array (probably due to not protecting
		@@ -218,10 +240,14 @@ public final class ArraySet<E> implements Collection<E>, Set<E> {
		mArray = new Object[size];
		}

		/**
		* Make sure <b>NOT</b> to call this method with arrays that can still be modified. In other
		* words, don't pass mHashes or mArray in directly.
		*/
		@UnsupportedAppUsage(maxTargetSdk = 28) // Allocations are an implementation detail.
		private static void freeArrays(final int[] hashes, final Object[] array, final int size) {
		if (hashes.length == (BASE_SIZE * 2)) {
		synchronized (ArraySet.class) {
		synchronized (sTwiceBaseCacheLock) {
		if (sTwiceBaseCacheSize < CACHE_SIZE) {
		array[0] = sTwiceBaseCache;
		array[1] = hashes;
		@@ -237,7 +263,7 @@ public final class ArraySet<E> implements Collection<E>, Set<E> {
		}
		}
		} else if (hashes.length == BASE_SIZE) {
		synchronized (ArraySet.class) {
		synchronized (sBaseCacheLock) {
		if (sBaseCacheSize < CACHE_SIZE) {
		array[0] = sBaseCache;
		array[1] = hashes;
		@@ -308,10 +334,16 @@ public final class ArraySet<E> implements Collection<E>, Set<E> {
		@Override
		public void clear() {
		if (mSize != 0) {
		freeArrays(mHashes, mArray, mSize);
		final int[] ohashes = mHashes;
		final Object[] oarray = mArray;
		final int osize = mSize;
		mHashes = EmptyArray.INT;
		mArray = EmptyArray.OBJECT;
		mSize = 0;
		freeArrays(ohashes, oarray, osize);
		}
		if (mSize != 0) {
		throw new ConcurrentModificationException();
		}
		}

		@@ -320,6 +352,7 @@ public final class ArraySet<E> implements Collection<E>, Set<E> {
		* items.
		*/
		public void ensureCapacity(int minimumCapacity) {
		final int oSize = mSize;
		if (mHashes.length < minimumCapacity) {
		final int[] ohashes = mHashes;
		final Object[] oarray = mArray;
		@@ -330,6 +363,9 @@ public final class ArraySet<E> implements Collection<E>, Set<E> {
		}
		freeArrays(ohashes, oarray, mSize);
		}
		if (mSize != oSize) {
		throw new ConcurrentModificationException();
		}
		}

		/**
		@@ -400,11 +436,10 @@ public final class ArraySet<E> implements Collection<E>, Set<E> {
		*
		* @param value the object to add.
		* @return {@code true} if this set is modified, {@code false} otherwise.
		* @throws ClassCastException
		* when the class of the object is inappropriate for this set.
		*/
		@Override
		public boolean add(E value) {
		final int oSize = mSize;
		final int hash;
		int index;
		if (value == null) {
		@@ -419,9 +454,9 @@ public final class ArraySet<E> implements Collection<E>, Set<E> {
		}

		index = ~index;
		if (mSize >= mHashes.length) {
		final int n = mSize >= (BASE_SIZE * 2) ? (mSize + (mSize >> 1))
		: (mSize >= BASE_SIZE ? (BASE_SIZE * 2) : BASE_SIZE);
		if (oSize >= mHashes.length) {
		final int n = oSize >= (BASE_SIZE * 2) ? (oSize + (oSize >> 1))
		: (oSize >= BASE_SIZE ? (BASE_SIZE * 2) : BASE_SIZE);

		if (DEBUG) Log.d(TAG, "add: grow from " + mHashes.length + " to " + n);

		@@ -429,21 +464,29 @@ public final class ArraySet<E> implements Collection<E>, Set<E> {
		final Object[] oarray = mArray;
		allocArrays(n);

		if (oSize != mSize) {
		throw new ConcurrentModificationException();
		}

		if (mHashes.length > 0) {
		if (DEBUG) Log.d(TAG, "add: copy 0-" + mSize + " to 0");
		if (DEBUG) Log.d(TAG, "add: copy 0-" + oSize + " to 0");
		System.arraycopy(ohashes, 0, mHashes, 0, ohashes.length);
		System.arraycopy(oarray, 0, mArray, 0, oarray.length);
		}

		freeArrays(ohashes, oarray, mSize);
		freeArrays(ohashes, oarray, oSize);
		}

		if (index < mSize) {
		if (index < oSize) {
		if (DEBUG) {
		Log.d(TAG, "add: move " + index + "-" + (mSize - index) + " to " + (index + 1));
		Log.d(TAG, "add: move " + index + "-" + (oSize - index) + " to " + (index + 1));
		}
		System.arraycopy(mHashes, index, mHashes, index + 1, mSize - index);
		System.arraycopy(mArray, index, mArray, index + 1, mSize - index);
		System.arraycopy(mHashes, index, mHashes, index + 1, oSize - index);
		System.arraycopy(mArray, index, mArray, index + 1, oSize - index);
		}

		if (oSize != mSize \|\| index >= mHashes.length) {
		throw new ConcurrentModificationException();
		}

		mHashes[index] = hash;
		@@ -458,6 +501,7 @@ public final class ArraySet<E> implements Collection<E>, Set<E> {
		* @hide
		*/
		public void append(E value) {
		final int oSize = mSize;
		final int index = mSize;
		final int hash = value == null ? 0
		: (mIdentityHashCode ? System.identityHashCode(value) : value.hashCode());
		@@ -476,6 +520,11 @@ public final class ArraySet<E> implements Collection<E>, Set<E> {
		add(value);
		return;
		}

		if (oSize != mSize) {
		throw new ConcurrentModificationException();
		}

		mSize = index + 1;
		mHashes[index] = hash;
		mArray[index] = value;
		@@ -492,6 +541,9 @@ public final class ArraySet<E> implements Collection<E>, Set<E> {
		if (N > 0) {
		System.arraycopy(array.mHashes, 0, mHashes, 0, N);
		System.arraycopy(array.mArray, 0, mArray, 0, N);
		if (0 != mSize) {
		throw new ConcurrentModificationException();
		}
		mSize = N;
		}
		} else {
		@@ -549,12 +601,14 @@ public final class ArraySet<E> implements Collection<E>, Set<E> {
		// Check if exception should be thrown outside of the critical path.
		throw new ArrayIndexOutOfBoundsException(index);
		}
		final int oSize = mSize;
		final Object old = mArray[index];
		if (mSize <= 1) {
		if (oSize <= 1) {
		// Now empty.
		if (DEBUG) Log.d(TAG, "remove: shrink from " + mHashes.length + " to 0");
		clear();
		} else {
		final int nSize = oSize - 1;
		if (shouldShrink()) {
		// Shrunk enough to reduce size of arrays.
		final int n = getNewShrunkenSize();
		@@ -565,31 +619,33 @@ public final class ArraySet<E> implements Collection<E>, Set<E> {
		final Object[] oarray = mArray;
		allocArrays(n);

		mSize--;
		if (index > 0) {
		if (DEBUG) Log.d(TAG, "remove: copy from 0-" + index + " to 0");
		System.arraycopy(ohashes, 0, mHashes, 0, index);
		System.arraycopy(oarray, 0, mArray, 0, index);
		}
		if (index < mSize) {
		if (index < nSize) {
		if (DEBUG) {
		Log.d(TAG, "remove: copy from " + (index + 1) + "-" + mSize
		Log.d(TAG, "remove: copy from " + (index + 1) + "-" + nSize
		+ " to " + index);
		}
		System.arraycopy(ohashes, index + 1, mHashes, index, mSize - index);
		System.arraycopy(oarray, index + 1, mArray, index, mSize - index);
		System.arraycopy(ohashes, index + 1, mHashes, index, nSize - index);
		System.arraycopy(oarray, index + 1, mArray, index, nSize - index);
		}
		} else {
		mSize--;
		if (index < mSize) {
		if (index < nSize) {
		if (DEBUG) {
		Log.d(TAG, "remove: move " + (index + 1) + "-" + mSize + " to " + index);
		Log.d(TAG, "remove: move " + (index + 1) + "-" + nSize + " to " + index);
		}
		System.arraycopy(mHashes, index + 1, mHashes, index, nSize - index);
		System.arraycopy(mArray, index + 1, mArray, index, nSize - index);
		}
		System.arraycopy(mHashes, index + 1, mHashes, index, mSize - index);
		System.arraycopy(mArray, index + 1, mArray, index, mSize - index);
		mArray[nSize] = null;
		}
		mArray[mSize] = null;
		if (oSize != mSize) {
		throw new ConcurrentModificationException();
		}
		mSize = nSize;
		}
		return (E) old;
		}

core/tests/coretests/src/android/util/ArraySetTest.java

0 → 100644

+113 −0

Original line number	Diff line number	Diff line
		/*
		* Copyright (C) 2019 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.util;

		import androidx.test.filters.LargeTest;

		import junit.framework.TestCase;

		import org.junit.After;
		import org.junit.Test;

		import java.util.ConcurrentModificationException;

		/**
		* Unit tests for ArraySet that don't belong in CTS.
		*/
		@LargeTest
		public class ArraySetTest extends TestCase {
		private static final String TAG = "ArraySetTest";
		ArraySet<String> mSet = new ArraySet<>();

		@After
		public void tearDown() {
		mSet = null;
		}

		/**
		* Attempt to generate a ConcurrentModificationException in ArraySet.
		* <p>
		* ArraySet is explicitly documented to be non-thread-safe, yet it's easy to accidentally screw
		* this up; ArraySet should (in the spirit of the core Java collection types) make an effort to
		* catch this and throw ConcurrentModificationException instead of crashing somewhere in its
		* internals.
		*/
		@Test
		public void testConcurrentModificationException() throws Exception {
		final int testDurMs = 10_000;
		System.out.println("Starting ArraySet concurrency test");
		new Thread(() -> {
		int i = 0;
		while (mSet != null) {
		try {
		mSet.add(String.format("key %d", i++));
		} catch (ArrayIndexOutOfBoundsException e) {
		Log.e(TAG, "concurrent modification uncaught, causing indexing failure", e);
		fail("Concurrent modification uncaught, causing indexing failure: " + e);
		} catch (ClassCastException e) {
		Log.e(TAG, "concurrent modification uncaught, causing cache corruption", e);
		fail("Concurrent modification uncaught, causing cache corruption: " + e);
		} catch (ConcurrentModificationException e) {
		System.out.println("[successfully caught CME at put #" + i
		+ " size=" + (mSet == null ? "??" : String.valueOf(mSet.size())) + "]");
		if (i % 200 == 0) {
		System.out.print(".");
		}
		}
		}
		}).start();
		for (int i = 0; i < (testDurMs / 100); i++) {
		try {
		if (mSet.size() % 4 == 0) {
		mSet.clear();
		}
		System.out.print("X");
		} catch (ArrayIndexOutOfBoundsException e) {
		Log.e(TAG, "concurrent modification uncaught, causing indexing failure", e);
		fail("Concurrent modification uncaught, causing indexing failure: " + e);
		} catch (ClassCastException e) {
		Log.e(TAG, "concurrent modification uncaught, causing cache corruption", e);
		fail("Concurrent modification uncaught, causing cache corruption: " + e);
		} catch (ConcurrentModificationException e) {
		System.out.println(
		"[successfully caught CME at clear #" + i + " size=" + mSet.size() + "]");
		}
		}
		}

		/**
		* Check to make sure the same operations behave as expected in a single thread.
		*/
		@Test
		public void testNonConcurrentAccesses() throws Exception {
		for (int i = 0; i < 100000; i++) {
		try {
		mSet.add(String.format("key %d", i++));
		if (i % 200 == 0) {
		System.out.print(".");
		}
		if (i % 500 == 0) {
		mSet.clear();
		System.out.print("X");
		}
		} catch (ConcurrentModificationException e) {
		Log.e(TAG, "concurrent modification caught on single thread", e);
		fail();
		}
		}
		}
		}