Skip to content

Commit

Permalink
Allocation improvements: (#998)
Browse files Browse the repository at this point in the history 
- 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
  • Loading branch information
@radcortez
radcortez authored Sep 19, 2023
1 parent e06b7b7 commit 32de20e
Show file tree
Hide file tree
Showing 9 changed files with 82 additions and 57 deletions.
24 changes: 12 additions & 12 deletions common/src/main/java/io/smallrye/config/common/utils/StringUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down
35 changes: 17 additions & 18 deletions implementation/src/main/java/io/smallrye/config/ConfigMappingContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -231,29 +232,27 @@ void unknownProperty(final String unknownProperty) {
unknownProperties.add(unknownProperty);
}

void validateUnknown(final boolean validateUnknown) {
Set<String> 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())));
}
}
}
Expand Down
7 changes: 5 additions & 2 deletions implementation/src/main/java/io/smallrye/config/ConfigMappingProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<ConfigValidationException.Problem> problems = context.getProblems();
if (!problems.isEmpty()) {
Expand Down
29 changes: 19 additions & 10 deletions implementation/src/main/java/io/smallrye/config/ConfigValue.java
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -105,8 +106,12 @@ public String getExtendedExpressionHandler() {
return extendedExpressionHandler;
}

boolean hasProblems() {
return problems != null && !problems.isEmpty();
}

List<Problem> getProblems() {
return Collections.unmodifiableList(problems);
return hasProblems() ? unmodifiableList(problems) : emptyList();
}

public ConfigValue withName(final String name) {
Expand Down Expand Up @@ -149,10 +154,6 @@ public ConfigValue withProblems(final List<Problem> 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) {
Expand Down Expand Up @@ -218,7 +219,7 @@ public static class ConfigValueBuilder {
private int configSourcePosition;
private int lineNumber = -1;
private String extendedExpressionHandler;
private final List<Problem> problems = new ArrayList<>();
private List<Problem> problems;

public ConfigValueBuilder withName(final String name) {
this.name = name;
Expand Down Expand Up @@ -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<Problem> 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);
Expand Down
2 changes: 1 addition & 1 deletion implementation/src/main/java/io/smallrye/config/DefaultValuesConfigSource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ public Set<String> 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<String, String> properties) {
Expand Down
17 changes: 11 additions & 6 deletions implementation/src/main/java/io/smallrye/config/ExpressionConfigSourceInterceptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<ResolveContext<RuntimeException>, StringBuilder>() {
Expand All @@ -61,28 +66,28 @@ public void accept(ResolveContext<RuntimeException> 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;
}

// 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();
}

/**
Expand Down
17 changes: 13 additions & 4 deletions implementation/src/main/java/io/smallrye/config/KeyMap.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,13 +111,21 @@ public KeyMap<V> find(final String path) {
}

public KeyMap<V> find(final NameIterator ni) {
return find(ni, new StringBuilder());
}

KeyMap<V> find(final NameIterator ni, final StringBuilder sb) {
if (!ni.hasNext()) {
return this;
}
String seg = ni.getNextSegment();
ni.next();
KeyMap<V> next = findOrDefault(seg);
return next == null ? null : next.find(ni);
KeyMap<V> next = findOrDefault(ni.getNextSegment(sb).toString());
if (next != null) {
ni.next();
sb.setLength(0);
return next.find(ni, sb);
} else {
return null;
}
}

public KeyMap<V> find(final Iterator<String> iter) {
Expand Down Expand Up @@ -238,6 +246,7 @@ public V findRootValue(final String path) {
}

public V findRootValue(final NameIterator ni) {
StringBuilder sb = new StringBuilder();
KeyMap<V> result = find(ni);
return result == null ? null : result.getRootValue();
}
Expand Down
5 changes: 2 additions & 3 deletions implementation/src/main/java/io/smallrye/config/SmallRyeConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -260,13 +260,12 @@ public <T> T getValue(String name, Converter<T> converter) {
* the specified type).
*/
public <T> T convertValue(ConfigValue configValue, Converter<T> converter) {
List<ConfigValidationException.Problem> 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<RuntimeException> exception = problem.getException();
if (exception.isPresent()) {
throw exception.get();
Expand Down
3 changes: 2 additions & 1 deletion implementation/src/test/java/io/smallrye/config/ConfigMappingInterfaceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down

0 comments on commit 32de20e

Please sign in to comment.