From c1c274f45d2d552dee093dbff472202fe31fb9e9 Mon Sep 17 00:00:00 2001 From: Lee Shombert Date: Fri, 1 Nov 2024 15:48:26 -0700 Subject: Builder-style args for PIC To quell the proliferation of constructor parameters, a new builder-style Args class is provided. This conveys the same parameters as IpcDataCache.Config. All existing constructors have been reflowed to call the constructor that uses Args. IpcDataCache has been changed to call the new Args-style constructor. Unit tests have been added for the new constructor. This refactor is starting to deconstruct the well-known property names into a module and an api name. This organization is already present in IpcDataCache. Dump methods are marked @NeverCompile, in response to an earlier CL commit. A minor reflow makes the query() marginally faster when a cache is disabled. Flag: EXEMPT refactor Bug: 373752556 Test: atest * FrameworksCoreTests:PropertyInvalidatedCacheTests * FrameworksCoreTests:IpcDataCacheTest * CtsOsTestCases:IpcDataCacheTest * ServiceBluetoothTests Change-Id: Id90462431feea858dc8f416a56062d8d66df0c12 --- .../java/android/app/PropertyInvalidatedCache.java | 190 +++++++++++++++------ core/java/android/os/IpcDataCache.java | 6 +- .../android/app/PropertyInvalidatedCacheTests.java | 97 +++++++++-- 3 files changed, 229 insertions(+), 64 deletions(-) diff --git a/core/java/android/app/PropertyInvalidatedCache.java b/core/java/android/app/PropertyInvalidatedCache.java index a39cf84a4ebe..038dcdb7efc9 100644 --- a/core/java/android/app/PropertyInvalidatedCache.java +++ b/core/java/android/app/PropertyInvalidatedCache.java @@ -17,6 +17,7 @@ package android.app; import static android.text.TextUtils.formatSimple; +import static com.android.internal.util.Preconditions.checkArgumentPositive; import android.annotation.NonNull; import android.annotation.Nullable; @@ -40,6 +41,7 @@ import com.android.internal.os.BackgroundThread; import dalvik.annotation.optimization.CriticalNative; import dalvik.annotation.optimization.FastNative; +import dalvik.annotation.optimization.NeverCompile; import java.io.ByteArrayOutputStream; import java.io.FileOutputStream; @@ -200,6 +202,23 @@ public class PropertyInvalidatedCache { return CACHE_KEY_PREFIX + "." + module + "." + new String(suffix); } + /** + * The list of known and legal modules. The list is not sorted. + */ + private static final String[] sValidModule = { + MODULE_SYSTEM, MODULE_BLUETOOTH, MODULE_TELEPHONY, MODULE_TEST, + }; + + /** + * Verify that the module string is in the legal list. Throw if it is not. + */ + private static void throwIfInvalidModule(@NonNull String name) { + for (int i = 0; i < sValidModule.length; i++) { + if (sValidModule[i].equals(name)) return; + } + throw new IllegalArgumentException("invalid module: " + name); + } + /** * All legal keys start with one of the following strings. */ @@ -254,8 +273,11 @@ public class PropertyInvalidatedCache { // written to global store. private static final int NONCE_BYPASS = 3; + // The largest reserved nonce value. Update this whenever a reserved nonce is added. + private static final int MAX_RESERVED_NONCE = NONCE_BYPASS; + private static boolean isReservedNonce(long n) { - return n >= NONCE_UNSET && n <= NONCE_BYPASS; + return n >= NONCE_UNSET && n <= MAX_RESERVED_NONCE; } /** @@ -293,7 +315,7 @@ public class PropertyInvalidatedCache { private long mMisses = 0; @GuardedBy("mLock") - private long[] mSkips = new long[]{ 0, 0, 0, 0 }; + private long[] mSkips = new long[MAX_RESERVED_NONCE + 1]; @GuardedBy("mLock") private long mMissOverflow = 0; @@ -853,6 +875,73 @@ public class PropertyInvalidatedCache { return h; } + /** + * A public argument builder to configure cache behavior. The root instance requires a + * module; this is immutable. New instances are created with member methods. It is important + * to note that the member methods create new instances: they do not modify 'this'. The api + * is allowed to be null in the record constructor to facility reuse of Args instances. + * @hide + */ + public static record Args(@NonNull String mModule, @Nullable String mApi, int mMaxEntries) { + + // Validation: the module must be one of the known module strings and the maxEntries must + // be positive. + public Args { + throwIfInvalidModule(mModule); + checkArgumentPositive(mMaxEntries, "max cache size must be positive"); + } + + // The base constructor must include the module. Modules do not change in a source file, + // so even if the Args is reused, the module will not/should not change. The api is null, + // which is not legal, but there is no reasonable default. Clients must call the api + // method to set the field properly. + public Args(@NonNull String module) { + this(module, /* api */ null, /* maxEntries */ 32); + } + + public Args api(@NonNull String api) { + return new Args(mModule, api, mMaxEntries); + } + + public Args maxEntries(int val) { + return new Args(mModule, mApi, val); + } + } + + /** + * Make a new property invalidated cache. The key is computed from the module and api + * parameters. + * + * @param args The cache configuration. + * @param cacheName Name of this cache in debug and dumpsys + * @param computer The code to compute values that are not in the cache. + * @hide + */ + public PropertyInvalidatedCache(@NonNull Args args, @NonNull String cacheName, + @Nullable QueryHandler computer) { + mPropertyName = createPropertyName(args.mModule, args.mApi); + mCacheName = cacheName; + mNonce = getNonceHandler(mPropertyName); + mMaxEntries = args.mMaxEntries; + mCache = createMap(); + mComputer = (computer != null) ? computer : new DefaultComputer<>(this); + registerCache(); + } + + /** + * Burst a property name into module and api. Throw if the key is invalid. This method is + * used in to transition legacy cache constructors to the args constructor. + */ + private static Args parseProperty(@NonNull String name) { + throwIfInvalidCacheKey(name); + // Strip off the leading well-known prefix. + String base = name.substring(CACHE_KEY_PREFIX.length() + 1); + int dot = base.indexOf("."); + String module = base.substring(0, dot); + String api = base.substring(dot + 1); + return new Args(module).api(api); + } + /** * Make a new property invalidated cache. This constructor names the cache after the * property name. New clients should prefer the constructor that takes an explicit @@ -867,7 +956,7 @@ public class PropertyInvalidatedCache { * @hide */ public PropertyInvalidatedCache(int maxEntries, @NonNull String propertyName) { - this(maxEntries, propertyName, propertyName); + this(parseProperty(propertyName).maxEntries(maxEntries), propertyName, null); } /** @@ -883,13 +972,7 @@ public class PropertyInvalidatedCache { */ public PropertyInvalidatedCache(int maxEntries, @NonNull String propertyName, @NonNull String cacheName) { - mPropertyName = propertyName; - mCacheName = cacheName; - mNonce = getNonceHandler(mPropertyName); - mMaxEntries = maxEntries; - mComputer = new DefaultComputer<>(this); - mCache = createMap(); - registerCache(); + this(parseProperty(propertyName).maxEntries(maxEntries), cacheName, null); } /** @@ -907,13 +990,7 @@ public class PropertyInvalidatedCache { @TestApi public PropertyInvalidatedCache(int maxEntries, @NonNull String module, @NonNull String api, @NonNull String cacheName, @NonNull QueryHandler computer) { - mPropertyName = createPropertyName(module, api); - mCacheName = cacheName; - mNonce = getNonceHandler(mPropertyName); - mMaxEntries = maxEntries; - mComputer = computer; - mCache = createMap(); - registerCache(); + this(new Args(module).maxEntries(maxEntries).api(api), cacheName, computer); } // Create a map. This should be called only from the constructor. @@ -1181,7 +1258,8 @@ public class PropertyInvalidatedCache { public @Nullable Result query(@NonNull Query query) { // Let access to mDisabled race: it's atomic anyway. long currentNonce = (!isDisabled()) ? getCurrentNonce() : NONCE_DISABLED; - if (bypass(query)) { + if (!isReservedNonce(currentNonce) + && bypass(query)) { currentNonce = NONCE_BYPASS; } for (;;) { @@ -1649,54 +1727,62 @@ public class PropertyInvalidatedCache { return false; } - /** - * helper method to check if dump should be skipped due to zero values - * @param args takes command arguments to check if -brief is present - * @return True if dump should be skipped - */ - private boolean skipDump(String[] args) { - for (String a : args) { - if (a.equals(BRIEF)) { - return (mSkips[NONCE_CORKED] + mSkips[NONCE_UNSET] + mSkips[NONCE_DISABLED] - + mSkips[NONCE_BYPASS] + mHits + mMisses) == 0; - } + @GuardedBy("mLock") + private long getSkipsLocked() { + int sum = 0; + for (int i = 0; i < mSkips.length; i++) { + sum += mSkips[i]; } - return false; + return sum; } + // Return true if this cache has had any activity. If the hits, misses, and skips are all + // zero then the client never tried to use the cache. + private boolean isActive() { + synchronized (mLock) { + return mHits + mMisses + getSkipsLocked() > 0; + } + } + + @NeverCompile private void dumpContents(PrintWriter pw, boolean detailed, String[] args) { // If the user has requested specific caches and this is not one of them, return // immediately. if (detailed && !showDetailed(args)) { return; } + // Does the user want brief output? + boolean brief = false; + for (String a : args) brief |= a.equals(BRIEF); NonceHandler.Stats stats = mNonce.getStats(); synchronized (mLock) { - if (!skipDump(args)) { - pw.println(formatSimple(" Cache Name: %s", cacheName())); - pw.println(formatSimple(" Property: %s", mPropertyName)); - final long skips = - mSkips[NONCE_CORKED] + mSkips[NONCE_UNSET] + mSkips[NONCE_DISABLED] - + mSkips[NONCE_BYPASS]; - pw.println(formatSimple( - " Hits: %d, Misses: %d, Skips: %d, Clears: %d", - mHits, mMisses, skips, mClears)); - pw.println(formatSimple( - " Skip-corked: %d, Skip-unset: %d, Skip-bypass: %d, Skip-other: %d", - mSkips[NONCE_CORKED], mSkips[NONCE_UNSET], - mSkips[NONCE_BYPASS], mSkips[NONCE_DISABLED])); - pw.println(formatSimple( - " Nonce: 0x%016x, Invalidates: %d, CorkedInvalidates: %d", - mLastSeenNonce, stats.invalidated, stats.corkedInvalidates)); - pw.println(formatSimple( - " Current Size: %d, Max Size: %d, HW Mark: %d, Overflows: %d", - mCache.size(), mMaxEntries, mHighWaterMark, mMissOverflow)); - pw.println(formatSimple(" Enabled: %s", mDisabled ? "false" : "true")); - pw.println(""); + if (brief && !isActive()) { + return; } + pw.println(formatSimple(" Cache Name: %s", cacheName())); + pw.println(formatSimple(" Property: %s", mPropertyName)); + pw.println(formatSimple( + " Hits: %d, Misses: %d, Skips: %d, Clears: %d", + mHits, mMisses, getSkipsLocked(), mClears)); + + // Print all the skip reasons. + pw.format(" Skip-%s: %d", sNonceName[0], mSkips[0]); + for (int i = 1; i < mSkips.length; i++) { + pw.format(", Skip-%s: %d", sNonceName[i], mSkips[i]); + } + pw.println(); + + pw.println(formatSimple( + " Nonce: 0x%016x, Invalidates: %d, CorkedInvalidates: %d", + mLastSeenNonce, stats.invalidated, stats.corkedInvalidates)); + pw.println(formatSimple( + " Current Size: %d, Max Size: %d, HW Mark: %d, Overflows: %d", + mCache.size(), mMaxEntries, mHighWaterMark, mMissOverflow)); + pw.println(formatSimple(" Enabled: %s", mDisabled ? "false" : "true")); + // No specific cache was requested. This is the default, and no details // should be dumped. if (!detailed) { @@ -1723,6 +1809,7 @@ public class PropertyInvalidatedCache { * specific caches (selection is by cache name or property name); if these switches * are used then the output includes both cache statistics and cache entries. */ + @NeverCompile private static void dumpCacheInfo(@NonNull PrintWriter pw, @NonNull String[] args) { if (!sEnabled) { pw.println(" Caching is disabled in this process."); @@ -1755,6 +1842,7 @@ public class PropertyInvalidatedCache { * are used then the output includes both cache statistics and cache entries. * @hide */ + @NeverCompile public static void dumpCacheInfo(@NonNull ParcelFileDescriptor pfd, @NonNull String[] args) { // Create a PrintWriter that uses a byte array. The code can safely write to // this array without fear of blocking. The completed byte array will be sent diff --git a/core/java/android/os/IpcDataCache.java b/core/java/android/os/IpcDataCache.java index 7875c23be038..8db1567336d3 100644 --- a/core/java/android/os/IpcDataCache.java +++ b/core/java/android/os/IpcDataCache.java @@ -23,6 +23,7 @@ import android.annotation.StringDef; import android.annotation.SystemApi; import android.annotation.TestApi; import android.app.PropertyInvalidatedCache; +import android.app.PropertyInvalidatedCache.Args; import android.text.TextUtils; import android.util.ArraySet; @@ -341,7 +342,7 @@ public class IpcDataCache extends PropertyInvalidatedCache computer) { - super(maxEntries, module, api, cacheName, computer); + super(new Args(module).maxEntries(maxEntries).api(api), cacheName, computer); } /** @@ -563,7 +564,8 @@ public class IpcDataCache extends PropertyInvalidatedCache computer) { - super(config.maxEntries(), config.module(), config.api(), config.name(), computer); + super(new Args(config.module()).maxEntries(config.maxEntries()).api(config.api()), + config.name(), computer); } /** diff --git a/core/tests/coretests/src/android/app/PropertyInvalidatedCacheTests.java b/core/tests/coretests/src/android/app/PropertyInvalidatedCacheTests.java index c2d8f9129e2c..da1fffa0fac4 100644 --- a/core/tests/coretests/src/android/app/PropertyInvalidatedCacheTests.java +++ b/core/tests/coretests/src/android/app/PropertyInvalidatedCacheTests.java @@ -17,6 +17,9 @@ package android.app; import static android.app.PropertyInvalidatedCache.NONCE_UNSET; +import static android.app.PropertyInvalidatedCache.MODULE_BLUETOOTH; +import static android.app.PropertyInvalidatedCache.MODULE_SYSTEM; +import static android.app.PropertyInvalidatedCache.MODULE_TEST; import static android.app.PropertyInvalidatedCache.NonceStore.INVALID_NONCE_INDEX; import static com.android.internal.os.Flags.FLAG_APPLICATION_SHARED_MEMORY_ENABLED; @@ -27,6 +30,8 @@ import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import android.app.PropertyInvalidatedCache.Args; +import android.annotation.SuppressLint; import com.android.internal.os.ApplicationSharedMemory; import android.platform.test.annotations.IgnoreUnderRavenwood; @@ -57,7 +62,7 @@ public class PropertyInvalidatedCacheTests { DeviceFlagsValueProvider.createCheckFlagsRule(); // Configuration for creating caches - private static final String MODULE = PropertyInvalidatedCache.MODULE_TEST; + private static final String MODULE = MODULE_TEST; private static final String API = "testApi"; // This class is a proxy for binder calls. It contains a counter that increments @@ -245,6 +250,12 @@ public class PropertyInvalidatedCacheTests { mQuery = query; } + // Create a cache from the args. The name of the cache is the api. + TestCache(Args args, TestQuery query) { + super(args, args.mApi(), query); + mQuery = query; + } + public int getRecomputeCount() { return mQuery.getRecomputeCount(); } @@ -374,14 +385,11 @@ public class PropertyInvalidatedCacheTests { @Test public void testPropertyNames() { String n1; - n1 = PropertyInvalidatedCache.createPropertyName( - PropertyInvalidatedCache.MODULE_SYSTEM, "getPackageInfo"); + n1 = PropertyInvalidatedCache.createPropertyName(MODULE_SYSTEM, "getPackageInfo"); assertEquals(n1, "cache_key.system_server.get_package_info"); - n1 = PropertyInvalidatedCache.createPropertyName( - PropertyInvalidatedCache.MODULE_SYSTEM, "get_package_info"); + n1 = PropertyInvalidatedCache.createPropertyName(MODULE_SYSTEM, "get_package_info"); assertEquals(n1, "cache_key.system_server.get_package_info"); - n1 = PropertyInvalidatedCache.createPropertyName( - PropertyInvalidatedCache.MODULE_BLUETOOTH, "getState"); + n1 = PropertyInvalidatedCache.createPropertyName(MODULE_BLUETOOTH, "getState"); assertEquals(n1, "cache_key.bluetooth.get_state"); } @@ -391,7 +399,7 @@ public class PropertyInvalidatedCacheTests { reason = "SystemProperties doesn't have permission check") public void testPermissionFailure() { // Create a cache that will write a system nonce. - TestCache sysCache = new TestCache(PropertyInvalidatedCache.MODULE_SYSTEM, "mode1"); + TestCache sysCache = new TestCache(MODULE_SYSTEM, "mode1"); try { // Invalidate the cache, which writes the system property. There must be a permission // failure. @@ -407,7 +415,7 @@ public class PropertyInvalidatedCacheTests { @Test public void testTestMode() { // Create a cache that will write a system nonce. - TestCache sysCache = new TestCache(PropertyInvalidatedCache.MODULE_SYSTEM, "mode1"); + TestCache sysCache = new TestCache(MODULE_SYSTEM, "mode1"); sysCache.testPropertyName(); // Invalidate the cache. This must succeed because the property has been marked for @@ -416,7 +424,7 @@ public class PropertyInvalidatedCacheTests { // Create a cache that uses MODULE_TEST. Invalidation succeeds whether or not the // property is tagged as being tested. - TestCache testCache = new TestCache(PropertyInvalidatedCache.MODULE_TEST, "mode2"); + TestCache testCache = new TestCache(MODULE_TEST, "mode2"); testCache.invalidateCache(); testCache.testPropertyName(); testCache.invalidateCache(); @@ -432,7 +440,7 @@ public class PropertyInvalidatedCacheTests { // The expected exception. } // Configuring a property for testing must fail if test mode is false. - TestCache cache2 = new TestCache(PropertyInvalidatedCache.MODULE_SYSTEM, "mode3"); + TestCache cache2 = new TestCache(MODULE_SYSTEM, "mode3"); try { cache2.testPropertyName(); fail("expected an IllegalStateException"); @@ -444,6 +452,34 @@ public class PropertyInvalidatedCacheTests { PropertyInvalidatedCache.setTestMode(true); } + // Test the Args-style constructor. + @Test + public void testArgsConstructor() { + // Create a cache with a maximum of four entries. + TestCache cache = new TestCache(new Args(MODULE_TEST).api("init1").maxEntries(4), + new TestQuery()); + + cache.invalidateCache(); + for (int i = 1; i <= 4; i++) { + assertEquals("foo" + i, cache.query(i)); + assertEquals(i, cache.getRecomputeCount()); + } + // Everything is in the cache. The recompute count must not increase. + for (int i = 1; i <= 4; i++) { + assertEquals("foo" + i, cache.query(i)); + assertEquals(4, cache.getRecomputeCount()); + } + // Overflow the max entries. The recompute count increases by one. + assertEquals("foo5", cache.query(5)); + assertEquals(5, cache.getRecomputeCount()); + // The oldest entry (1) has been evicted. Iterating through the first four entries will + // sequentially evict them all because the loop is proceeding oldest to newest. + for (int i = 1; i <= 4; i++) { + assertEquals("foo" + i, cache.query(i)); + assertEquals(5+i, cache.getRecomputeCount()); + } + } + // Verify the behavior of shared memory nonce storage. This does not directly test the cache // storing nonces in shared memory. @RequiresFlagsEnabled(FLAG_APPLICATION_SHARED_MEMORY_ENABLED) @@ -495,4 +531,43 @@ public class PropertyInvalidatedCacheTests { shmem.close(); } + + // Verify that an invalid module causes an exception. + private void testInvalidModule(String module) { + try { + @SuppressLint("UnusedVariable") + Args arg = new Args(module); + fail("expected an invalid module exception: module=" + module); + } catch (IllegalArgumentException e) { + // Expected exception. + } + } + + // Test various instantiation errors. The good path is tested in other methods. + @Test + public void testArgumentErrors() { + // Verify that an illegal module throws an exception. + testInvalidModule(MODULE_SYSTEM.substring(0, MODULE_SYSTEM.length() - 1)); + testInvalidModule(MODULE_SYSTEM + "x"); + testInvalidModule("mymodule"); + + // Verify that a negative max entries throws. + Args arg = new Args(MODULE_SYSTEM); + try { + arg.maxEntries(0); + fail("expected an invalid maxEntries exception"); + } catch (IllegalArgumentException e) { + // Expected exception. + } + + // Verify that creating a cache with an invalid property string throws. + try { + final String badKey = "cache_key.volume_list"; + @SuppressLint("UnusedVariable") + var cache = new PropertyInvalidatedCache(4, badKey); + fail("expected bad property exception: prop=" + badKey); + } catch (IllegalArgumentException e) { + // Expected exception. + } + } } -- cgit v1.2.3-59-g8ed1b