From 313c9d3c9b71dacc463daab14e22f59ad56518bf Mon Sep 17 00:00:00 2001 From: Oliver Drotbohm Date: Tue, 23 Nov 2021 02:15:43 +0100 Subject: [PATCH] #1701 - Fix performance degradations introduced by #467. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The commits for #467 significantly degraded performance as CachingMappingDiscoverer.getParams(Method) doesn't properly cache the result of the call which causes quite expensive, unnecessarily repeated annotation lookups for the very same method. We also avoid the creation of an Optional instance for the sole purpose of a simple null check. Introduce FormatterFactory to potentially cache the to-String formatting functions and thus avoid repeated evaluation and Function object creation. We now also avoid the creation of ParamRequestCondition instances if no @RequestParams(params = …) values could be found in the first place. --- .../core/AnnotationMappingDiscoverer.java | 3 +- .../server/core/CachingMappingDiscoverer.java | 15 ++- .../server/core/SpringAffordanceBuilder.java | 1 - .../hateoas/server/core/WebHandler.java | 111 ++++++++++++------ .../server/mvc/WebMvcLinkBuilderFactory.java | 28 +++-- 5 files changed, 102 insertions(+), 56 deletions(-) diff --git a/src/main/java/org/springframework/hateoas/server/core/AnnotationMappingDiscoverer.java b/src/main/java/org/springframework/hateoas/server/core/AnnotationMappingDiscoverer.java index 0dd8e187..988b0a23 100644 --- a/src/main/java/org/springframework/hateoas/server/core/AnnotationMappingDiscoverer.java +++ b/src/main/java/org/springframework/hateoas/server/core/AnnotationMappingDiscoverer.java @@ -15,7 +15,6 @@ */ package org.springframework.hateoas.server.core; -import static java.util.Optional.*; import static org.springframework.core.annotation.AnnotatedElementUtils.*; import static org.springframework.core.annotation.AnnotationUtils.*; @@ -177,7 +176,7 @@ public class AnnotationMappingDiscoverer implements MappingDiscoverer { Annotation annotation = findMergedAnnotation(method, annotationType); String[] params = (String[]) getValue(annotation, "params"); - return ofNullable(params).orElseGet(() -> new String[0]); + return params == null ? new String[0] : params; } private String[] getMappingFrom(@Nullable Annotation annotation) { diff --git a/src/main/java/org/springframework/hateoas/server/core/CachingMappingDiscoverer.java b/src/main/java/org/springframework/hateoas/server/core/CachingMappingDiscoverer.java index 8159fdd4..756a9109 100644 --- a/src/main/java/org/springframework/hateoas/server/core/CachingMappingDiscoverer.java +++ b/src/main/java/org/springframework/hateoas/server/core/CachingMappingDiscoverer.java @@ -37,6 +37,8 @@ public class CachingMappingDiscoverer implements MappingDiscoverer { private static final Map MAPPINGS = new ConcurrentReferenceHashMap<>(); private static final Map> METHODS = new ConcurrentReferenceHashMap<>(); + private static final Map PARAMS = new ConcurrentReferenceHashMap<>(); + private static final Map> CONSUMES = new ConcurrentReferenceHashMap<>(); private final MappingDiscoverer delegate; @@ -68,10 +70,7 @@ public class CachingMappingDiscoverer implements MappingDiscoverer { @Nullable @Override public String getMapping(Method method) { - - String key = key(method.getDeclaringClass(), method); - - return MAPPINGS.computeIfAbsent(key, __ -> delegate.getMapping(method)); + return MAPPINGS.computeIfAbsent(key(method), __ -> delegate.getMapping(method)); } /* @@ -102,7 +101,7 @@ public class CachingMappingDiscoverer implements MappingDiscoverer { */ @Override public List getConsumes(Method method) { - return delegate.getConsumes(method); + return CONSUMES.computeIfAbsent(key(method), __ -> delegate.getConsumes(method)); } /* @@ -111,7 +110,11 @@ public class CachingMappingDiscoverer implements MappingDiscoverer { */ @Override public String[] getParams(Method method) { - return delegate.getParams(method); + return PARAMS.computeIfAbsent(key(method), __ -> delegate.getParams(method)); + } + + private static String key(Method method) { + return key(method.getDeclaringClass(), method); } private static String key(Class type, @Nullable Method method) { diff --git a/src/main/java/org/springframework/hateoas/server/core/SpringAffordanceBuilder.java b/src/main/java/org/springframework/hateoas/server/core/SpringAffordanceBuilder.java index 87aaa082..4969d19a 100644 --- a/src/main/java/org/springframework/hateoas/server/core/SpringAffordanceBuilder.java +++ b/src/main/java/org/springframework/hateoas/server/core/SpringAffordanceBuilder.java @@ -45,7 +45,6 @@ import org.springframework.web.bind.annotation.RequestParam; */ public class SpringAffordanceBuilder { - @SuppressWarnings("deprecation") // public static final MappingDiscoverer DISCOVERER = CachingMappingDiscoverer .of(new PropertyResolvingMappingDiscoverer(new AnnotationMappingDiscoverer(RequestMapping.class))); diff --git a/src/main/java/org/springframework/hateoas/server/core/WebHandler.java b/src/main/java/org/springframework/hateoas/server/core/WebHandler.java index ed9e66d6..a21170e1 100644 --- a/src/main/java/org/springframework/hateoas/server/core/WebHandler.java +++ b/src/main/java/org/springframework/hateoas/server/core/WebHandler.java @@ -78,7 +78,8 @@ public class WebHandler { @Nullable BiFunction additionalUriHandler, Function finisher, Supplier conversionService) { - return linkTo(invocationValue, creator, additionalUriHandler).conclude(finisher, conversionService.get()); + return linkTo(invocationValue, creator, additionalUriHandler).conclude(finisher, + conversionService.get()); } private static PreparedWebHandler linkTo(Object invocationValue, @@ -99,6 +100,8 @@ public class WebHandler { return (finisher, conversionService) -> { + FormatterFactory factory = new FormatterFactory(conversionService); + UriComponentsBuilder builder = finisher.apply(mapping); UriTemplate template = UriTemplateFactory.templateFor(mapping == null ? "/" : mapping); Map values = new HashMap<>(); @@ -114,7 +117,7 @@ public class WebHandler { Object source = classMappingParameters.next(); values.put(name, variable.prepareAndEncode( - HandlerMethodParameter.prepareValue(source, conversionService, TypeDescriptor.forObject(source)))); + HandlerMethodParameter.prepareValue(source, factory, TypeDescriptor.forObject(source)))); } Method method = invocation.getMethod(); @@ -126,7 +129,7 @@ public class WebHandler { TemplateVariable variable = TemplateVariable.segment(parameter.getVariableName()); Object verifiedValue = parameter.getVerifiedValue(arguments); Object preparedValue = verifiedValue == null ? verifiedValue - : parameter.prepareValue(verifiedValue, conversionService); + : parameter.prepareValue(verifiedValue, factory); values.put(variable.getName(), variable.prepareAndEncode(preparedValue)); } @@ -135,7 +138,7 @@ public class WebHandler { for (HandlerMethodParameter parameter : parameters.getParameterAnnotatedWith(RequestParam.class, arguments)) { - bindRequestParameters(builder, parameter, arguments, conversionService); + bindRequestParameters(builder, parameter, arguments, factory); boolean isSkipValue = SKIP_VALUE.equals(parameter.getVerifiedValue(arguments)); boolean isMapParameter = Map.class.isAssignableFrom(parameter.parameter.getParameterType()); @@ -186,7 +189,7 @@ public class WebHandler { */ @SuppressWarnings("unchecked") private static void bindRequestParameters(UriComponentsBuilder builder, HandlerMethodParameter parameter, - Object[] arguments, ConversionService conversionService) { + Object[] arguments, FormatterFactory factory) { Object value = parameter.getVerifiedValue(arguments); @@ -198,7 +201,7 @@ public class WebHandler { if (value instanceof MultiValueMap) { - Map> requestParams = (Map>) parameter.prepareValue(value, conversionService); + Map> requestParams = (Map>) parameter.prepareValue(value, factory); for (Entry> entry : requestParams.entrySet()) { for (Object element : entry.getValue()) { @@ -212,7 +215,7 @@ public class WebHandler { if (value instanceof Map) { - Map requestParams = (Map) parameter.prepareValue(value, conversionService); + Map requestParams = (Map) parameter.prepareValue(value, factory); for (Entry entry : requestParams.entrySet()) { @@ -234,7 +237,7 @@ public class WebHandler { if (value instanceof Collection) { - Collection collection = (Collection) parameter.prepareValue(value, conversionService); + Collection collection = (Collection) parameter.prepareValue(value, factory); if (parameter.isNonComposite()) { builder.queryParam(key, variable.prepareAndEncode(collection)); @@ -256,31 +259,11 @@ public class WebHandler { } else { if (key != null) { - builder.queryParam(key, variable.prepareAndEncode(parameter.prepareValue(value, conversionService))); + builder.queryParam(key, variable.prepareAndEncode(parameter.prepareValue(value, factory))); } } } - private static Function getFormatter(ConversionService conversionService, TypeDescriptor descriptor) { - - return source -> { - - if (String.class.isInstance(source)) { - return (String) source; - } - - Object result = conversionService.canConvert(descriptor, STRING_DESCRIPTOR) - ? conversionService.convert(source, descriptor, STRING_DESCRIPTOR) - : source == null ? null : source.toString(); - - if (result == null) { - throw new IllegalArgumentException(String.format("Conversion of value %s resulted in null!", source)); - } - - return (String) result; - }; - } - private static class HandlerMethodParameters { private static final List> ANNOTATIONS = Arrays.asList(RequestParam.class, @@ -419,7 +402,7 @@ public class WebHandler { return variableName; } - public Object prepareValue(Object value, ConversionService conversionService) { + public Object prepareValue(Object value, FormatterFactory conversionService) { Object result = prepareValue(value, conversionService, typeDescriptor); @@ -428,7 +411,7 @@ public class WebHandler { @Nullable @SuppressWarnings("unchecked") - public static Object prepareValue(@Nullable Object value, ConversionService conversionService, + public static Object prepareValue(@Nullable Object value, FormatterFactory factory, @Nullable TypeDescriptor descriptor) { if (descriptor == null || value == null) { @@ -437,6 +420,10 @@ public class WebHandler { value = ObjectUtils.unwrapOptional(value); + if (String.class.isInstance(value)) { + return value; + } + if (Collection.class.isInstance(value)) { List prepared = new ArrayList<>(); @@ -444,7 +431,7 @@ public class WebHandler { for (Object element : (Collection) value) { TypeDescriptor elementTypeDescriptor = descriptor.elementTypeDescriptor(element); - prepared.add(prepareValue(element, conversionService, elementTypeDescriptor)); + prepared.add(prepareValue(element, factory, elementTypeDescriptor)); } return prepared; @@ -459,14 +446,14 @@ public class WebHandler { TypeDescriptor keyTypeDescriptor = descriptor.getMapKeyTypeDescriptor(entry.getKey()); TypeDescriptor elementTypeDescriptor = descriptor.elementTypeDescriptor(entry.getValue()); - prepared.put(prepareValue(entry.getKey(), conversionService, keyTypeDescriptor), - prepareValue(entry.getValue(), conversionService, elementTypeDescriptor)); + prepared.put(prepareValue(entry.getKey(), factory, keyTypeDescriptor), + prepareValue(entry.getValue(), factory, elementTypeDescriptor)); } return prepared; } - return getFormatter(conversionService, descriptor).apply(value); + return factory.getFormatter(descriptor).apply(value); } private String determineVariableName() { @@ -508,6 +495,60 @@ public class WebHandler { public abstract boolean isRequired(); } + /** + * Factory to create to-{@link String} converters by type. Caching, to avoid repeated calculations and + * {@link Function} object creation. + * + * @author Oliver Drotbohm + */ + private static class FormatterFactory { + + private static final Function DEFAULT = source -> source == null ? null : source.toString(); + + private final Map> formatters = new HashMap<>(); + private final ConversionService conversionService; + + /** + * Creates a new {@link FormatterFactory} for the given {@link ConversionService}. + * + * @param conversionService must not be {@literal null}. + */ + public FormatterFactory(ConversionService conversionService) { + this.conversionService = conversionService; + } + + /** + * Return the formatting function to map objects of the given {@link TypeDescriptor} to String. + * + * @param descriptor must not be {@literal null}. + * @return will never be {@literal null}. + */ + public Function getFormatter(TypeDescriptor descriptor) { + + if (STRING_DESCRIPTOR.equals(descriptor)) { + return DEFAULT; + } + + return formatters.computeIfAbsent(descriptor, it -> { + + if (!conversionService.canConvert(descriptor, STRING_DESCRIPTOR)) { + return DEFAULT; + } + + return source -> { + + Object result = conversionService.convert(source, descriptor, STRING_DESCRIPTOR); + + if (result == null) { + throw new IllegalArgumentException(String.format("Conversion of value %s resulted in null!", source)); + } + + return (String) result; + }; + }); + } + } + /** * {@link HandlerMethodParameter} implementation to work with {@link RequestParam}. * diff --git a/src/main/java/org/springframework/hateoas/server/mvc/WebMvcLinkBuilderFactory.java b/src/main/java/org/springframework/hateoas/server/mvc/WebMvcLinkBuilderFactory.java index fdbfc983..2f4d365f 100644 --- a/src/main/java/org/springframework/hateoas/server/mvc/WebMvcLinkBuilderFactory.java +++ b/src/main/java/org/springframework/hateoas/server/mvc/WebMvcLinkBuilderFactory.java @@ -152,21 +152,25 @@ public class WebMvcLinkBuilderFactory implements MethodLinkBuilderFactory { String[] primaryParams = SpringAffordanceBuilder.DISCOVERER.getParams(invocation.getMethod()); - ParamsRequestCondition paramsRequestCondition = new ParamsRequestCondition(primaryParams); - for (NameValueExpression expression : paramsRequestCondition.getExpressions()) { + if (primaryParams.length > 0) { - if (expression.isNegated()) { - continue; + ParamsRequestCondition paramsRequestCondition = new ParamsRequestCondition(primaryParams); + + for (NameValueExpression expression : paramsRequestCondition.getExpressions()) { + + if (expression.isNegated()) { + continue; + } + + String value = expression.getValue(); + + if (value == null) { + continue; + } + + builder.queryParam(expression.getName(), value); } - - String value = expression.getValue(); - - if (value == null) { - continue; - } - - builder.queryParam(expression.getName(), value); } MethodParameters parameters = MethodParameters.of(invocation.getMethod());