Remove Use of Optional when a MethodHandle isn't Found (Azure#29103)

Author: alzimmermsft

sdk/core/azure-core/src/main/java/com/azure/core/implementation/jackson/HeaderCollectionHandler.java

@@ -15,15 +15,18 @@
 import java.util.HashMap;
 import java.util.Locale;
 import java.util.Map;
-import java.util.Optional;
 import java.util.concurrent.ConcurrentHashMap;
 
 /*
  * Internal helper class that helps manage converting headers into their header collection.
  */
 final class HeaderCollectionHandler {
     private static final int CACHE_SIZE_LIMIT = 10000;
-    private static final Map<Field, Optional<MethodHandle>> FIELD_TO_SETTER_CACHE = new ConcurrentHashMap<>();
+    private static final Map<Field, MethodHandle> FIELD_TO_SETTER_CACHE = new ConcurrentHashMap<>();
+
+    // Dummy constant that indicates no setter was found for the Field.
+    private static final MethodHandle NO_SETTER_HANDLE = MethodHandles.identity(HeaderCollectionHandler.class);
+
     private final String prefix;
     private final int prefixLength;
     private final Map<String, String> values;
@@ -83,14 +86,14 @@ private boolean usePublicSetter(Object deserializedHeaders, ClientLogger logger)
         final String clazzSimpleName = clazz.getSimpleName();
         final String fieldName = declaringField.getName();
 
-        Optional<MethodHandle> setterHandler = getFromCache(declaringField, clazz, clazzSimpleName, fieldName, logger);
+        MethodHandle setterHandler = getFromCache(declaringField, clazz, clazzSimpleName, fieldName, logger);
 
-        if (!setterHandler.isPresent()) {
+        if (setterHandler == NO_SETTER_HANDLE) {
             return false;
         }
 
         try {
-            setterHandler.get().invokeWithArguments(deserializedHeaders, values);
+            setterHandler.invokeWithArguments(deserializedHeaders, values);
             logger.verbose("Set header collection {} on class {} using MethodHandle.", fieldName, clazzSimpleName);
 
             return true;
@@ -109,7 +112,7 @@ private static String getPotentialSetterName(String fieldName) {
         return "set" + fieldName.substring(0, 1).toUpperCase(Locale.ROOT) + fieldName.substring(1);
     }
 
-    private static Optional<MethodHandle> getFromCache(Field key, Class<?> clazz, String clazzSimpleName,
+    private static MethodHandle getFromCache(Field key, Class<?> clazz, String clazzSimpleName,
         String fieldName, ClientLogger logger) {
         if (FIELD_TO_SETTER_CACHE.size() >= CACHE_SIZE_LIMIT) {
             FIELD_TO_SETTER_CACHE.clear();
@@ -121,7 +124,16 @@ private static Optional<MethodHandle> getFromCache(Field key, Class<?> clazz, St
                 lookupToUse = ReflectionUtilsApi.INSTANCE.getLookupToUse(clazz);
             } catch (Exception ex) {
                 logger.verbose("Failed to retrieve MethodHandles.Lookup for field {}.", field, ex);
-                return Optional.empty();
+
+                // In a previous implementation compute returned null here in an attempt to indicate that there is no
+                // setter for the field. Unfortunately, null isn't a valid indicator to computeIfAbsent that a
+                // computation has been performed and this cache would never effectively be a cache as compute would
+                // always be performed when there was no setter for the field.
+                //
+                // Now the implementation returns a dummy constant when there is no setter for the field. This now
+                // results in this case properly inserting into the cache and only running when a new type is seen or
+                // the cache is cleared due to reaching capacity.
+                return NO_SETTER_HANDLE;
             }
 
             String setterName = getPotentialSetterName(fieldName);
@@ -132,7 +144,7 @@ private static Optional<MethodHandle> getFromCache(Field key, Class<?> clazz, St
 
                 logger.verbose("Using MethodHandle for setter {} on class {}.", setterName, clazzSimpleName);
 
-                return Optional.of(handle);
+                return handle;
             } catch (ReflectiveOperationException ex) {
                 logger.verbose("Failed to retrieve MethodHandle for setter {} on class {}.", setterName,
                     clazzSimpleName, ex);
@@ -145,13 +157,21 @@ private static Optional<MethodHandle> getFromCache(Field key, Class<?> clazz, St
                 logger.verbose("Using unreflected MethodHandle for setter {} on class {}.", setterName,
                     clazzSimpleName);
 
-                return Optional.of(handle);
+                return handle;
             } catch (ReflectiveOperationException ex) {
                 logger.verbose("Failed to unreflect MethodHandle for setter {} on class {}.", setterName,
                     clazzSimpleName, ex);
             }
 
-            return Optional.empty();
+            // In a previous implementation compute returned null here in an attempt to indicate that there is no setter
+            // for the field. Unfortunately, null isn't a valid indicator to computeIfAbsent that a computation has been
+            // performed and this cache would never effectively be a cache as compute would always be performed when
+            // there was no setter for the field.
+            //
+            // Now the implementation returns a dummy constant when there is no setter for the field. This now results
+            // in this case properly inserting into the cache and only running when a new type is seen or the cache is
+            // cleared due to reaching capacity.
+            return NO_SETTER_HANDLE;
         });
     }
 }

sdk/core/azure-core/src/main/java/com/azure/core/implementation/jackson/ObjectMapperShim.java

@@ -27,7 +27,6 @@
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
-import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.function.BiConsumer;
@@ -45,9 +44,12 @@ public final class ObjectMapperShim {
     private static final int CACHE_SIZE_LIMIT = 10000;
 
     private static final Map<Type, JavaType> TYPE_TO_JAVA_TYPE_CACHE = new ConcurrentHashMap<>();
-    private static final Map<Type, Optional<MethodHandle>> TYPE_TO_STRONGLY_TYPED_HEADERS_CONSTRUCTOR_CACHE
+    private static final Map<Type, MethodHandle> TYPE_TO_STRONGLY_TYPED_HEADERS_CONSTRUCTOR_CACHE
         = new ConcurrentHashMap<>();
 
+    // Dummy constant that indicates an HttpHeaders-based constructor wasn't found for the Type.
+    private static final MethodHandle NO_CONSTRUCTOR_HANDLE = MethodHandles.identity(ObjectMapperShim.class);
+
     /**
      * Creates and configures JSON {@code ObjectMapper} capable of serializing azure.core types, with flattening and
      * additional properties support.
@@ -298,10 +300,10 @@ public <T> T deserialize(HttpHeaders headers, Type deserializedHeadersType) thro
         }
 
         try {
-            Optional<MethodHandle> constructor = getFromHeadersConstructorCache(deserializedHeadersType);
+            MethodHandle constructor = getFromHeadersConstructorCache(deserializedHeadersType);
 
-            if (constructor.isPresent()) {
-                return (T) constructor.get().invokeWithArguments(headers);
+            if (constructor != NO_CONSTRUCTOR_HANDLE) {
+                return (T) constructor.invokeWithArguments(headers);
             }
         } catch (Throwable throwable) {
             if (throwable instanceof Error) {
@@ -421,7 +423,7 @@ private static JavaType getFromTypeCache(Type key, Function<Type, JavaType> comp
         return TYPE_TO_JAVA_TYPE_CACHE.computeIfAbsent(key, compute);
     }
 
-    private static Optional<MethodHandle> getFromHeadersConstructorCache(Type key) {
+    private static MethodHandle getFromHeadersConstructorCache(Type key) {
         if (TYPE_TO_STRONGLY_TYPED_HEADERS_CONSTRUCTOR_CACHE.size() >= CACHE_SIZE_LIMIT) {
             TYPE_TO_STRONGLY_TYPED_HEADERS_CONSTRUCTOR_CACHE.clear();
         }
@@ -430,25 +432,23 @@ private static Optional<MethodHandle> getFromHeadersConstructorCache(Type key) {
             try {
                 Class<?> headersClass = TypeUtil.getRawClass(type);
                 MethodHandles.Lookup lookup = ReflectionUtilsApi.INSTANCE.getLookupToUse(headersClass);
-                return Optional.of(lookup.unreflectConstructor(headersClass.getDeclaredConstructor(HttpHeaders.class)));
+                return lookup.unreflectConstructor(headersClass.getDeclaredConstructor(HttpHeaders.class));
             } catch (Throwable throwable) {
                 if (throwable instanceof Error) {
                     throw (Error) throwable;
                 }
 
-                // In a previous implementation the cache was a Map<Type, MethodHandle> and null was being returned
-                // here in an attempt to indicate that there is no HttpHeaders-based declared constructor.
-                // Unfortunately, null isn't a valid indicator to computeIfAbsent that a computation has been performed
-                // and this cache would never effectively be a cache as compute would always be performed when there
-                // was no matching constructor.
+                // In a previous implementation compute returned null here in an attempt to indicate that there is no
+                // HttpHeaders-based declared constructor. Unfortunately, null isn't a valid indicator to
+                // computeIfAbsent that a computation has been performed and this cache would never effectively be a
+                // cache as compute would always be performed when there was no matching constructor.
                 //
-                // Now the implementation for the cache is Map<Type, Optional<MethodHandle>> and Optional.empty is
-                // returned when there is no matching HttpHeaders-based constructor. This now results in this case
-                // properly inserting into the cache and only running when a new type is seen or the cache is cleared
-                // due to reaching capacity.
+                // Now the implementation returns a dummy constant when there is no matching HttpHeaders-based
+                // constructor. This now results in this case properly inserting into the cache and only running when a
+                // new type is seen or the cache is cleared due to reaching capacity.
                 //
                 // With this change, benchmarking deserialize(HttpHeaders, Type) saw a 20% performance improvement.
-                return Optional.empty();
+                return NO_CONSTRUCTOR_HANDLE;
             }
         });
     }