Memoize system environment resolution in NavigableMapConfig

grails-core/src/main/groovy/org/grails/config/NavigableMapConfig.java

@@ -20,14 +20,17 @@
 
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.IdentityHashMap;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Properties;
 import java.util.Set;
 import java.util.StringTokenizer;
+import java.util.concurrent.ConcurrentHashMap;
 
 import groovy.util.ConfigObject;
 import org.codehaus.groovy.runtime.DefaultGroovyMethods;
@@ -53,8 +56,17 @@
 @Deprecated
 public abstract class NavigableMapConfig implements Config {
     protected static final Logger LOG = LoggerFactory.getLogger(NavigableMapConfig.class);
+
+    /**
+     * Upper bound on each key-derived cache. Configuration keys come from a small fixed set of source
+     * literals; the bound only guards against a caller synthesising unbounded key strings at runtime.
+     */
+    private static final int MAX_CACHED_KEYS = 2048;
+
     protected ClassLoader classLoader = Thread.currentThread().getContextClassLoader();
     protected ConfigurableConversionService conversionService = new DefaultConversionService();
+    private final ConcurrentHashMap<String, Optional<String>> systemEnvironmentCache = new ConcurrentHashMap<>();
+    private final ConcurrentHashMap<String, List<String>> dotNotatedKeyCache = new ConcurrentHashMap<>();
     protected NavigableMap configMap = new NavigableMap() {
         @Override
         protected Object mergeMapEntry(NavigableMap targetMap, String sourceKey, Object newValue) {
@@ -231,8 +243,26 @@ public <T> T getProperty(String key, Class<T> targetType, T defaultValue) {
     }
 
     private Object findInSystemEnvironment(String key) {
+        if (key == null) {
+            return null;
+        }
+        Optional<String> cached = systemEnvironmentCache.get(key);
+        if (cached != null) {
+            return cached.orElse(null);
+        }
         String propertyName = resolvePropertyName(key);
-        return propertyName != null ? System.getenv(propertyName) : null;
+        String value = propertyName != null ? System.getenv(propertyName) : null;
+        // Resolving a key to its environment value depends only on the key and on the process
+        // environment, which is fixed for the lifetime of this config, so the result is memoized per
+        // instance. Without this, every getProperty() call spends up to eight System.getenv() probes
+        // and six string allocations in resolvePropertyName()/checkPropertyName() re-deriving a
+        // constant answer. The cache is deliberately per-instance rather than static: tests that
+        // install environment variables reflectively build a fresh config afterwards and must observe
+        // the environment as it stands then.
+        if (systemEnvironmentCache.size() < MAX_CACHED_KEYS) {
+            systemEnvironmentCache.put(key, Optional.ofNullable(value));
+        }
+        return value;
     }
 
     private String resolvePropertyName(String name) {
@@ -404,8 +434,17 @@ private Object getValueWithDotNotatedKeySupport(NavigableMap configMap, String k
             return null;
         }
 
-        List<String> keys = convertTokensIntoArrayList(new StringTokenizer(key, "."));
-        if (keys.size() == 0) {
+        List<String> keys = dotNotatedKeyCache.get(key);
+        if (keys == null) {
+            keys = convertTokensIntoArrayList(new StringTokenizer(key, "."));
+            // Splitting a dotted key is a pure function of the key, and configuration keys come from a
+            // small fixed set of source literals, so the token list is cached rather than rebuilt (with
+            // a fresh StringTokenizer and ArrayList) on every lookup.
+            if (dotNotatedKeyCache.size() < MAX_CACHED_KEYS) {
+                dotNotatedKeyCache.put(key, keys);
+            }
+        }
+        if (keys.isEmpty()) {
             return null;
         }
 
@@ -421,10 +460,10 @@ private Object getValueWithDotNotatedKeySupport(NavigableMap configMap, String k
     }
 
     private List<String> convertTokensIntoArrayList(StringTokenizer st) {
-        List<String> elements = new ArrayList<>();
+        List<String> elements = new ArrayList<>(st.countTokens());
         while (st.hasMoreTokens()) {
             elements.add(st.nextToken());
         }
-        return elements;
+        return Collections.unmodifiableList(elements);
     }
 }

grails-core/src/test/groovy/org/grails/config/SystemEnvironmentConfigSpec.groovy

@@ -73,6 +73,56 @@ property-with_mixed.symbols: from-yml
         'property-with_mixed.symbols' | 'PROPERTY_WITH_MIXED_SYMBOLS' | 'from-env'
     }
 
+    void 'a property read more than once keeps resolving to the system environment value'() {
+        given: 'configuration that is overridden by the environment'
+        def config = configFor('property.with.period: from-yml')
+        modifiableSystemEnvironment.put('PROPERTY_WITH_PERIOD', 'from-env')
+
+        expect: 'every read resolves to the environment value, not just the first'
+        config.getProperty('property.with.period') == 'from-env'
+        config.getProperty('property.with.period') == 'from-env'
+        config.getProperty('property.with.period') == 'from-env'
+
+        cleanup:
+        modifiableSystemEnvironment.remove('PROPERTY_WITH_PERIOD')
+    }
+
+    void 'a property that has already been read still reflects later configuration changes'() {
+        given: 'a property that has been read once'
+        def config = configFor('some.nested.value: original')
+        assert config.getProperty('some.nested.value') == 'original'
+
+        when: 'the configuration is changed'
+        config.merge(['some.nested.value': 'updated'])
+
+        then: 'the new value is returned rather than the previously resolved one'
+        config.getProperty('some.nested.value') == 'updated'
+    }
+
+    void 'a config created after an environment variable is installed observes it'() {
+        given: 'a config created and read before the variable exists'
+        def before = configFor('late.bound.property: from-yml')
+        assert before.getProperty('late.bound.property') == 'from-yml'
+
+        when: 'the variable is installed and a new config is created'
+        modifiableSystemEnvironment.put('LATE_BOUND_PROPERTY', 'from-env')
+        def after = configFor('late.bound.property: from-yml')
+
+        then: 'the new config resolves to the environment value'
+        after.getProperty('late.bound.property') == 'from-env'
+
+        cleanup:
+        modifiableSystemEnvironment.remove('LATE_BOUND_PROPERTY')
+    }
+
+    private static PropertySourcesConfig configFor(String yaml) {
+        def yamlPropertiesSource = new YamlPropertySourceLoader()
+                .load('application.yml', new ByteArrayResource(yaml.bytes, 'test.yml'), null)
+        def propertySources = new MutablePropertySources()
+        propertySources.addFirst(yamlPropertiesSource.first())
+        new PropertySourcesConfig(propertySources)
+    }
+
     // From https://github.com/spring-projects/spring-framework/blob/4.3.x/spring-core/src/test/java/org/springframework/core/env/StandardEnvironmentTests.java#L492
     @SuppressWarnings("unchecked")
     static Map<String, String> getModifiableSystemEnvironment() {