Improve configuration property binding performance with extremely large input files

[wilkinsona]

Courtesy of @martinvisser, we have an example of the binder still being slow in 2.2 snapshots relative to 1.5's performance. The input YAML file is 7500 lines long so considerably bigger than the case we investigated with Initializr.

[dreis2211]

Most of the time is spent in ConfigurationPropertyName.elementEquals still. Or ConfigurationPropertyName.isAncestorOf depending on how you see it. Here's a screenshot from an application that simply tries to bind the large file:

(This is a snapshot in between, so don't trust the overall invocations or timings. The share of CPU usage is more important here)

[andresetevejob]

I want to investigate on this issue.can i do?

[dreis2211]

I'm currently working already on some improvements as well, @wilkinsona . See the referenced issue #16447 for one of those. I'm currently working on another one already, but I'm not confident enough in the solution yet.

[dreis2211]

Other than the already provided PRs I'm still trying around on this and found one interesting place that you might want to look at. When simply assigning the cache key in SpringIterableConfigurationPropertySource.getCache() instead of copying it, benchmarks yield the following results:

	Baseline	New
	13.386	8.062
	12.658	8.121
	12.530	8.365
	12.147	8.813
	12.016	8.401
	12.316	9.393
	11.928	8.388
	11.754	8.242
	11.663	8.674
	11.704	8.622
Mean	12.210	8.508
Range	1.723	1.331

Unfortunately, this is meant to fix #13344 and can thus not be removed at the moment. For the given example this means that AbstractSet.equals() is called on Sets with 5000 entries basically every time someone interacts with SpringIterableConfigurationPropertySource.getCache() - which is the majority of interaction.

I'm currently not able to try out things a lot, but I wanted to let you know at least. Maybe there is a way to change the CacheKey creation. Maybe there is a way to create something like an UnmodifiableMapPropertySource that might be helpful as well. After all, #13344 is about reacting to changes on the underlying map. If we can make sure that the underlying map is not subject of change, there is a possibility for better performance.

Cheers,
Christoph

[wilkinsona]

Ouch, that's a heavy price to pay for something that's only of benefit to a subset of users. Thanks for identifying it.

I wonder if we can use an == check against the key of any existing cache key and reuse it when it matches:

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 241ce2180b..50d3680336 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
@@ -44,7 +44,7 @@ import org.springframework.util.ObjectUtils;
 class SpringIterableConfigurationPropertySource extends SpringConfigurationPropertySource
                implements IterableConfigurationPropertySource {
 
-       private volatile Object cacheKey;
+       private volatile CacheKey cacheKey;
 
        private volatile Cache cache;
 
@@ -131,7 +131,7 @@ class SpringIterableConfigurationPropertySource extends SpringConfigurationPrope
        }
 
        private Cache getCache() {
-               CacheKey cacheKey = CacheKey.get(getPropertySource());
+               CacheKey cacheKey = CacheKey.get(this.cacheKey, getPropertySource());
                if (ObjectUtils.nullSafeEquals(cacheKey, this.cacheKey)) {
                        return this.cache;
                }
@@ -204,9 +204,14 @@ class SpringIterableConfigurationPropertySource extends SpringConfigurationPrope
                        return this.key.hashCode();
                }
 
-               public static CacheKey get(EnumerablePropertySource<?> source) {
+               public static CacheKey get(CacheKey existingCacheKey,
+                               EnumerablePropertySource<?> source) {
                        if (source instanceof MapPropertySource) {
-                               return new CacheKey(((MapPropertySource) source).getSource().keySet());
+                               Object key = ((MapPropertySource) source).getSource().keySet();
+                               if (existingCacheKey != null && existingCacheKey.key == key) {
+                                       return existingCacheKey;
+                               }
+                               return new CacheKey(key);
                        }
                        return new CacheKey(source.getPropertyNames());
                }

[wilkinsona]

@dreis2211 What's the Baseline in your numbers above? With 2.2.0.M2, I'm seeing times similar to those in your New column. Times with latest 2.2 snapshots plus the diff above applied are a bit quicker:

	Baseline	New
	8.176	8.037
	9.292	8.983
	8.128	7.794
	9.074	7.756
	8.543	8.493
	8.383	8.823
	8.631	8.525
	8.675	7.742
	8.563	7.859
	9.072	7.622
Mean	8.654	8.163
Range	1.164	1.361

[dreis2211]

Interesting - I tested with quite a few baselines. Basically I switch to a new one after one of my provided PRs was merged to master. So the current baseline is a 2.2.0 snapshot, but even with M2 I get very different results to yours.

	Baseline	New
	15.258	7.887
	12.686	7.335
	13.028	7.911
	11.813	9.204
	13.435	8.029
	12.673	8.041
	12.549	7.937
	12.681	7.954
	12.286	7.882
	12.565	7.995
Mean	12.897	8.018
Range	3.445	1.869

Just for completeness - I added a System.exit() call to the example ConfigurationPropertiesApplication so I don't get any port clashes. I wonder if that makes a difference (I wouldn't expect that at least).

To your suggestion. That shouldn't make a difference as we're still comparing a LinkedKeySet vs. a HashSet where == shouldn't apply.