From a6b4f410f286a5385acdbbd5437b374b02883abd Mon Sep 17 00:00:00 2001 From: Lee Shombert Date: Thu, 8 Sep 2022 13:09:35 -0700 Subject: Refactor PropertyInvalidatedCache locks Bug: 241763149 Refactor the sCorkLock into sCorkLock, which protects code that specifically supports corking, and sGlobalLock, which protects all other code that touches global objects (like the list of all caches). Deadlock was not actually observed in the bug and has never been reported, but the possibility was identified and is being fixed here. Bypass the bulk of disableLocal(name) if the name has already been bypassed. This speeds repeated calls to disableLocal() by not iterating over sCaches. Many threads were terminated by a watchdog while iterating over sCaches inside disableLocal(). It is not known why this block of code was running so often when the time out expired. An open question is whether or not disabled caches should be removed from the sCaches array. The only reason to leave them in the array is to report on them during dumpsys, but this seems like a small benefit Test: atest * FrameworksCoreTests:IpcDataCacheTest * FrameworksCoreTests:PropertyInvalidatedCacheTests * CtsOsTestCases:IpcDataCacheTest Change-Id: I4c298f380044c0be93cdb47d66d7d7e2b260004f --- .../java/android/app/PropertyInvalidatedCache.java | 98 ++++++++++++++-------- 1 file changed, 62 insertions(+), 36 deletions(-) diff --git a/core/java/android/app/PropertyInvalidatedCache.java b/core/java/android/app/PropertyInvalidatedCache.java index 13934e592d40..a51b9d3956df 100644 --- a/core/java/android/app/PropertyInvalidatedCache.java +++ b/core/java/android/app/PropertyInvalidatedCache.java @@ -391,7 +391,12 @@ public class PropertyInvalidatedCache { private static final boolean DEBUG = false; private static final boolean VERIFY = false; - // Per-Cache performance counters. As some cache instances are declared static, + /** + * The object-private lock. + */ + private final Object mLock = new Object(); + + // Per-Cache performance counters. @GuardedBy("mLock") private long mHits = 0; @@ -410,25 +415,19 @@ public class PropertyInvalidatedCache { @GuardedBy("mLock") private long mClears = 0; - // Most invalidation is done in a static context, so the counters need to be accessible. - @GuardedBy("sCorkLock") - private static final HashMap sInvalidates = new HashMap<>(); - /** - * Record the number of invalidate or cork calls that were nops because - * the cache was already corked. This is static because invalidation is - * done in a static context. + * Protect objects that support corking. mLock and sGlobalLock must never be taken while this + * is held. */ - @GuardedBy("sCorkLock") - private static final HashMap sCorkedInvalidates = new HashMap<>(); + private static final Object sCorkLock = new Object(); /** - * If sEnabled is false then all cache operations are stubbed out. Set - * it to false inside test processes. + * Record the number of invalidate or cork calls that were nops because the cache was already + * corked. This is static because invalidation is done in a static context. Entries are + * indexed by the cache property. */ - private static boolean sEnabled = true; - - private static final Object sCorkLock = new Object(); + @GuardedBy("sCorkLock") + private static final HashMap sCorkedInvalidates = new HashMap<>(); /** * A map of cache keys that we've "corked". (The values are counts.) When a cache key is @@ -439,22 +438,40 @@ public class PropertyInvalidatedCache { @GuardedBy("sCorkLock") private static final HashMap sCorks = new HashMap<>(); + /** + * A lock for the global list of caches and cache keys. This must never be taken inside mLock + * or sCorkLock. + */ + private static final Object sGlobalLock = new Object(); + /** * A map of cache keys that have been disabled in the local process. When a key is * disabled locally, existing caches are disabled and the key is saved in this map. * Future cache instances that use the same key will be disabled in their constructor. */ - @GuardedBy("sCorkLock") + @GuardedBy("sGlobalLock") private static final HashSet sDisabledKeys = new HashSet<>(); /** * Weakly references all cache objects in the current process, allowing us to iterate over * them all for purposes like issuing debug dumps and reacting to memory pressure. */ - @GuardedBy("sCorkLock") + @GuardedBy("sGlobalLock") private static final WeakHashMap sCaches = new WeakHashMap<>(); - private final Object mLock = new Object(); + /** + * Counts of the number of times a cache key was invalidated. Invalidation occurs in a static + * context with no cache object available, so this is a static map. Entries are indexed by + * the cache property. + */ + @GuardedBy("sGlobalLock") + private static final HashMap sInvalidates = new HashMap<>(); + + /** + * If sEnabled is false then all cache operations are stubbed out. Set + * it to false inside test processes. + */ + private static boolean sEnabled = true; /** * Name of the property that holds the unique value that we use to invalidate the cache. @@ -595,14 +612,17 @@ public class PropertyInvalidatedCache { }; } - // Register the map in the global list. If the cache is disabled globally, disable it - // now. + /** + * Register the map in the global list. If the cache is disabled globally, disable it + * now. This method is only ever called from the constructor, which means no other thread has + * access to the object yet. It can safely be modified outside any lock. + */ private void registerCache() { - synchronized (sCorkLock) { - sCaches.put(this, null); + synchronized (sGlobalLock) { if (sDisabledKeys.contains(mCacheName)) { disableInstance(); } + sCaches.put(this, null); } } @@ -797,8 +817,9 @@ public class PropertyInvalidatedCache { } /** - * Disable the use of this cache in this process. This method is using during - * testing. To disable a cache in normal code, use disableLocal(). + * Disable the use of this cache in this process. This method is using internally and during + * testing. To disable a cache in normal code, use disableLocal(). A disabled cache cannot + * be re-enabled. * @hide */ @TestApi @@ -811,17 +832,23 @@ public class PropertyInvalidatedCache { /** * Disable the local use of all caches with the same name. All currently registered caches - * using the key will be disabled now, and all future cache instances that use the key will be - * disabled in their constructor. + * with the name will be disabled now, and all future cache instances that use the name will + * be disabled in their constructor. */ private static final void disableLocal(@NonNull String name) { - synchronized (sCorkLock) { - sDisabledKeys.add(name); + synchronized (sGlobalLock) { + if (sDisabledKeys.contains(name)) { + // The key is already in recorded so there is no further work to be done. + return; + } for (PropertyInvalidatedCache cache : sCaches.keySet()) { if (name.equals(cache.mCacheName)) { cache.disableInstance(); } } + // Record the disabled key after the iteration. If an exception occurs during the + // iteration above, and the code is retried, the function should not exit early. + sDisabledKeys.add(name); } } @@ -834,7 +861,7 @@ public class PropertyInvalidatedCache { */ @TestApi public final void forgetDisableLocal() { - synchronized (sCorkLock) { + synchronized (sGlobalLock) { sDisabledKeys.remove(mCacheName); } } @@ -851,9 +878,9 @@ public class PropertyInvalidatedCache { } /** - * Disable this cache in the current process, and all other caches that use the same - * name. This does not affect caches that have a different name but use the same - * property. + * Disable this cache in the current process, and all other present and future caches that use + * the same name. This does not affect caches that have a different name but use the same + * property. Once disabled, a cache cannot be reenabled. * @hide */ @TestApi @@ -1381,10 +1408,9 @@ public class PropertyInvalidatedCache { /** * Returns a list of caches alive at the current time. */ + @GuardedBy("sGlobalLock") private static @NonNull ArrayList getActiveCaches() { - synchronized (sCorkLock) { - return new ArrayList(sCaches.keySet()); - } + return new ArrayList(sCaches.keySet()); } /** @@ -1540,7 +1566,7 @@ public class PropertyInvalidatedCache { boolean detail = anyDetailed(args); ArrayList activeCaches; - synchronized (sCorkLock) { + synchronized (sGlobalLock) { activeCaches = getActiveCaches(); if (!detail) { dumpCorkInfo(pw); -- cgit v1.2.3-59-g8ed1b