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

1 files changed, 111 insertions, 101 deletions

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<Class, HashMap<String, Method>> sGetterPropertyMap =
             new HashMap<Class, HashMap<String, Method>>();
 
-    // 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<Class, HashMap<String, Method>> 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<String, Method> 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<String, Method>();
@@ -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<Keyframe> 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<Keyframe> 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<propName, int> for setter method
-            try {
-                mPropertyMapLock.writeLock().lock();
+            synchronized(sJNISetterPropertyMap) {
                 HashMap<String, Long> 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<String, Long>();
-                            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<String, Long>();
+                        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<propName, int> for setter method
-            try {
-                mPropertyMapLock.writeLock().lock();
+            synchronized (sJNISetterPropertyMap) {
                 HashMap<String, Long> 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<String, Long>();
-                            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<String, Long>();
+                        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<String, Long> 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<String, Long>();
-                            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<String, Long>();
+                        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<String, Long> 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<String, Long>();
-                            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<String, Long>();
+                        sJNISetterPropertyMap.put(targetClass, propertyMap);
+                    }
+                    propertyMap.put(mPropertyName, mJniSetter);
                 }
-            } finally {
-                mPropertyMapLock.writeLock().unlock();
             }
         }
     }