Polish "Optimize CacheKey handling for immutable sources"

Make immutable properties more explicit and trust that they truly won't change. See gh-16717
spring-projects · Jun 4, 2019 · 3bfc923 · 3bfc923
1 parent 44d8321
commit 3bfc923
Show file tree

Hide file tree

Showing 5 changed files with 73 additions and 28 deletions.
diff --git a/...ngframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java b/...ngframework/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<Object>((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/...pring-boot/src/main/java/org/springframework/boot/env/OriginTrackedMapPropertySource.java b/...pring-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<String> {
 
-	@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/...pring-boot/src/main/java/org/springframework/boot/env/PropertiesPropertySourceLoader.java b/...pring-boot/src/main/java/org/springframework/boot/env/PropertiesPropertySourceLoader.java
@@ -48,8 +48,8 @@ public List<PropertySource<?>> 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/...ject/spring-boot/src/main/java/org/springframework/boot/env/YamlPropertySourceLoader.java b/...ject/spring-boot/src/main/java/org/springframework/boot/env/YamlPropertySourceLoader.java
@@ -55,7 +55,7 @@ public List<PropertySource<?>> 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/...mework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java b/...mework/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<String, Object> 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}.
 	 */