From 32de20e0ffe564cec7494d983768d72fe07e5fdd Mon Sep 17 00:00:00 2001 From: Roberto Cortez Date: Tue, 19 Sep 2023 13:26:40 +0100 Subject: [PATCH] Allocation improvements: (#998) - Don't compile Expression if not required - Improved algorithm to detect false unknowns - Check only for wildcards in Defaults if there are values - Reuse StringBuilder when looking in KeyMap --- .../config/common/utils/StringUtil.java | 24 ++++++------- .../smallrye/config/ConfigMappingContext.java | 35 +++++++++---------- .../config/ConfigMappingProvider.java | 7 ++-- .../java/io/smallrye/config/ConfigValue.java | 29 +++++++++------ .../config/DefaultValuesConfigSource.java | 2 +- .../ExpressionConfigSourceInterceptor.java | 17 +++++---- .../main/java/io/smallrye/config/KeyMap.java | 17 ++++++--- .../io/smallrye/config/SmallRyeConfig.java | 5 ++- .../config/ConfigMappingInterfaceTest.java | 3 +- 9 files changed, 82 insertions(+), 57 deletions(-) diff --git a/common/src/main/java/io/smallrye/config/common/utils/StringUtil.java b/common/src/main/java/io/smallrye/config/common/utils/StringUtil.java index d19909cba..4a007c7fa 100644 --- a/common/src/main/java/io/smallrye/config/common/utils/StringUtil.java +++ b/common/src/main/java/io/smallrye/config/common/utils/StringUtil.java @@ -102,36 +102,36 @@ private static char toAsciiLowerCase(char c) { return isAsciiUpperCase(c) ? (char) (c + 32) : c; } - public static boolean equalsIgnoreCaseReplacingNonAlphanumericByUnderscores(final String property, - CharSequence mappedProperty) { - int length = mappedProperty.length(); - if (property.length() != mappedProperty.length()) { + public static boolean equalsIgnoreCaseReplacingNonAlphanumericByUnderscores(final String envProperty, + CharSequence dottedProperty) { + int length = dottedProperty.length(); + if (envProperty.length() != dottedProperty.length()) { // special-case/slow-path - if (length == 0 || property.length() != mappedProperty.length() + 1) { + if (length == 0 || envProperty.length() != dottedProperty.length() + 1) { return false; } - if (mappedProperty.charAt(length - 1) == '"' && - property.charAt(length - 1) == '_' && property.charAt(length) == '_') { - length = mappedProperty.length() - 1; + if (dottedProperty.charAt(length - 1) == '"' && + envProperty.charAt(length - 1) == '_' && envProperty.charAt(length) == '_') { + length = dottedProperty.length() - 1; } else { return false; } } for (int i = 0; i < length; i++) { - char ch = mappedProperty.charAt(i); + char ch = dottedProperty.charAt(i); if (!isAsciiLetterOrDigit(ch)) { - if (property.charAt(i) != '_') { + if (envProperty.charAt(i) != '_') { return false; } continue; } - final char pCh = property.charAt(i); + final char pCh = envProperty.charAt(i); // in theory property should be ascii too, but better play safe if (pCh < 128) { if (toAsciiLowerCase(pCh) != toAsciiLowerCase(ch)) { return false; } - } else if (Character.toLowerCase(property.charAt(i)) != Character.toLowerCase(ch)) { + } else if (Character.toLowerCase(envProperty.charAt(i)) != Character.toLowerCase(ch)) { return false; } } diff --git a/implementation/src/main/java/io/smallrye/config/ConfigMappingContext.java b/implementation/src/main/java/io/smallrye/config/ConfigMappingContext.java index b8135d206..d09afe7b0 100644 --- a/implementation/src/main/java/io/smallrye/config/ConfigMappingContext.java +++ b/implementation/src/main/java/io/smallrye/config/ConfigMappingContext.java @@ -28,6 +28,7 @@ import io.smallrye.config.ConfigMappingInterface.CollectionProperty; import io.smallrye.config.ConfigMappingInterface.NamingStrategy; +import io.smallrye.config.common.utils.StringUtil; /** * A mapping context. This is used by generated classes during configuration mapping, and is released once the configuration @@ -231,29 +232,27 @@ void unknownProperty(final String unknownProperty) { unknownProperties.add(unknownProperty); } - void validateUnknown(final boolean validateUnknown) { - Set usedProperties = new HashSet<>(); - for (String property : config.getPropertyNames()) { - if (unknownProperties.contains(property)) { - continue; - } - - usedProperties.add(replaceNonAlphanumericByUnderscores(property)); - } - usedProperties.removeAll(unknownProperties); - - for (String property : unknownProperties) { + void reportUnknown() { + // an unknown property may still be used if it was coming from the EnvSource + for (String unknownProperty : unknownProperties) { boolean found = false; - String envProperty = replaceNonAlphanumericByUnderscores(property); - for (String usedProperty : usedProperties) { - if (usedProperty.equalsIgnoreCase(envProperty)) { + String unknownEnvProperty = replaceNonAlphanumericByUnderscores(unknownProperty); + for (String userProperty : config.getPropertyNames()) { + if (unknownProperty.equals(userProperty)) { + continue; + } + + // Match another property with the same semantic meaning + if (StringUtil.equalsIgnoreCaseReplacingNonAlphanumericByUnderscores(unknownEnvProperty, userProperty)) { found = true; break; } } - if (!found && validateUnknown) { - ConfigValue configValue = config.getConfigValue(property); - problems.add(new Problem(ConfigMessages.msg.propertyDoesNotMapToAnyRoot(property, configValue.getLocation()))); + + if (!found) { + ConfigValue configValue = config.getConfigValue(unknownProperty); + problems.add(new Problem( + ConfigMessages.msg.propertyDoesNotMapToAnyRoot(unknownProperty, configValue.getLocation()))); } } } diff --git a/implementation/src/main/java/io/smallrye/config/ConfigMappingProvider.java b/implementation/src/main/java/io/smallrye/config/ConfigMappingProvider.java index 799b07789..28118edd8 100644 --- a/implementation/src/main/java/io/smallrye/config/ConfigMappingProvider.java +++ b/implementation/src/main/java/io/smallrye/config/ConfigMappingProvider.java @@ -994,8 +994,11 @@ private ConfigMappingContext mapConfigurationInternal(SmallRyeConfig config) thr } } - context.validateUnknown( - config.getOptionalValue(SMALLRYE_CONFIG_MAPPING_VALIDATE_UNKNOWN, boolean.class).orElse(this.validateUnknown)); + boolean validateUnknown = config.getOptionalValue(SMALLRYE_CONFIG_MAPPING_VALIDATE_UNKNOWN, boolean.class) + .orElse(this.validateUnknown); + if (validateUnknown) { + context.reportUnknown(); + } List problems = context.getProblems(); if (!problems.isEmpty()) { diff --git a/implementation/src/main/java/io/smallrye/config/ConfigValue.java b/implementation/src/main/java/io/smallrye/config/ConfigValue.java index 4300d711b..8da1e35b5 100644 --- a/implementation/src/main/java/io/smallrye/config/ConfigValue.java +++ b/implementation/src/main/java/io/smallrye/config/ConfigValue.java @@ -1,9 +1,10 @@ package io.smallrye.config; import static io.smallrye.config.ProfileConfigSourceInterceptor.convertProfile; +import static java.util.Collections.emptyList; +import static java.util.Collections.unmodifiableList; import java.util.ArrayList; -import java.util.Collections; import java.util.Comparator; import java.util.List; import java.util.Objects; @@ -105,8 +106,12 @@ public String getExtendedExpressionHandler() { return extendedExpressionHandler; } + boolean hasProblems() { + return problems != null && !problems.isEmpty(); + } + List getProblems() { - return Collections.unmodifiableList(problems); + return hasProblems() ? unmodifiableList(problems) : emptyList(); } public ConfigValue withName(final String name) { @@ -149,10 +154,6 @@ public ConfigValue withProblems(final List problems) { return from().withProblems(problems).build(); } - public ConfigValue withProblem(final Problem problem) { - return from().addProblem(problem).build(); - } - @Override public boolean equals(final Object o) { if (this == o) { @@ -218,7 +219,7 @@ public static class ConfigValueBuilder { private int configSourcePosition; private int lineNumber = -1; private String extendedExpressionHandler; - private final List problems = new ArrayList<>(); + private List problems; public ConfigValueBuilder withName(final String name) { this.name = name; @@ -266,22 +267,30 @@ public ConfigValueBuilder withExtendedExpressionHandler(final String extendedExp } public ConfigValueBuilder noProblems() { - this.problems.clear(); + this.problems = emptyList(); return this; } public ConfigValueBuilder withProblems(final List problems) { - this.problems.addAll(problems); + if (problems != null) { + if (this.problems == null) { + this.problems = new ArrayList<>(); + } + this.problems.addAll(problems); + } return this; } public ConfigValueBuilder addProblem(final Problem problem) { + if (this.problems == null) { + this.problems = new ArrayList<>(); + } this.problems.add(problem); return this; } public ConfigValue build() { - if (!this.problems.isEmpty()) { + if (problems != null && !problems.isEmpty()) { this.value = null; } return new ConfigValue(this); diff --git a/implementation/src/main/java/io/smallrye/config/DefaultValuesConfigSource.java b/implementation/src/main/java/io/smallrye/config/DefaultValuesConfigSource.java index af18896c1..eb36de9e3 100644 --- a/implementation/src/main/java/io/smallrye/config/DefaultValuesConfigSource.java +++ b/implementation/src/main/java/io/smallrye/config/DefaultValuesConfigSource.java @@ -27,7 +27,7 @@ public Set getPropertyNames() { public String getValue(final String propertyName) { String value = properties.get(propertyName); - return value == null ? wildcards.findRootValue(propertyName) : value; + return value == null && !wildcards.isEmpty() ? wildcards.findRootValue(propertyName) : value; } void addDefaults(final Map properties) { diff --git a/implementation/src/main/java/io/smallrye/config/ExpressionConfigSourceInterceptor.java b/implementation/src/main/java/io/smallrye/config/ExpressionConfigSourceInterceptor.java index 1174736e0..671b3db07 100644 --- a/implementation/src/main/java/io/smallrye/config/ExpressionConfigSourceInterceptor.java +++ b/implementation/src/main/java/io/smallrye/config/ExpressionConfigSourceInterceptor.java @@ -50,7 +50,12 @@ private ConfigValue getValue(final ConfigSourceInterceptorContext context, final return null; } - ConfigValue.ConfigValueBuilder value = configValue.from(); + // Avoid extra StringBuilder allocations from Expression + if (configValue.getValue().indexOf('$') == -1) { + return configValue; + } + + ConfigValue.ConfigValueBuilder valueBuilder = configValue.from(); Expression expression = Expression.compile(escapeDollarIfExists(configValue.getValue()), LENIENT_SYNTAX, NO_TRIM, NO_SMART_BRACES, DOUBLE_COLON); String expanded = expression.evaluate(new BiConsumer, StringBuilder>() { @@ -61,7 +66,7 @@ public void accept(ResolveContext resolveContext, StringBuilde // Requires a handler lookup int index = key.indexOf("::"); if (index != -1) { - value.withExtendedExpressionHandler(key.substring(0, index)); + valueBuilder.withExtendedExpressionHandler(key.substring(0, index)); stringBuilder.append(key, index + 2, key.length()); return; } @@ -69,20 +74,20 @@ public void accept(ResolveContext resolveContext, StringBuilde // Expression lookup ConfigValue resolve = getValue(context, key, depth + 1); if (resolve != null) { - if (resolve.getProblems().isEmpty()) { + if (!resolve.hasProblems()) { stringBuilder.append(resolve.getValue()); } else { - value.withProblems(resolve.getProblems()); + valueBuilder.withProblems(resolve.getProblems()); } } else if (resolveContext.hasDefault()) { resolveContext.expandDefault(); } else { - value.addProblem(new Problem(msg.expandingElementNotFound(key, configValue.getName()))); + valueBuilder.addProblem(new Problem(msg.expandingElementNotFound(key, configValue.getName()))); } } }); - return value.withValue(expanded).build(); + return valueBuilder.withValue(expanded).build(); } /** diff --git a/implementation/src/main/java/io/smallrye/config/KeyMap.java b/implementation/src/main/java/io/smallrye/config/KeyMap.java index dab559817..d5cea2bef 100644 --- a/implementation/src/main/java/io/smallrye/config/KeyMap.java +++ b/implementation/src/main/java/io/smallrye/config/KeyMap.java @@ -111,13 +111,21 @@ public KeyMap find(final String path) { } public KeyMap find(final NameIterator ni) { + return find(ni, new StringBuilder()); + } + + KeyMap find(final NameIterator ni, final StringBuilder sb) { if (!ni.hasNext()) { return this; } - String seg = ni.getNextSegment(); - ni.next(); - KeyMap next = findOrDefault(seg); - return next == null ? null : next.find(ni); + KeyMap next = findOrDefault(ni.getNextSegment(sb).toString()); + if (next != null) { + ni.next(); + sb.setLength(0); + return next.find(ni, sb); + } else { + return null; + } } public KeyMap find(final Iterator iter) { @@ -238,6 +246,7 @@ public V findRootValue(final String path) { } public V findRootValue(final NameIterator ni) { + StringBuilder sb = new StringBuilder(); KeyMap result = find(ni); return result == null ? null : result.getRootValue(); } diff --git a/implementation/src/main/java/io/smallrye/config/SmallRyeConfig.java b/implementation/src/main/java/io/smallrye/config/SmallRyeConfig.java index 1376fc189..f70854f53 100644 --- a/implementation/src/main/java/io/smallrye/config/SmallRyeConfig.java +++ b/implementation/src/main/java/io/smallrye/config/SmallRyeConfig.java @@ -260,13 +260,12 @@ public T getValue(String name, Converter converter) { * the specified type). */ public T convertValue(ConfigValue configValue, Converter converter) { - List problems = configValue.getProblems(); - if (!problems.isEmpty()) { + if (configValue.hasProblems()) { // TODO - Maybe it will depend on the problem, but we only get the expression NoSuchElement here for now if (Converters.isOptionalConverter(converter)) { configValue = configValue.noProblems(); } else { - ConfigValidationException.Problem problem = problems.get(0); + ConfigValidationException.Problem problem = configValue.getProblems().get(0); Optional exception = problem.getException(); if (exception.isPresent()) { throw exception.get(); diff --git a/implementation/src/test/java/io/smallrye/config/ConfigMappingInterfaceTest.java b/implementation/src/test/java/io/smallrye/config/ConfigMappingInterfaceTest.java index 95d7d0275..70d886b3e 100644 --- a/implementation/src/test/java/io/smallrye/config/ConfigMappingInterfaceTest.java +++ b/implementation/src/test/java/io/smallrye/config/ConfigMappingInterfaceTest.java @@ -847,7 +847,8 @@ void prefixPropertyInRootUnknown() { .withMapping(ServerPrefix.class, "server") .withSources(config("serverBoot", "server")) .withSources(config("server.host", "localhost", "server.port", "8080")) - .withSources(config("server.name", "localhost")); + .withSources(config("server.name", "localhost")) + .withSources(new EnvConfigSource(Map.of("SERVER_ALIAS", "alias"), 300)); ConfigValidationException exception = assertThrows(ConfigValidationException.class, builder::build); assertEquals("SRCFG00050: server.name in KeyValuesConfigSource does not map to any root",