From a9ef8767e2c800fd3ad15e746543456963260859 Mon Sep 17 00:00:00 2001 From: Jeremy Meyer Date: Thu, 8 Jun 2023 22:54:30 +0000 Subject: [PATCH] Improve performance of generations in the resource cache Tracking generations in the resouce cache caused a performance regression, likely from allocating an Entry object for every resource retrieval. This optimizes that by having the retrieval of the value be separate from retrieval of the generation. This makes it so that on cache hits there is no difference from before the generation change and a cache miss only adds the overhead of a method call to retrieve an int. Test: ran with forest to determine improvement Fixes: 285575799, 285489092, 285730765, 285607129, 285489541, 285943600 (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:094f7289f1ae341bbb9fdbb17f2aff66e507dbfd) Merged-In: I5144f08bb38d0e600b7f556df834292e50afb91c Change-Id: I5144f08bb38d0e600b7f556df834292e50afb91c --- .../android/animation/AnimatorInflater.java | 24 +++++----- .../res/ConfigurationBoundResourceCache.java | 12 ++--- .../android/content/res/DrawableCache.java | 24 ++-------- .../android/content/res/ResourcesImpl.java | 27 ++++------- .../content/res/ThemedResourceCache.java | 41 ++++++----------- .../ConfigurationBoundResourceCacheTest.java | 46 +++++++++---------- 6 files changed, 68 insertions(+), 106 deletions(-) diff --git a/core/java/android/animation/AnimatorInflater.java b/core/java/android/animation/AnimatorInflater.java index 2198fcd706377..f67c68eb044d2 100644 --- a/core/java/android/animation/AnimatorInflater.java +++ b/core/java/android/animation/AnimatorInflater.java @@ -111,20 +111,20 @@ public class AnimatorInflater { float pathErrorScale) throws NotFoundException { final ConfigurationBoundResourceCache animatorCache = resources .getAnimatorCache(); - ConfigurationBoundResourceCache.Entry animatorEntry = - animatorCache.getInstance(id, resources, theme); - if (animatorEntry.hasValue()) { + Animator animator = animatorCache.getInstance(id, resources, theme); + if (animator != null) { if (DBG_ANIMATOR_INFLATER) { Log.d(TAG, "loaded animator from cache, " + resources.getResourceName(id)); } - return animatorEntry.getValue(); + return animator; } else if (DBG_ANIMATOR_INFLATER) { Log.d(TAG, "cache miss for animator " + resources.getResourceName(id)); } + int cacheGeneration = animatorCache.getGeneration(); XmlResourceParser parser = null; try { parser = resources.getAnimation(id); - Animator animator = createAnimatorFromXml(resources, theme, parser, pathErrorScale); + animator = createAnimatorFromXml(resources, theme, parser, pathErrorScale); if (animator != null) { animator.appendChangingConfigurations(getChangingConfigs(resources, id)); final ConstantState constantState = animator.createConstantState(); @@ -132,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, animatorEntry.getGeneration()); + animatorCache.put(id, theme, constantState, cacheGeneration); // create a new animator so that cached version is never used by the user animator = constantState.newInstance(resources, theme); } @@ -161,22 +161,22 @@ public class AnimatorInflater { final ConfigurationBoundResourceCache cache = resources .getStateListAnimatorCache(); final Theme theme = context.getTheme(); - ConfigurationBoundResourceCache.Entry animatorEntry = - cache.getInstance(id, resources, theme); - if (animatorEntry.hasValue()) { - return animatorEntry.getValue(); + StateListAnimator animator = cache.getInstance(id, resources, theme); + if (animator != null) { + return animator; } + int cacheGeneration = cache.getGeneration(); XmlResourceParser parser = null; try { parser = resources.getAnimation(id); - StateListAnimator animator = + 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, animatorEntry.getGeneration()); + cache.put(id, theme, constantState, cacheGeneration); // 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 4da3c18883bdd..5e10a57683588 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 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()); + public T getInstance(long key, Resources resources, Resources.Theme theme) { + final ConstantState entry = get(key, theme); + if (entry != null) { + return entry.newInstance(resources, theme); } - return new Entry<>(null, e.getGeneration()); + return null; } @Override diff --git a/core/java/android/content/res/DrawableCache.java b/core/java/android/content/res/DrawableCache.java index b51d14a3c472c..d0ebe3304065e 100644 --- a/core/java/android/content/res/DrawableCache.java +++ b/core/java/android/content/res/DrawableCache.java @@ -40,32 +40,14 @@ class DrawableCache extends ThemedResourceCache { */ @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.R, trackingBug = 170729553) public Drawable getInstance(long key, Resources resources, Resources.Theme theme) { - final Entry entry = get(key, theme); - if (entry.getValue() != null) { - return entry.getValue().newDrawable(resources, theme); + final Drawable.ConstantState entry = get(key, theme); + if (entry != null) { + return entry.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 25154d5c16238..1fdfcd067867d 100644 --- a/core/java/android/content/res/ResourcesImpl.java +++ b/core/java/android/content/res/ResourcesImpl.java @@ -650,21 +650,16 @@ public class ResourcesImpl { key = (((long) value.assetCookie) << 32) | value.data; } - int cacheGeneration; + int cacheGeneration = caches.getGeneration(); // 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 ThemedResourceCache.Entry cachedDrawable = - caches.getDrawable(key, wrapper, theme); - if (cachedDrawable.hasValue()) { - cachedDrawable.getValue().setChangingConfigurations( - value.changingConfigurations); - return cachedDrawable.getValue(); + Drawable cachedDrawable = caches.getInstance(key, wrapper, theme); + if (cachedDrawable != null) { + cachedDrawable.setChangingConfigurations(value.changingConfigurations); + return cachedDrawable; } - cacheGeneration = cachedDrawable.getGeneration(); - } else { - cacheGeneration = ThemedResourceCache.UNDEFINED_GENERATION; } // Next, check preloaded drawables. Preloaded drawables may contain @@ -1009,16 +1004,15 @@ public class ResourcesImpl { TypedValue value, int id) { final long key = (((long) value.assetCookie) << 32) | value.data; final ConfigurationBoundResourceCache cache = mComplexColorCache; - ThemedResourceCache.Entry complexColorEntry = - cache.getInstance(key, wrapper, theme); - if (complexColorEntry.hasValue()) { - return complexColorEntry.getValue(); + ComplexColor complexColor = cache.getInstance(key, wrapper, theme); + if (complexColor != null) { + return complexColor; } + int cacheGeneration = cache.getGeneration(); final android.content.res.ConstantState factory = sPreloadedComplexColors.get(key); - ComplexColor complexColor = null; if (factory != null) { complexColor = factory.newInstance(wrapper, theme); } @@ -1035,8 +1029,7 @@ public class ResourcesImpl { sPreloadedComplexColors.put(key, complexColor.getConstantState()); } } else { - cache.put(key, theme, complexColor.getConstantState(), - complexColorEntry.getGeneration()); + cache.put(key, theme, complexColor.getConstantState(), cacheGeneration); } } return complexColor; diff --git a/core/java/android/content/res/ThemedResourceCache.java b/core/java/android/content/res/ThemedResourceCache.java index e7fd2755a9417..a7cd168690b43 100644 --- a/core/java/android/content/res/ThemedResourceCache.java +++ b/core/java/android/content/res/ThemedResourceCache.java @@ -41,29 +41,6 @@ abstract class ThemedResourceCache { 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. * @@ -108,6 +85,15 @@ abstract class ThemedResourceCache { } } + /** + * Returns the current generation of the cache + * + * @return The current generation + */ + public int getGeneration() { + return mGeneration; + } + /** * Returns an entry from the cache. * @@ -116,7 +102,7 @@ abstract class ThemedResourceCache { * @return a cached entry, or {@code null} if not in the cache */ @Nullable - public Entry get(long key, @Nullable Theme theme) { + public T 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 @@ -126,7 +112,7 @@ abstract class ThemedResourceCache { if (themedEntries != null) { final WeakReference themedEntry = themedEntries.get(key); if (themedEntry != null) { - return new Entry(themedEntry.get(), mGeneration); + return themedEntry.get(); } } @@ -134,14 +120,15 @@ abstract class ThemedResourceCache { if (unthemedEntries != null) { final WeakReference unthemedEntry = unthemedEntries.get(key); if (unthemedEntry != null) { - return new Entry(unthemedEntry.get(), mGeneration); + return unthemedEntry.get(); } } } - return new Entry(null, mGeneration); + return null; } + /** * Prunes cache entries that have been invalidated by a configuration * change. diff --git a/core/tests/coretests/src/android/content/res/ConfigurationBoundResourceCacheTest.java b/core/tests/coretests/src/android/content/res/ConfigurationBoundResourceCacheTest.java index b5f18c26dc972..6ffdee1c4b4f8 100644 --- a/core/tests/coretests/src/android/content/res/ConfigurationBoundResourceCacheTest.java +++ b/core/tests/coretests/src/android/content/res/ConfigurationBoundResourceCacheTest.java @@ -45,7 +45,7 @@ public class ConfigurationBoundResourceCacheTest @SmallTest public void testGetEmpty() { final Resources res = getActivity().getResources(); - assertNull(mCache.getInstance(-1, res, null).getValue()); + assertNull(mCache.getInstance(-1, res, null)); } @SmallTest @@ -53,9 +53,9 @@ public class ConfigurationBoundResourceCacheTest mCache.put(1, null, new DummyFloatConstantState(5f), ThemedResourceCache.UNDEFINED_GENERATION); final Resources res = getActivity().getResources(); - 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()); + assertEquals(5f, mCache.getInstance(1, res, null)); + assertNotSame(5f, mCache.getInstance(1, res, null)); + assertEquals(null, mCache.getInstance(1, res, getActivity().getTheme())); } @SmallTest @@ -63,9 +63,9 @@ public class ConfigurationBoundResourceCacheTest mCache.put(1, getActivity().getTheme(), new DummyFloatConstantState(5f), ThemedResourceCache.UNDEFINED_GENERATION); final Resources res = getActivity().getResources(); - 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()); + assertEquals(null, mCache.getInstance(1, res, null)); + assertEquals(5f, mCache.getInstance(1, res, getActivity().getTheme())); + assertNotSame(5f, mCache.getInstance(1, res, getActivity().getTheme())); } @SmallTest @@ -75,10 +75,10 @@ public class ConfigurationBoundResourceCacheTest mCache.put(1, null, new DummyFloatConstantState(10f), ThemedResourceCache.UNDEFINED_GENERATION); final Resources res = getActivity().getResources(); - 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()); + 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())); } @SmallTest @@ -98,9 +98,9 @@ public class ConfigurationBoundResourceCacheTest Configuration.ORIENTATION_PORTRAIT : Configuration.ORIENTATION_LANDSCAPE; int changes = calcConfigChanges(res, newCnf); - assertEquals(staticDim, mCache.getInstance(key, res, getActivity().getTheme()).getValue()); + assertEquals(staticDim, mCache.getInstance(key, res, getActivity().getTheme())); mCache.onConfigurationChange(changes); - assertEquals(staticDim, mCache.getInstance(key, res, getActivity().getTheme()).getValue()); + assertEquals(staticDim, mCache.getInstance(key, res, getActivity().getTheme())); } @SmallTest @@ -123,9 +123,9 @@ public class ConfigurationBoundResourceCacheTest : Configuration.ORIENTATION_LANDSCAPE; int changes = calcConfigChanges(res, newCnf); assertEquals(changingDim, - mCache.getInstance(key, res, getActivity().getTheme()).getValue()); + mCache.getInstance(key, res, getActivity().getTheme())); mCache.onConfigurationChange(changes); - assertNull(mCache.get(key, getActivity().getTheme()).getValue()); + assertNull(mCache.get(key, getActivity().getTheme())); } @SmallTest @@ -152,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()).getValue()); + getActivity().getTheme())); assertEquals(changingDim, mCache.getInstance(R.dimen.resource_cache_test_orientation_dependent, res, - getActivity().getTheme()).getValue()); + getActivity().getTheme())); mCache.onConfigurationChange(changes); assertEquals(staticDim, mCache.getInstance(R.dimen.resource_cache_test_generic, res, - getActivity().getTheme()).getValue()); + getActivity().getTheme())); assertNull(mCache.getInstance(R.dimen.resource_cache_test_orientation_dependent, res, - getActivity().getTheme()).getValue()); + getActivity().getTheme())); } @SmallTest @@ -198,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).getValue()); + mCache.getInstance(R.dimen.resource_cache_test_generic, res, theme)); assertEquals(changingDim, mCache.getInstance(R.dimen.resource_cache_test_orientation_dependent, res, - theme).getValue()); + theme)); } 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).getValue()); + mCache.getInstance(R.dimen.resource_cache_test_generic, res, theme)); assertNull(mCache.getInstance(R.dimen.resource_cache_test_orientation_dependent, res, - theme).getValue()); + theme)); } }