From 44d832158a60ba518565869e868052f72d0a972e Mon Sep 17 00:00:00 2001 From: dreis2211 Date: Sat, 4 May 2019 18:47:41 +0200 Subject: [PATCH 1/2] Optimize CacheKey handling for immutable sources Update `SpringIterableConfigurationPropertySource` so that cache keys do not need to be checked if property sources are immutable. See gh-16717 --- ...ngIterableConfigurationPropertySource.java | 32 ++++++++++++++++--- ...rableConfigurationPropertySourceTests.java | 15 ++++++++- 2 files changed, 41 insertions(+), 6 deletions(-) diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java index fda347cadce0..909c38f890f5 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java @@ -25,6 +25,7 @@ import java.util.Set; import java.util.stream.Stream; +import org.springframework.boot.env.OriginTrackedMapPropertySource; import org.springframework.core.env.EnumerablePropertySource; import org.springframework.core.env.MapPropertySource; import org.springframework.core.env.PropertySource; @@ -192,21 +193,37 @@ private static final class CacheKey { private final Object key; - private CacheKey(Object key) { + private final int size; + + private final boolean unmodifiableKey; + + private CacheKey(Object key, boolean unmodifiableKey) { this.key = key; + this.size = calculateSize(key); + this.unmodifiableKey = unmodifiableKey; } public CacheKey copy() { - return new CacheKey(copyKey(this.key)); + return new CacheKey(copyKey(this.key), this.unmodifiableKey); } private Object copyKey(Object key) { + if (this.unmodifiableKey) { + return key; + } if (key instanceof Set) { return new HashSet((Set) key); } return ((String[]) key).clone(); } + private int calculateSize(Object key) { + if (key instanceof Set) { + return ((Set) key).size(); + } + return ((String[]) key).length; + } + @Override public boolean equals(Object obj) { if (this == obj) { @@ -215,7 +232,11 @@ public boolean equals(Object obj) { if (obj == null || getClass() != obj.getClass()) { return false; } - return ObjectUtils.nullSafeEquals(this.key, ((CacheKey) obj).key); + CacheKey otherCacheKey = (CacheKey) obj; + if (this.size != otherCacheKey.size) { + return false; + } + return ObjectUtils.nullSafeEquals(this.key, otherCacheKey.key); } @Override @@ -225,9 +246,10 @@ public int hashCode() { public static CacheKey get(EnumerablePropertySource source) { if (source instanceof MapPropertySource) { - return new CacheKey(((MapPropertySource) source).getSource().keySet()); + return new CacheKey(((MapPropertySource) source).getSource().keySet(), + source instanceof OriginTrackedMapPropertySource); } - return new CacheKey(source.getPropertyNames()); + return new CacheKey(source.getPropertyNames(), false); } } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java index 96a96be3d207..eb1bb8135e71 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java @@ -25,6 +25,7 @@ import org.junit.Test; +import org.springframework.boot.env.OriginTrackedMapPropertySource; import org.springframework.boot.origin.Origin; import org.springframework.boot.origin.OriginLookup; import org.springframework.core.env.EnumerablePropertySource; @@ -160,7 +161,7 @@ public void containsDescendantOfShouldCheckSourceNames() { } @Test - public void propertySourceKeyDataChangeInvalidatesCache() { + public void simpleMapPropertySourceKeyDataChangeInvalidatesCache() { // gh-13344 Map map = new LinkedHashMap<>(); map.put("key1", "value1"); @@ -184,6 +185,18 @@ public void concurrentModificationExceptionInvalidatesCache() { source, DefaultPropertyMapper.INSTANCE); assertThat(adapter.stream().count()).isEqualTo(2); map.setThrowException(true); + } + + public void originTrackedMapPropertySourceKeyAdditionInvalidatesCache() { + // gh-13344 + Map map = new LinkedHashMap<>(); + map.put("key1", "value1"); + map.put("key2", "value2"); + EnumerablePropertySource source = new OriginTrackedMapPropertySource("test", + map); + SpringIterableConfigurationPropertySource adapter = new SpringIterableConfigurationPropertySource( + source, DefaultPropertyMapper.INSTANCE); + assertThat(adapter.stream().count()).isEqualTo(2); map.put("key3", "value3"); assertThat(adapter.stream().count()).isEqualTo(3); } From 3bfc9235df8fb2a921866c240a05d1dca46ea1b8 Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Mon, 3 Jun 2019 17:22:19 -0700 Subject: [PATCH 2/2] Polish "Optimize CacheKey handling for immutable sources" Make immutable properties more explicit and trust that they truly won't change. See gh-16717 --- ...ngIterableConfigurationPropertySource.java | 50 ++++++++++--------- .../env/OriginTrackedMapPropertySource.java | 31 +++++++++++- .../env/PropertiesPropertySourceLoader.java | 4 +- .../boot/env/YamlPropertySourceLoader.java | 2 +- ...rableConfigurationPropertySourceTests.java | 14 ++++++ 5 files changed, 73 insertions(+), 28 deletions(-) diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java index 909c38f890f5..93bc099fd0ec 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java @@ -29,6 +29,7 @@ import org.springframework.core.env.EnumerablePropertySource; import org.springframework.core.env.MapPropertySource; import org.springframework.core.env.PropertySource; +import org.springframework.core.env.StandardEnvironment; import org.springframework.core.env.SystemEnvironmentPropertySource; import org.springframework.util.ObjectUtils; @@ -191,39 +192,29 @@ public void setMappings(PropertyMapping[] mappings) { private static final class CacheKey { - private final Object key; - - private final int size; + private static final CacheKey IMMUTABLE_PROPERTY_SOURCE = new CacheKey( + new Object[0]); - private final boolean unmodifiableKey; + private final Object key; - private CacheKey(Object key, boolean unmodifiableKey) { + private CacheKey(Object key) { this.key = key; - this.size = calculateSize(key); - this.unmodifiableKey = unmodifiableKey; } public CacheKey copy() { - return new CacheKey(copyKey(this.key), this.unmodifiableKey); + if (this == IMMUTABLE_PROPERTY_SOURCE) { + return IMMUTABLE_PROPERTY_SOURCE; + } + return new CacheKey(copyKey(this.key)); } private Object copyKey(Object key) { - if (this.unmodifiableKey) { - return key; - } if (key instanceof Set) { return new HashSet((Set) key); } return ((String[]) key).clone(); } - private int calculateSize(Object key) { - if (key instanceof Set) { - return ((Set) key).size(); - } - return ((String[]) key).length; - } - @Override public boolean equals(Object obj) { if (this == obj) { @@ -233,9 +224,6 @@ public boolean equals(Object obj) { return false; } CacheKey otherCacheKey = (CacheKey) obj; - if (this.size != otherCacheKey.size) { - return false; - } return ObjectUtils.nullSafeEquals(this.key, otherCacheKey.key); } @@ -245,11 +233,25 @@ public int hashCode() { } public static CacheKey get(EnumerablePropertySource source) { + if (isImmutable(source)) { + return IMMUTABLE_PROPERTY_SOURCE; + } if (source instanceof MapPropertySource) { - return new CacheKey(((MapPropertySource) source).getSource().keySet(), - source instanceof OriginTrackedMapPropertySource); + MapPropertySource mapPropertySource = (MapPropertySource) source; + return new CacheKey(mapPropertySource.getSource().keySet()); + } + return new CacheKey(source.getPropertyNames()); + } + + private static boolean isImmutable(EnumerablePropertySource source) { + if (source instanceof OriginTrackedMapPropertySource) { + return ((OriginTrackedMapPropertySource) source).isImmutable(); + } + if (StandardEnvironment.SYSTEM_ENVIRONMENT_PROPERTY_SOURCE_NAME + .equals(source.getName())) { + return source.getSource() == System.getenv(); } - return new CacheKey(source.getPropertyNames(), false); + return false; } } diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/env/OriginTrackedMapPropertySource.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/env/OriginTrackedMapPropertySource.java index 767e06c3b39f..d7b130e293e1 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/env/OriginTrackedMapPropertySource.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/env/OriginTrackedMapPropertySource.java @@ -22,6 +22,7 @@ import org.springframework.boot.origin.OriginLookup; import org.springframework.boot.origin.OriginTrackedValue; import org.springframework.core.env.MapPropertySource; +import org.springframework.core.env.PropertySource; /** * {@link OriginLookup} backed by a {@link Map} containing {@link OriginTrackedValue @@ -35,9 +36,28 @@ public final class OriginTrackedMapPropertySource extends MapPropertySource implements OriginLookup { - @SuppressWarnings({ "unchecked", "rawtypes" }) + private final boolean immutable; + + /** + * Create a new {@link OriginTrackedMapPropertySource} instance. + * @param name the property source name + * @param source the underlying map source + */ + @SuppressWarnings("rawtypes") public OriginTrackedMapPropertySource(String name, Map source) { + this(name, source, false); + } + + /** + * Create a new {@link OriginTrackedMapPropertySource} instance. + * @param name the property source name + * @param source the underlying map source + * @param immutable if the underlying source is immutable and guaranteed not to change + */ + @SuppressWarnings({ "unchecked", "rawtypes" }) + public OriginTrackedMapPropertySource(String name, Map source, boolean immutable) { super(name, source); + this.immutable = immutable; } @Override @@ -58,4 +78,13 @@ public Origin getOrigin(String name) { return null; } + /** + * Return {@code true} if this {@link PropertySource} is immutable and has contents + * that will never change. + * @return if the property source is read only + */ + public boolean isImmutable() { + return this.immutable; + } + } diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/env/PropertiesPropertySourceLoader.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/env/PropertiesPropertySourceLoader.java index 178a966adde1..00bf634e75c3 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/env/PropertiesPropertySourceLoader.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/env/PropertiesPropertySourceLoader.java @@ -48,8 +48,8 @@ public List> load(String name, Resource resource) if (properties.isEmpty()) { return Collections.emptyList(); } - return Collections - .singletonList(new OriginTrackedMapPropertySource(name, properties)); + return Collections.singletonList(new OriginTrackedMapPropertySource(name, + Collections.unmodifiableMap(properties), true)); } @SuppressWarnings({ "unchecked", "rawtypes" }) diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/env/YamlPropertySourceLoader.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/env/YamlPropertySourceLoader.java index d49776a683ef..ecd544239519 100755 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/env/YamlPropertySourceLoader.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/env/YamlPropertySourceLoader.java @@ -55,7 +55,7 @@ public List> load(String name, Resource resource) for (int i = 0; i < loaded.size(); i++) { String documentNumber = (loaded.size() != 1) ? " (document #" + i + ")" : ""; propertySources.add(new OriginTrackedMapPropertySource(name + documentNumber, - loaded.get(i))); + Collections.unmodifiableMap(loaded.get(i)), true)); } return propertySources; } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java index eb1bb8135e71..acaf152f6c04 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java @@ -201,6 +201,20 @@ public void originTrackedMapPropertySourceKeyAdditionInvalidatesCache() { assertThat(adapter.stream().count()).isEqualTo(3); } + public void readOnlyOriginTrackedMapPropertySourceKeyAdditionDoesNotInvalidateCache() { + // gh-16717 + Map map = new LinkedHashMap<>(); + map.put("key1", "value1"); + map.put("key2", "value2"); + EnumerablePropertySource source = new OriginTrackedMapPropertySource("test", + map, true); + SpringIterableConfigurationPropertySource adapter = new SpringIterableConfigurationPropertySource( + source, DefaultPropertyMapper.INSTANCE); + assertThat(adapter.stream().count()).isEqualTo(2); + map.put("key3", "value3"); + assertThat(adapter.stream().count()).isEqualTo(2); + } + /** * Test {@link PropertySource} that's also an {@link OriginLookup}. */