From 691487d2c370dc2c7dbf360a25244b99651c18af Mon Sep 17 00:00:00 2001 From: George Mount Date: Mon, 10 Nov 2014 16:46:31 -0800 Subject: Remove unnecessary reflection lookup in Animators. Bug 17978210 When Properties are used with PropertyValuesHolders or ObjectAnimators, the reflection lookup for getters and setters is unnecessary. Fixed problem in which static maps were being protected by instance locks. Fixed problem where we were repeatedly doing a reflection lookup on methods that don't exist. Change-Id: Ic0a1b62357f3aaaa4c900fef6087583ad0e964b6 --- .../android/animation/PropertyValuesHolder.java | 212 +++++++++++---------- 1 file changed, 111 insertions(+), 101 deletions(-) (limited to 'core/java/android/animation/PropertyValuesHolder.java') diff --git a/core/java/android/animation/PropertyValuesHolder.java b/core/java/android/animation/PropertyValuesHolder.java index 97426c3..bd7bca0 100644 --- a/core/java/android/animation/PropertyValuesHolder.java +++ b/core/java/android/animation/PropertyValuesHolder.java @@ -105,10 +105,6 @@ public class PropertyValuesHolder implements Cloneable { private static final HashMap> sGetterPropertyMap = new HashMap>(); - // This lock is used to ensure that only one thread is accessing the property maps - // at a time. - final ReentrantReadWriteLock mPropertyMapLock = new ReentrantReadWriteLock(); - // Used to pass single value to varargs parameter in setter invocation final Object[] mTmpValueArray = new Object[1]; @@ -737,16 +733,19 @@ public class PropertyValuesHolder implements Cloneable { HashMap> propertyMapMap, String prefix, Class valueType) { Method setterOrGetter = null; - try { + synchronized(propertyMapMap) { // Have to lock property map prior to reading it, to guard against // another thread putting something in there after we've checked it // but before we've added an entry to it - mPropertyMapLock.writeLock().lock(); HashMap propertyMap = propertyMapMap.get(targetClass); + boolean wasInMap = false; if (propertyMap != null) { - setterOrGetter = propertyMap.get(mPropertyName); + wasInMap = propertyMap.containsKey(mPropertyName); + if (wasInMap) { + setterOrGetter = propertyMap.get(mPropertyName); + } } - if (setterOrGetter == null) { + if (!wasInMap) { setterOrGetter = getPropertyFunction(targetClass, prefix, valueType); if (propertyMap == null) { propertyMap = new HashMap(); @@ -754,8 +753,6 @@ public class PropertyValuesHolder implements Cloneable { } propertyMap.put(mPropertyName, setterOrGetter); } - } finally { - mPropertyMapLock.writeLock().unlock(); } return setterOrGetter; } @@ -811,30 +808,33 @@ public class PropertyValuesHolder implements Cloneable { mProperty = null; } } - Class targetClass = target.getClass(); - if (mSetter == null) { - setupSetter(targetClass); - } - List keyframes = mKeyframes.getKeyframes(); - int keyframeCount = keyframes == null ? 0 : keyframes.size(); - for (int i = 0; i < keyframeCount; i++) { - Keyframe kf = keyframes.get(i); - if (!kf.hasValue() || kf.valueWasSetOnStart()) { - if (mGetter == null) { - setupGetter(targetClass); + // We can't just say 'else' here because the catch statement sets mProperty to null. + if (mProperty == null) { + Class targetClass = target.getClass(); + if (mSetter == null) { + setupSetter(targetClass); + } + List keyframes = mKeyframes.getKeyframes(); + int keyframeCount = keyframes == null ? 0 : keyframes.size(); + for (int i = 0; i < keyframeCount; i++) { + Keyframe kf = keyframes.get(i); + if (!kf.hasValue() || kf.valueWasSetOnStart()) { if (mGetter == null) { - // Already logged the error - just return to avoid NPE - return; + setupGetter(targetClass); + if (mGetter == null) { + // Already logged the error - just return to avoid NPE + return; + } + } + try { + Object value = convertBack(mGetter.invoke(target)); + kf.setValue(value); + kf.setValueWasSetOnStart(true); + } catch (InvocationTargetException e) { + Log.e("PropertyValuesHolder", e.toString()); + } catch (IllegalAccessException e) { + Log.e("PropertyValuesHolder", e.toString()); } - } - try { - Object value = convertBack(mGetter.invoke(target)); - kf.setValue(value); - kf.setValueWasSetOnStart(true); - } catch (InvocationTargetException e) { - Log.e("PropertyValuesHolder", e.toString()); - } catch (IllegalAccessException e) { - Log.e("PropertyValuesHolder", e.toString()); } } } @@ -1178,32 +1178,33 @@ public class PropertyValuesHolder implements Cloneable { return; } // Check new static hashmap for setter method - try { - mPropertyMapLock.writeLock().lock(); + synchronized(sJNISetterPropertyMap) { HashMap propertyMap = sJNISetterPropertyMap.get(targetClass); + boolean wasInMap = false; if (propertyMap != null) { - Long jniSetter = propertyMap.get(mPropertyName); - if (jniSetter != null) { - mJniSetter = jniSetter; + wasInMap = propertyMap.containsKey(mPropertyName); + if (wasInMap) { + Long jniSetter = propertyMap.get(mPropertyName); + if (jniSetter != null) { + mJniSetter = jniSetter; + } } } - if (mJniSetter == 0) { + if (!wasInMap) { String methodName = getMethodName("set", mPropertyName); - mJniSetter = nGetIntMethod(targetClass, methodName); - if (mJniSetter != 0) { - if (propertyMap == null) { - propertyMap = new HashMap(); - sJNISetterPropertyMap.put(targetClass, propertyMap); - } - propertyMap.put(mPropertyName, mJniSetter); + try { + mJniSetter = nGetIntMethod(targetClass, methodName); + } catch (NoSuchMethodError e) { + // Couldn't find it via JNI - try reflection next. Probably means the method + // doesn't exist, or the type is wrong. An error will be logged later if + // reflection fails as well. } + if (propertyMap == null) { + propertyMap = new HashMap(); + sJNISetterPropertyMap.put(targetClass, propertyMap); + } + propertyMap.put(mPropertyName, mJniSetter); } - } catch (NoSuchMethodError e) { - // Couldn't find it via JNI - try reflection next. Probably means the method - // doesn't exist, or the type is wrong. An error will be logged later if - // reflection fails as well. - } finally { - mPropertyMapLock.writeLock().unlock(); } if (mJniSetter == 0) { // Couldn't find method through fast JNI approach - just use reflection @@ -1315,32 +1316,33 @@ public class PropertyValuesHolder implements Cloneable { return; } // Check new static hashmap for setter method - try { - mPropertyMapLock.writeLock().lock(); + synchronized (sJNISetterPropertyMap) { HashMap propertyMap = sJNISetterPropertyMap.get(targetClass); + boolean wasInMap = false; if (propertyMap != null) { - Long jniSetter = propertyMap.get(mPropertyName); - if (jniSetter != null) { - mJniSetter = jniSetter; + wasInMap = propertyMap.containsKey(mPropertyName); + if (wasInMap) { + Long jniSetter = propertyMap.get(mPropertyName); + if (jniSetter != null) { + mJniSetter = jniSetter; + } } } - if (mJniSetter == 0) { + if (!wasInMap) { String methodName = getMethodName("set", mPropertyName); - mJniSetter = nGetFloatMethod(targetClass, methodName); - if (mJniSetter != 0) { - if (propertyMap == null) { - propertyMap = new HashMap(); - sJNISetterPropertyMap.put(targetClass, propertyMap); - } - propertyMap.put(mPropertyName, mJniSetter); + try { + mJniSetter = nGetFloatMethod(targetClass, methodName); + } catch (NoSuchMethodError e) { + // Couldn't find it via JNI - try reflection next. Probably means the method + // doesn't exist, or the type is wrong. An error will be logged later if + // reflection fails as well. + } + if (propertyMap == null) { + propertyMap = new HashMap(); + sJNISetterPropertyMap.put(targetClass, propertyMap); } + propertyMap.put(mPropertyName, mJniSetter); } - } catch (NoSuchMethodError e) { - // Couldn't find it via JNI - try reflection next. Probably means the method - // doesn't exist, or the type is wrong. An error will be logged later if - // reflection fails as well. - } finally { - mPropertyMapLock.writeLock().unlock(); } if (mJniSetter == 0) { // Couldn't find method through fast JNI approach - just use reflection @@ -1419,16 +1421,19 @@ public class PropertyValuesHolder implements Cloneable { if (mJniSetter != 0) { return; } - try { - mPropertyMapLock.writeLock().lock(); + synchronized(sJNISetterPropertyMap) { HashMap propertyMap = sJNISetterPropertyMap.get(targetClass); + boolean wasInMap = false; if (propertyMap != null) { - Long jniSetterLong = propertyMap.get(mPropertyName); - if (jniSetterLong != null) { - mJniSetter = jniSetterLong; + wasInMap = propertyMap.containsKey(mPropertyName); + if (wasInMap) { + Long jniSetter = propertyMap.get(mPropertyName); + if (jniSetter != null) { + mJniSetter = jniSetter; + } } } - if (mJniSetter == 0) { + if (!wasInMap) { String methodName = getMethodName("set", mPropertyName); calculateValue(0f); float[] values = (float[]) getAnimatedValue(); @@ -1437,19 +1442,20 @@ public class PropertyValuesHolder implements Cloneable { mJniSetter = nGetMultipleFloatMethod(targetClass, methodName, numParams); } catch (NoSuchMethodError e) { // try without the 'set' prefix - mJniSetter = nGetMultipleFloatMethod(targetClass, mPropertyName, numParams); - } - if (mJniSetter != 0) { - if (propertyMap == null) { - propertyMap = new HashMap(); - sJNISetterPropertyMap.put(targetClass, propertyMap); + try { + mJniSetter = nGetMultipleFloatMethod(targetClass, mPropertyName, + numParams); + } catch (NoSuchMethodError e2) { + // just try reflection next } - propertyMap.put(mPropertyName, mJniSetter); } + if (propertyMap == null) { + propertyMap = new HashMap(); + sJNISetterPropertyMap.put(targetClass, propertyMap); + } + propertyMap.put(mPropertyName, mJniSetter); } - } finally { - mPropertyMapLock.writeLock().unlock(); - } + } } } @@ -1522,16 +1528,19 @@ public class PropertyValuesHolder implements Cloneable { if (mJniSetter != 0) { return; } - try { - mPropertyMapLock.writeLock().lock(); + synchronized(sJNISetterPropertyMap) { HashMap propertyMap = sJNISetterPropertyMap.get(targetClass); + boolean wasInMap = false; if (propertyMap != null) { - Long jniSetterLong = propertyMap.get(mPropertyName); - if (jniSetterLong != null) { - mJniSetter = jniSetterLong; + wasInMap = propertyMap.containsKey(mPropertyName); + if (wasInMap) { + Long jniSetter = propertyMap.get(mPropertyName); + if (jniSetter != null) { + mJniSetter = jniSetter; + } } } - if (mJniSetter == 0) { + if (!wasInMap) { String methodName = getMethodName("set", mPropertyName); calculateValue(0f); int[] values = (int[]) getAnimatedValue(); @@ -1540,18 +1549,19 @@ public class PropertyValuesHolder implements Cloneable { mJniSetter = nGetMultipleIntMethod(targetClass, methodName, numParams); } catch (NoSuchMethodError e) { // try without the 'set' prefix - mJniSetter = nGetMultipleIntMethod(targetClass, mPropertyName, numParams); - } - if (mJniSetter != 0) { - if (propertyMap == null) { - propertyMap = new HashMap(); - sJNISetterPropertyMap.put(targetClass, propertyMap); + try { + mJniSetter = nGetMultipleIntMethod(targetClass, mPropertyName, + numParams); + } catch (NoSuchMethodError e2) { + // couldn't find it. } - propertyMap.put(mPropertyName, mJniSetter); } + if (propertyMap == null) { + propertyMap = new HashMap(); + sJNISetterPropertyMap.put(targetClass, propertyMap); + } + propertyMap.put(mPropertyName, mJniSetter); } - } finally { - mPropertyMapLock.writeLock().unlock(); } } } -- cgit v1.1