Skip to content

Commit 83aa21d

Browse files
alzimmermsftalzimmermsft
authored
Resolve HttpHeaders Deserialization Performance (Azure#29097)
1 parent ab99a01 commit 83aa21d

File tree

5 files changed

+94
-75
lines changed

5 files changed

+94
-75
lines changed

‎sdk/core/azure-core/src/main/java/com/azure/core/http/rest/ResponseConstructorsCache.java‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,12 @@ private static MethodHandle locateResponseConstructor(Class<?> responseClass) {
9999
}
100100
}
101101
}
102-
return null;
102+
103+
// Before this was returning null, but in all cases where null is returned from this method an exception would
104+
// be thrown later. Instead, just throw here to properly use computeIfAbsent by not inserting a null key-value
105+
// pair that would cause the computation to always be performed.
106+
throw LOGGER.logExceptionAsError(new RuntimeException("Cannot find suitable constructor for class "
107+
+ responseClass));
103108
}
104109

105110
/**

‎sdk/core/azure-core/src/main/java/com/azure/core/http/rest/RestProxy.java‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -509,11 +509,7 @@ private static Response createResponse(HttpDecodedResponse response, Type entity
509509
// given a way to register their constructor as a callback method that consumes HttpDecodedResponse and the
510510
// body as an Object.
511511
MethodHandle constructorHandle = RESPONSE_CONSTRUCTORS_CACHE.get(cls);
512-
if (constructorHandle == null) {
513-
throw LOGGER.logExceptionAsError(new RuntimeException("Cannot find suitable constructor for class " + cls));
514-
} else {
515-
return RESPONSE_CONSTRUCTORS_CACHE.invoke(constructorHandle, response, bodyAsObject);
516-
}
512+
return RESPONSE_CONSTRUCTORS_CACHE.invoke(constructorHandle, response, bodyAsObject);
517513
}
518514

519515
private static Mono<?> handleBodyReturnType(final HttpDecodedResponse response,

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

Lines changed: 40 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -15,15 +15,15 @@
1515
import java.util.HashMap;
1616
import java.util.Locale;
1717
import java.util.Map;
18+
import java.util.Optional;
1819
import java.util.concurrent.ConcurrentHashMap;
19-
import java.util.function.Function;
2020

2121
/*
2222
* Internal helper class that helps manage converting headers into their header collection.
2323
*/
2424
final class HeaderCollectionHandler {
2525
private static final int CACHE_SIZE_LIMIT = 10000;
26-
private static final Map<Field, MethodHandle> FIELD_TO_SETTER_CACHE = new ConcurrentHashMap<>();
26+
private static final Map<Field, Optional<MethodHandle>> FIELD_TO_SETTER_CACHE = new ConcurrentHashMap<>();
2727
private final String prefix;
2828
private final int prefixLength;
2929
private final Map<String, String> values;
@@ -83,13 +83,45 @@ private boolean usePublicSetter(Object deserializedHeaders, ClientLogger logger)
8383
final String clazzSimpleName = clazz.getSimpleName();
8484
final String fieldName = declaringField.getName();
8585

86-
MethodHandle setterHandler = getFromCache(declaringField, field -> {
86+
Optional<MethodHandle> setterHandler = getFromCache(declaringField, clazz, clazzSimpleName, fieldName, logger);
87+
88+
if (!setterHandler.isPresent()) {
89+
return false;
90+
}
91+
92+
try {
93+
setterHandler.get().invokeWithArguments(deserializedHeaders, values);
94+
logger.verbose("Set header collection {} on class {} using MethodHandle.", fieldName, clazzSimpleName);
95+
96+
return true;
97+
} catch (Throwable ex) {
98+
if (ex instanceof Error) {
99+
throw (Error) ex;
100+
}
101+
102+
logger.verbose("Failed to set header {} collection on class {} using MethodHandle.", fieldName,
103+
clazzSimpleName, ex);
104+
return false;
105+
}
106+
}
107+
108+
private static String getPotentialSetterName(String fieldName) {
109+
return "set" + fieldName.substring(0, 1).toUpperCase(Locale.ROOT) + fieldName.substring(1);
110+
}
111+
112+
private static Optional<MethodHandle> getFromCache(Field key, Class<?> clazz, String clazzSimpleName,
113+
String fieldName, ClientLogger logger) {
114+
if (FIELD_TO_SETTER_CACHE.size() >= CACHE_SIZE_LIMIT) {
115+
FIELD_TO_SETTER_CACHE.clear();
116+
}
117+
118+
return FIELD_TO_SETTER_CACHE.computeIfAbsent(key, field -> {
87119
MethodHandles.Lookup lookupToUse;
88120
try {
89121
lookupToUse = ReflectionUtilsApi.INSTANCE.getLookupToUse(clazz);
90122
} catch (Exception ex) {
91123
logger.verbose("Failed to retrieve MethodHandles.Lookup for field {}.", field, ex);
92-
return null;
124+
return Optional.empty();
93125
}
94126

95127
String setterName = getPotentialSetterName(fieldName);
@@ -100,58 +132,26 @@ private boolean usePublicSetter(Object deserializedHeaders, ClientLogger logger)
100132

101133
logger.verbose("Using MethodHandle for setter {} on class {}.", setterName, clazzSimpleName);
102134

103-
return handle;
135+
return Optional.of(handle);
104136
} catch (ReflectiveOperationException ex) {
105137
logger.verbose("Failed to retrieve MethodHandle for setter {} on class {}.", setterName,
106138
clazzSimpleName, ex);
107139
}
108140

109141
try {
110-
Method setterMethod = deserializedHeaders.getClass()
111-
.getDeclaredMethod(setterName, Map.class);
142+
Method setterMethod = clazz.getDeclaredMethod(setterName, Map.class);
112143
MethodHandle handle = lookupToUse.unreflect(setterMethod);
113144

114145
logger.verbose("Using unreflected MethodHandle for setter {} on class {}.", setterName,
115146
clazzSimpleName);
116147

117-
return handle;
148+
return Optional.of(handle);
118149
} catch (ReflectiveOperationException ex) {
119150
logger.verbose("Failed to unreflect MethodHandle for setter {} on class {}.", setterName,
120151
clazzSimpleName, ex);
121152
}
122153

123-
return null;
154+
return Optional.empty();
124155
});
125-
126-
if (setterHandler == null) {
127-
return false;
128-
}
129-
130-
try {
131-
setterHandler.invokeWithArguments(deserializedHeaders, values);
132-
logger.verbose("Set header collection {} on class {} using MethodHandle.", fieldName, clazzSimpleName);
133-
134-
return true;
135-
} catch (Throwable ex) {
136-
if (ex instanceof Error) {
137-
throw (Error) ex;
138-
}
139-
140-
logger.verbose("Failed to set header {} collection on class {} using MethodHandle.", fieldName,
141-
clazzSimpleName, ex);
142-
return false;
143-
}
144-
}
145-
146-
private static String getPotentialSetterName(String fieldName) {
147-
return "set" + fieldName.substring(0, 1).toUpperCase(Locale.ROOT) + fieldName.substring(1);
148-
}
149-
150-
private static MethodHandle getFromCache(Field key, Function<Field, MethodHandle> compute) {
151-
if (FIELD_TO_SETTER_CACHE.size() >= CACHE_SIZE_LIMIT) {
152-
FIELD_TO_SETTER_CACHE.clear();
153-
}
154-
155-
return FIELD_TO_SETTER_CACHE.computeIfAbsent(key, compute);
156156
}
157157
}

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

Lines changed: 45 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import java.util.List;
2828
import java.util.Locale;
2929
import java.util.Map;
30+
import java.util.Optional;
3031
import java.util.Set;
3132
import java.util.concurrent.ConcurrentHashMap;
3233
import java.util.function.BiConsumer;
@@ -44,7 +45,7 @@ public final class ObjectMapperShim {
4445
private static final int CACHE_SIZE_LIMIT = 10000;
4546

4647
private static final Map<Type, JavaType> TYPE_TO_JAVA_TYPE_CACHE = new ConcurrentHashMap<>();
47-
private static final Map<Type, MethodHandle> TYPE_TO_STRONGLY_TYPED_HEADERS_CONSTRUCTOR_CACHE
48+
private static final Map<Type, Optional<MethodHandle>> TYPE_TO_STRONGLY_TYPED_HEADERS_CONSTRUCTOR_CACHE
4849
= new ConcurrentHashMap<>();
4950

5051
/**
@@ -283,10 +284,10 @@ private JavaType createJavaType(Type type) {
283284
javaTypeArguments[i] = createJavaType(actualTypeArguments[i]);
284285
}
285286

286-
return getFromCache(TYPE_TO_JAVA_TYPE_CACHE, type, t -> mapper.getTypeFactory()
287+
return getFromTypeCache(type, t -> mapper.getTypeFactory()
287288
.constructParametricType((Class<?>) parameterizedType.getRawType(), javaTypeArguments));
288289
} else {
289-
return getFromCache(TYPE_TO_JAVA_TYPE_CACHE, type, t -> mapper.getTypeFactory().constructType(t));
290+
return getFromTypeCache(type, t -> mapper.getTypeFactory().constructType(t));
290291
}
291292
}
292293

@@ -297,23 +298,10 @@ public <T> T deserialize(HttpHeaders headers, Type deserializedHeadersType) thro
297298
}
298299

299300
try {
300-
Class<?> headersClass = TypeUtil.getRawClass(deserializedHeadersType);
301-
302-
MethodHandle constructor = getFromCache(TYPE_TO_STRONGLY_TYPED_HEADERS_CONSTRUCTOR_CACHE,
303-
deserializedHeadersType, type -> {
304-
try {
305-
MethodHandles.Lookup lookup = ReflectionUtilsApi.INSTANCE.getLookupToUse(headersClass);
306-
return lookup.unreflectConstructor(headersClass.getDeclaredConstructor(HttpHeaders.class));
307-
} catch (Throwable throwable) {
308-
if (throwable instanceof Error) {
309-
throw (Error) throwable;
310-
}
311-
return null;
312-
}
313-
});
314-
315-
if (constructor != null) {
316-
return (T) constructor.invokeWithArguments(headers);
301+
Optional<MethodHandle> constructor = getFromHeadersConstructorCache(deserializedHeadersType);
302+
303+
if (constructor.isPresent()) {
304+
return (T) constructor.get().invokeWithArguments(headers);
317305
}
318306
} catch (Throwable throwable) {
319307
if (throwable instanceof Error) {
@@ -423,13 +411,45 @@ public <T extends JsonNode> T valueToTree(Object fromValue) {
423411
}
424412

425413
/*
426-
* Helper method that gets the value for the given key from the cache.
414+
* Helper methods that gets the value for the given key from the cache.
427415
*/
428-
private static <K, V> V getFromCache(Map<K, V> cache, K key, Function<K, V> compute) {
429-
if (cache.size() >= CACHE_SIZE_LIMIT) {
430-
cache.clear();
416+
private static JavaType getFromTypeCache(Type key, Function<Type, JavaType> compute) {
417+
if (TYPE_TO_JAVA_TYPE_CACHE.size() >= CACHE_SIZE_LIMIT) {
418+
TYPE_TO_JAVA_TYPE_CACHE.clear();
419+
}
420+
421+
return TYPE_TO_JAVA_TYPE_CACHE.computeIfAbsent(key, compute);
422+
}
423+
424+
private static Optional<MethodHandle> getFromHeadersConstructorCache(Type key) {
425+
if (TYPE_TO_STRONGLY_TYPED_HEADERS_CONSTRUCTOR_CACHE.size() >= CACHE_SIZE_LIMIT) {
426+
TYPE_TO_STRONGLY_TYPED_HEADERS_CONSTRUCTOR_CACHE.clear();
431427
}
432428

433-
return cache.computeIfAbsent(key, compute);
429+
return TYPE_TO_STRONGLY_TYPED_HEADERS_CONSTRUCTOR_CACHE.computeIfAbsent(key, type -> {
430+
try {
431+
Class<?> headersClass = TypeUtil.getRawClass(type);
432+
MethodHandles.Lookup lookup = ReflectionUtilsApi.INSTANCE.getLookupToUse(headersClass);
433+
return Optional.of(lookup.unreflectConstructor(headersClass.getDeclaredConstructor(HttpHeaders.class)));
434+
} catch (Throwable throwable) {
435+
if (throwable instanceof Error) {
436+
throw (Error) throwable;
437+
}
438+
439+
// In a previous implementation the cache was a Map<Type, MethodHandle> and null was being returned
440+
// here in an attempt to indicate that there is no HttpHeaders-based declared constructor.
441+
// Unfortunately, null isn't a valid indicator to computeIfAbsent that a computation has been performed
442+
// and this cache would never effectively be a cache as compute would always be performed when there
443+
// was no matching constructor.
444+
//
445+
// Now the implementation for the cache is Map<Type, Optional<MethodHandle>> and Optional.empty is
446+
// returned when there is no matching HttpHeaders-based constructor. This now results in this case
447+
// properly inserting into the cache and only running when a new type is seen or the cache is cleared
448+
// due to reaching capacity.
449+
//
450+
// With this change, benchmarking deserialize(HttpHeaders, Type) saw a 20% performance improvement.
451+
return Optional.empty();
452+
}
453+
});
434454
}
435455
}

‎sdk/core/azure-core/src/test/java/com/azure/core/http/rest/ResponseConstructorsCacheBenchMark.java‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,11 +55,9 @@ public void reflectionCache(Blackhole blackhole) {
5555
(Class<? extends Response<?>>) TypeUtil.getRawClass(inputs[i].returnType());
5656
// Step1: Locate Constructor using Reflection.
5757
MethodHandle handle = defaultCache.get(responseClass);
58-
if (handle == null) {
59-
throw new IllegalStateException("Response constructor with expected parameters not found.");
60-
}
6158
// Step2: Invoke Constructor using Reflection.
62-
Response<?> response = defaultCache.invoke(handle, inputs[i].decodedResponse(), inputs[i].bodyAsObject());
59+
Response<?> response = defaultCache.invoke(handle, inputs[i].decodedResponse(),
60+
inputs[i].bodyAsObject());
6361
// avoid JVM dead code detection
6462
blackhole.consume(response);
6563
}

0 commit comments

Comments
 (0)