Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit a6b4f410 authored by Lee Shombert's avatar Lee Shombert
Browse files

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
parent de2e7c43
Loading
Loading
Loading
Loading
+62 −36
Original line number Diff line number Diff line
@@ -391,7 +391,12 @@ public class PropertyInvalidatedCache<Query, Result> {
    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<Query, Result> {
    @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<String, Long> 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<String, Long> 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<String, Long> 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<Query, Result> {
    @GuardedBy("sCorkLock")
    private static final HashMap<String, Integer> 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<String> 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<PropertyInvalidatedCache, Void> 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<String, Long> 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<Query, Result> {
        };
    }

    // 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<Query, Result> {
    }

    /**
     * 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<Query, Result> {

    /**
     * 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<Query, Result> {
     */
    @TestApi
    public final void forgetDisableLocal() {
        synchronized (sCorkLock) {
        synchronized (sGlobalLock) {
            sDisabledKeys.remove(mCacheName);
        }
    }
@@ -851,9 +878,9 @@ public class PropertyInvalidatedCache<Query, Result> {
    }

    /**
     * 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,11 +1408,10 @@ public class PropertyInvalidatedCache<Query, Result> {
    /**
     * Returns a list of caches alive at the current time.
     */
    @GuardedBy("sGlobalLock")
    private static @NonNull ArrayList<PropertyInvalidatedCache> getActiveCaches() {
        synchronized (sCorkLock) {
        return new ArrayList<PropertyInvalidatedCache>(sCaches.keySet());
    }
    }

    /**
     * Returns a list of the active corks in a process.
@@ -1540,7 +1566,7 @@ public class PropertyInvalidatedCache<Query, Result> {
        boolean detail = anyDetailed(args);

        ArrayList<PropertyInvalidatedCache> activeCaches;
        synchronized (sCorkLock) {
        synchronized (sGlobalLock) {
            activeCaches = getActiveCaches();
            if (!detail) {
                dumpCorkInfo(pw);