From 1056dff9825611400f648aa97a617db5df01af74 Mon Sep 17 00:00:00 2001 From: Jeremy Meyer Date: Thu, 25 May 2023 21:59:12 +0000 Subject: [PATCH] Track generations in resource caches Fixes: 274777221 Test: presubmits (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:599a7f22af5db7004e0a9c586168ae379189fe90) Merged-In: I37dc8d986ff4ae252a2b3f03f48e11a07429df8b Change-Id: I37dc8d986ff4ae252a2b3f03f48e11a07429df8b --- .../android/animation/AnimatorInflater.java | 23 +++--- .../res/ConfigurationBoundResourceCache.java | 12 +-- .../android/content/res/DrawableCache.java | 24 +++++- .../android/content/res/ResourcesImpl.java | 32 +++++--- .../content/res/ThemedResourceCache.java | 47 +++++++++-- .../ConfigurationBoundResourceCacheTest.java | 77 +++++++++++-------- 6 files changed, 144 insertions(+), 71 deletions(-) diff --git a/core/java/android/animation/AnimatorInflater.java b/core/java/android/animation/AnimatorInflater.java index f69bbfd3c53f1..2198fcd706377 100644 --- a/core/java/android/animation/AnimatorInflater.java +++ b/core/java/android/animation/AnimatorInflater.java @@ -111,19 +111,20 @@ public class AnimatorInflater { float pathErrorScale) throws NotFoundException { final ConfigurationBoundResourceCache animatorCache = resources .getAnimatorCache(); - Animator animator = animatorCache.getInstance(id, resources, theme); - if (animator != null) { + ConfigurationBoundResourceCache.Entry animatorEntry = + animatorCache.getInstance(id, resources, theme); + if (animatorEntry.hasValue()) { if (DBG_ANIMATOR_INFLATER) { Log.d(TAG, "loaded animator from cache, " + resources.getResourceName(id)); } - return animator; + return animatorEntry.getValue(); } else if (DBG_ANIMATOR_INFLATER) { Log.d(TAG, "cache miss for animator " + resources.getResourceName(id)); } XmlResourceParser parser = null; try { parser = resources.getAnimation(id); - animator = createAnimatorFromXml(resources, theme, parser, pathErrorScale); + Animator animator = createAnimatorFromXml(resources, theme, parser, pathErrorScale); if (animator != null) { animator.appendChangingConfigurations(getChangingConfigs(resources, id)); final ConstantState constantState = animator.createConstantState(); @@ -131,7 +132,7 @@ public class AnimatorInflater { if (DBG_ANIMATOR_INFLATER) { Log.d(TAG, "caching animator for res " + resources.getResourceName(id)); } - animatorCache.put(id, theme, constantState); + animatorCache.put(id, theme, constantState, animatorEntry.getGeneration()); // create a new animator so that cached version is never used by the user animator = constantState.newInstance(resources, theme); } @@ -160,20 +161,22 @@ public class AnimatorInflater { final ConfigurationBoundResourceCache cache = resources .getStateListAnimatorCache(); final Theme theme = context.getTheme(); - StateListAnimator animator = cache.getInstance(id, resources, theme); - if (animator != null) { - return animator; + ConfigurationBoundResourceCache.Entry animatorEntry = + cache.getInstance(id, resources, theme); + if (animatorEntry.hasValue()) { + return animatorEntry.getValue(); } XmlResourceParser parser = null; try { parser = resources.getAnimation(id); - animator = createStateListAnimatorFromXml(context, parser, Xml.asAttributeSet(parser)); + StateListAnimator animator = + createStateListAnimatorFromXml(context, parser, Xml.asAttributeSet(parser)); if (animator != null) { animator.appendChangingConfigurations(getChangingConfigs(resources, id)); final ConstantState constantState = animator .createConstantState(); if (constantState != null) { - cache.put(id, theme, constantState); + cache.put(id, theme, constantState, animatorEntry.getGeneration()); // return a clone so that the animator in constant state is never used. animator = constantState.newInstance(resources, theme); } diff --git a/core/java/android/content/res/ConfigurationBoundResourceCache.java b/core/java/android/content/res/ConfigurationBoundResourceCache.java index 5e10a57683588..4da3c18883bdd 100644 --- a/core/java/android/content/res/ConfigurationBoundResourceCache.java +++ b/core/java/android/content/res/ConfigurationBoundResourceCache.java @@ -37,16 +37,16 @@ public class ConfigurationBoundResourceCache extends ThemedResourceCache entry = get(key, theme); - if (entry != null) { - return entry.newInstance(resources, theme); + public Entry getInstance(long key, Resources resources, Resources.Theme theme) { + final Entry> e = get(key, theme); + if (e.hasValue()) { + return new Entry<>(e.getValue().newInstance(resources, theme), e.getGeneration()); } - return null; + return new Entry<>(null, e.getGeneration()); } @Override diff --git a/core/java/android/content/res/DrawableCache.java b/core/java/android/content/res/DrawableCache.java index d0ebe3304065e..b51d14a3c472c 100644 --- a/core/java/android/content/res/DrawableCache.java +++ b/core/java/android/content/res/DrawableCache.java @@ -40,14 +40,32 @@ class DrawableCache extends ThemedResourceCache { */ @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.R, trackingBug = 170729553) public Drawable getInstance(long key, Resources resources, Resources.Theme theme) { - final Drawable.ConstantState entry = get(key, theme); - if (entry != null) { - return entry.newDrawable(resources, theme); + final Entry entry = get(key, theme); + if (entry.getValue() != null) { + return entry.getValue().newDrawable(resources, theme); } return null; } + /** + * If the resource is cached, creates and returns a new instance of it. + * + * @param key a key that uniquely identifies the drawable resource + * @param resources a Resources object from which to create new instances. + * @param theme the theme where the resource will be used + * @return an Entry wrapping a a new instance of the resource, or {@code null} if not in + * the cache + */ + public Entry getDrawable(long key, Resources resources, Resources.Theme theme) { + final Entry e = get(key, theme); + if (e.hasValue()) { + return new Entry<>(e.getValue().newDrawable(resources, theme), e.getGeneration()); + } + + return new Entry<>(null, e.getGeneration()); + } + @Override public boolean shouldInvalidateEntry(Drawable.ConstantState entry, int configChanges) { return Configuration.needNewResources(configChanges, entry.getChangingConfigurations()); diff --git a/core/java/android/content/res/ResourcesImpl.java b/core/java/android/content/res/ResourcesImpl.java index 3a2863e5636d2..25154d5c16238 100644 --- a/core/java/android/content/res/ResourcesImpl.java +++ b/core/java/android/content/res/ResourcesImpl.java @@ -650,15 +650,21 @@ public class ResourcesImpl { key = (((long) value.assetCookie) << 32) | value.data; } + int cacheGeneration; // First, check whether we have a cached version of this drawable // that was inflated against the specified theme. Skip the cache if // we're currently preloading or we're not using the cache. if (!mPreloading && useCache) { - final Drawable cachedDrawable = caches.getInstance(key, wrapper, theme); - if (cachedDrawable != null) { - cachedDrawable.setChangingConfigurations(value.changingConfigurations); - return cachedDrawable; + final ThemedResourceCache.Entry cachedDrawable = + caches.getDrawable(key, wrapper, theme); + if (cachedDrawable.hasValue()) { + cachedDrawable.getValue().setChangingConfigurations( + value.changingConfigurations); + return cachedDrawable.getValue(); } + cacheGeneration = cachedDrawable.getGeneration(); + } else { + cacheGeneration = ThemedResourceCache.UNDEFINED_GENERATION; } // Next, check preloaded drawables. Preloaded drawables may contain @@ -702,7 +708,8 @@ public class ResourcesImpl { if (dr != null) { dr.setChangingConfigurations(value.changingConfigurations); if (useCache) { - cacheDrawable(value, isColorDrawable, caches, theme, canApplyTheme, key, dr); + cacheDrawable(value, isColorDrawable, caches, theme, canApplyTheme, key, dr, + cacheGeneration); if (needsNewDrawableAfterCache) { Drawable.ConstantState state = dr.getConstantState(); if (state != null) { @@ -733,7 +740,7 @@ public class ResourcesImpl { } private void cacheDrawable(TypedValue value, boolean isColorDrawable, DrawableCache caches, - Resources.Theme theme, boolean usesTheme, long key, Drawable dr) { + Resources.Theme theme, boolean usesTheme, long key, Drawable dr, int cacheGeneration) { final Drawable.ConstantState cs = dr.getConstantState(); if (cs == null) { return; @@ -761,7 +768,7 @@ public class ResourcesImpl { } } else { synchronized (mAccessLock) { - caches.put(key, theme, cs, usesTheme); + caches.put(key, theme, cs, cacheGeneration, usesTheme); } } } @@ -1002,14 +1009,16 @@ public class ResourcesImpl { TypedValue value, int id) { final long key = (((long) value.assetCookie) << 32) | value.data; final ConfigurationBoundResourceCache cache = mComplexColorCache; - ComplexColor complexColor = cache.getInstance(key, wrapper, theme); - if (complexColor != null) { - return complexColor; + ThemedResourceCache.Entry complexColorEntry = + cache.getInstance(key, wrapper, theme); + if (complexColorEntry.hasValue()) { + return complexColorEntry.getValue(); } final android.content.res.ConstantState factory = sPreloadedComplexColors.get(key); + ComplexColor complexColor = null; if (factory != null) { complexColor = factory.newInstance(wrapper, theme); } @@ -1026,7 +1035,8 @@ public class ResourcesImpl { sPreloadedComplexColors.put(key, complexColor.getConstantState()); } } else { - cache.put(key, theme, complexColor.getConstantState()); + cache.put(key, theme, complexColor.getConstantState(), + complexColorEntry.getGeneration()); } } return complexColor; diff --git a/core/java/android/content/res/ThemedResourceCache.java b/core/java/android/content/res/ThemedResourceCache.java index 3270944ce7e37..e7fd2755a9417 100644 --- a/core/java/android/content/res/ThemedResourceCache.java +++ b/core/java/android/content/res/ThemedResourceCache.java @@ -33,11 +33,37 @@ import java.lang.ref.WeakReference; * @param type of data to cache */ abstract class ThemedResourceCache { + public static final int UNDEFINED_GENERATION = -1; @UnsupportedAppUsage private ArrayMap>> mThemedEntries; private LongSparseArray> mUnthemedEntries; private LongSparseArray> mNullThemedEntries; + private int mGeneration; + + public static class Entry { + private S mValue; + private int mGeneration; + + + public S getValue() { + return mValue; + } + + public boolean hasValue() { + return mValue != null; + } + + public int getGeneration() { + return mGeneration; + } + + Entry(S value, int generation) { + this.mValue = value; + this.mGeneration = generation; + } + } + /** * Adds a new theme-dependent entry to the cache. * @@ -45,9 +71,10 @@ abstract class ThemedResourceCache { * @param theme the theme against which this entry was inflated, or * {@code null} if the entry has no theme applied * @param entry the entry to cache + * @param generation The generation of the cache to compare against before storing */ - public void put(long key, @Nullable Theme theme, @NonNull T entry) { - put(key, theme, entry, true); + public void put(long key, @Nullable Theme theme, @NonNull T entry, int generation) { + put(key, theme, entry, generation, true); } /** @@ -57,10 +84,12 @@ abstract class ThemedResourceCache { * @param theme the theme against which this entry was inflated, or * {@code null} if the entry has no theme applied * @param entry the entry to cache + * @param generation The generation of the cache to compare against before storing * @param usesTheme {@code true} if the entry is affected theme changes, * {@code false} otherwise */ - public void put(long key, @Nullable Theme theme, @NonNull T entry, boolean usesTheme) { + public void put(long key, @Nullable Theme theme, @NonNull T entry, int generation, + boolean usesTheme) { if (entry == null) { return; } @@ -72,7 +101,8 @@ abstract class ThemedResourceCache { } else { entries = getThemedLocked(theme, true); } - if (entries != null) { + if (entries != null + && ((generation == mGeneration) || (generation == UNDEFINED_GENERATION))) { entries.put(key, new WeakReference<>(entry)); } } @@ -86,7 +116,7 @@ abstract class ThemedResourceCache { * @return a cached entry, or {@code null} if not in the cache */ @Nullable - public T get(long key, @Nullable Theme theme) { + public Entry get(long key, @Nullable Theme theme) { // The themed (includes null-themed) and unthemed caches are mutually // exclusive, so we'll give priority to whichever one we think we'll // hit first. Since most of the framework drawables are themed, that's @@ -96,7 +126,7 @@ abstract class ThemedResourceCache { if (themedEntries != null) { final WeakReference themedEntry = themedEntries.get(key); if (themedEntry != null) { - return themedEntry.get(); + return new Entry(themedEntry.get(), mGeneration); } } @@ -104,12 +134,12 @@ abstract class ThemedResourceCache { if (unthemedEntries != null) { final WeakReference unthemedEntry = unthemedEntries.get(key); if (unthemedEntry != null) { - return unthemedEntry.get(); + return new Entry(unthemedEntry.get(), mGeneration); } } } - return null; + return new Entry(null, mGeneration); } /** @@ -121,6 +151,7 @@ abstract class ThemedResourceCache { @UnsupportedAppUsage public void onConfigurationChange(@Config int configChanges) { prune(configChanges); + mGeneration++; } /** diff --git a/core/tests/coretests/src/android/content/res/ConfigurationBoundResourceCacheTest.java b/core/tests/coretests/src/android/content/res/ConfigurationBoundResourceCacheTest.java index 4f8b85554f5c2..b5f18c26dc972 100644 --- a/core/tests/coretests/src/android/content/res/ConfigurationBoundResourceCacheTest.java +++ b/core/tests/coretests/src/android/content/res/ConfigurationBoundResourceCacheTest.java @@ -45,36 +45,40 @@ public class ConfigurationBoundResourceCacheTest @SmallTest public void testGetEmpty() { final Resources res = getActivity().getResources(); - assertNull(mCache.getInstance(-1, res, null)); + assertNull(mCache.getInstance(-1, res, null).getValue()); } @SmallTest public void testSetGet() { - mCache.put(1, null, new DummyFloatConstantState(5f)); + mCache.put(1, null, new DummyFloatConstantState(5f), + ThemedResourceCache.UNDEFINED_GENERATION); final Resources res = getActivity().getResources(); - assertEquals(5f, mCache.getInstance(1, res, null)); - assertNotSame(5f, mCache.getInstance(1, res, null)); - assertEquals(null, mCache.getInstance(1, res, getActivity().getTheme())); + assertEquals(5f, mCache.getInstance(1, res, null).getValue()); + assertNotSame(5f, mCache.getInstance(1, res, null).getValue()); + assertEquals(false, mCache.getInstance(1, res, getActivity().getTheme()).hasValue()); } @SmallTest public void testSetGetThemed() { - mCache.put(1, getActivity().getTheme(), new DummyFloatConstantState(5f)); + mCache.put(1, getActivity().getTheme(), new DummyFloatConstantState(5f), + ThemedResourceCache.UNDEFINED_GENERATION); final Resources res = getActivity().getResources(); - assertEquals(null, mCache.getInstance(1, res, null)); - assertEquals(5f, mCache.getInstance(1, res, getActivity().getTheme())); - assertNotSame(5f, mCache.getInstance(1, res, getActivity().getTheme())); + assertEquals(false, mCache.getInstance(1, res, null).hasValue()); + assertEquals(5f, mCache.getInstance(1, res, getActivity().getTheme()).getValue()); + assertNotSame(5f, mCache.getInstance(1, res, getActivity().getTheme()).getValue()); } @SmallTest public void testMultiThreadPutGet() { - mCache.put(1, getActivity().getTheme(), new DummyFloatConstantState(5f)); - mCache.put(1, null, new DummyFloatConstantState(10f)); + mCache.put(1, getActivity().getTheme(), new DummyFloatConstantState(5f), + ThemedResourceCache.UNDEFINED_GENERATION); + mCache.put(1, null, new DummyFloatConstantState(10f), + ThemedResourceCache.UNDEFINED_GENERATION); final Resources res = getActivity().getResources(); - assertEquals(10f, mCache.getInstance(1, res, null)); - assertNotSame(10f, mCache.getInstance(1, res, null)); - assertEquals(5f, mCache.getInstance(1, res, getActivity().getTheme())); - assertNotSame(5f, mCache.getInstance(1, res, getActivity().getTheme())); + assertEquals(10f, mCache.getInstance(1, res, null).getValue()); + assertNotSame(10f, mCache.getInstance(1, res, null).getValue()); + assertEquals(5f, mCache.getInstance(1, res, getActivity().getTheme()).getValue()); + assertNotSame(5f, mCache.getInstance(1, res, getActivity().getTheme()).getValue()); } @SmallTest @@ -86,16 +90,17 @@ public class ConfigurationBoundResourceCacheTest res.getValue(R.dimen.resource_cache_test_generic, staticValue, true); float staticDim = TypedValue.complexToDimension(staticValue.data, res.getDisplayMetrics()); mCache.put(key, getActivity().getTheme(), - new DummyFloatConstantState(staticDim, staticValue.changingConfigurations)); + new DummyFloatConstantState(staticDim, staticValue.changingConfigurations), + ThemedResourceCache.UNDEFINED_GENERATION); final Configuration cfg = res.getConfiguration(); Configuration newCnf = new Configuration(cfg); newCnf.orientation = cfg.orientation == Configuration.ORIENTATION_LANDSCAPE ? Configuration.ORIENTATION_PORTRAIT : Configuration.ORIENTATION_LANDSCAPE; int changes = calcConfigChanges(res, newCnf); - assertEquals(staticDim, mCache.getInstance(key, res, getActivity().getTheme())); + assertEquals(staticDim, mCache.getInstance(key, res, getActivity().getTheme()).getValue()); mCache.onConfigurationChange(changes); - assertEquals(staticDim, mCache.getInstance(key, res, getActivity().getTheme())); + assertEquals(staticDim, mCache.getInstance(key, res, getActivity().getTheme()).getValue()); } @SmallTest @@ -108,7 +113,8 @@ public class ConfigurationBoundResourceCacheTest float changingDim = TypedValue.complexToDimension(changingValue.data, res.getDisplayMetrics()); mCache.put(key, getActivity().getTheme(), - new DummyFloatConstantState(changingDim, changingValue.changingConfigurations)); + new DummyFloatConstantState(changingDim, changingValue.changingConfigurations), + ThemedResourceCache.UNDEFINED_GENERATION); final Configuration cfg = res.getConfiguration(); Configuration newCnf = new Configuration(cfg); @@ -116,9 +122,10 @@ public class ConfigurationBoundResourceCacheTest Configuration.ORIENTATION_PORTRAIT : Configuration.ORIENTATION_LANDSCAPE; int changes = calcConfigChanges(res, newCnf); - assertEquals(changingDim, mCache.getInstance(key, res, getActivity().getTheme())); + assertEquals(changingDim, + mCache.getInstance(key, res, getActivity().getTheme()).getValue()); mCache.onConfigurationChange(changes); - assertNull(mCache.get(key, getActivity().getTheme())); + assertNull(mCache.get(key, getActivity().getTheme()).getValue()); } @SmallTest @@ -133,9 +140,11 @@ public class ConfigurationBoundResourceCacheTest float changingDim = TypedValue.complexToDimension(changingValue.data, res.getDisplayMetrics()); mCache.put(R.dimen.resource_cache_test_generic, getActivity().getTheme(), - new DummyFloatConstantState(staticDim, staticValue.changingConfigurations)); + new DummyFloatConstantState(staticDim, staticValue.changingConfigurations), + ThemedResourceCache.UNDEFINED_GENERATION); mCache.put(R.dimen.resource_cache_test_orientation_dependent, getActivity().getTheme(), - new DummyFloatConstantState(changingDim, changingValue.changingConfigurations)); + new DummyFloatConstantState(changingDim, changingValue.changingConfigurations), + ThemedResourceCache.UNDEFINED_GENERATION); final Configuration cfg = res.getConfiguration(); Configuration newCnf = new Configuration(cfg); newCnf.orientation = cfg.orientation == Configuration.ORIENTATION_LANDSCAPE ? @@ -143,15 +152,15 @@ public class ConfigurationBoundResourceCacheTest : Configuration.ORIENTATION_LANDSCAPE; int changes = calcConfigChanges(res, newCnf); assertEquals(staticDim, mCache.getInstance(R.dimen.resource_cache_test_generic, res, - getActivity().getTheme())); + getActivity().getTheme()).getValue()); assertEquals(changingDim, mCache.getInstance(R.dimen.resource_cache_test_orientation_dependent, res, - getActivity().getTheme())); + getActivity().getTheme()).getValue()); mCache.onConfigurationChange(changes); assertEquals(staticDim, mCache.getInstance(R.dimen.resource_cache_test_generic, res, - getActivity().getTheme())); + getActivity().getTheme()).getValue()); assertNull(mCache.getInstance(R.dimen.resource_cache_test_orientation_dependent, res, - getActivity().getTheme())); + getActivity().getTheme()).getValue()); } @SmallTest @@ -173,10 +182,12 @@ public class ConfigurationBoundResourceCacheTest res.getDisplayMetrics()); final Resources.Theme theme = i == 0 ? getActivity().getTheme() : null; mCache.put(R.dimen.resource_cache_test_generic, theme, - new DummyFloatConstantState(staticDim, staticValues[i].changingConfigurations)); + new DummyFloatConstantState(staticDim, staticValues[i].changingConfigurations), + ThemedResourceCache.UNDEFINED_GENERATION); mCache.put(R.dimen.resource_cache_test_orientation_dependent, theme, new DummyFloatConstantState(changingDim, - changingValues[i].changingConfigurations)); + changingValues[i].changingConfigurations), + ThemedResourceCache.UNDEFINED_GENERATION); } final Configuration cfg = res.getConfiguration(); Configuration newCnf = new Configuration(cfg); @@ -187,18 +198,18 @@ public class ConfigurationBoundResourceCacheTest for (int i = 0; i < 2; i++) { final Resources.Theme theme = i == 0 ? getActivity().getTheme() : null; assertEquals(staticDim, - mCache.getInstance(R.dimen.resource_cache_test_generic, res, theme)); + mCache.getInstance(R.dimen.resource_cache_test_generic, res, theme).getValue()); assertEquals(changingDim, mCache.getInstance(R.dimen.resource_cache_test_orientation_dependent, res, - theme)); + theme).getValue()); } mCache.onConfigurationChange(changes); for (int i = 0; i < 2; i++) { final Resources.Theme theme = i == 0 ? getActivity().getTheme() : null; assertEquals(staticDim, - mCache.getInstance(R.dimen.resource_cache_test_generic, res, theme)); + mCache.getInstance(R.dimen.resource_cache_test_generic, res, theme).getValue()); assertNull(mCache.getInstance(R.dimen.resource_cache_test_orientation_dependent, res, - theme)); + theme).getValue()); } }