Scale hotspot in DefaultSingletonBeanRegistry#registerDependentBean

[SameerKhan15]

I work for an enterprise company and we are working on a very high scale use case that involves request throughput in the range of several thousand per sec per app server. We have been observing the current locking scheme in registerDependentBean as a scale hotspot.

So, I wrote ConcurrentLinkedHashSet as Custom Collection (see git link below). And we have tested it with the below change and the hotspot gets solved.

Version#1: This is already scale tested in our workload and yields very good scale, and we have not observed any issues with this change, for at least our workload

...
private final Map<String, ConcurrentLinkedHashSet<String>> dependentBeanMap = new ConcurrentHashMap<>(64);

private final Map<String,ConcurrentLinkedHashSet<String>> dependenciesForBeanMap = new ConcurrentHashMap<>(64);
...

public void registerDependentBean(String beanName, String dependentBeanName) {
    ...
    Set<String> dependentBeans = this.dependentBeanMap.computeIfAbsent(canonicalName, k -> new 
                                                                                                                         ConcurrentLinkedHashSet<>());
	if (!dependentBeans.add(dependentBeanName)) {
		return;
	}
     ...
    
     Set<String> dependenciesForBean =
      this.dependenciesForBeanMap.computeIfAbsent(dependentBeanName, k -> new 
                                                                                                                          ConcurrentLinkedHashSet<>());
     dependenciesForBean.add(canonicalName);
     
}

Version#2: A more "conservative" approach. We have not scale tested it yet but are in the process of doing so.

private boolean checkForBeanExistence(Map<String, ConcurrentLinkedHashSet<String>> map, String key, String val) {	
	Set<String> values = map.get(key);	
	if (values != null && values.contains(val)) {
		return true;	
	}
		
	return false;
}

public void registerDependentBean(String beanName, String dependentBeanName) {
	String canonicalName = canonicalName(beanName);
	if (!checkForBeanExistence(this.dependentBeanMap, canonicalName, dependentBeanName)) {
		synchronized(this.dependentBeanMap) {
			Set<String> dependentBeans = this.dependentBeanMap.computeIfAbsent(canonicalName, k 
                                                                                                                -> new ConcurrentLinkedHashSet<>());
			if (!dependentBeans.add(dependentBeanName)) {
				return;
			}
		}
	}
		

	if (!checkForBeanExistence(this.dependenciesForBeanMap, dependentBeanName, canonicalName)) {
		synchronized (this.dependenciesForBeanMap) {
			Set<String> dependenciesForBean = 
                                             this.dependenciesForBeanMap.computeIfAbsent(dependentBeanName, k -> new 
                                                                                                                                ConcurrentLinkedHashSet<>());
			dependenciesForBean.add(canonicalName);
		}
	}
}

So, I wanted to gauge the appetite for working through a proposal like the above. The biggest question is the functional correctness of a change like this. I ran these through spring tests and they all pass. And we have tested Version#1 in our scale workload and that works too. What other tests would we need to evaluate this? And if you see any problems with the above approach, we can have a discussion here.

[SameerKhan15]

And, if this commit is meant to solve above hotspot, let me know that as well. Thanks

[jhoeller]

@SameerKhan15 are you seeing this with non-singleton beans, I assume? Does it commonly show against repeated creation of the same bean (or the same few beans) in your scenario?

Admittedly, registerDependentBean is designed for use with singletons on startup, as well as lazy-init singletons later on, but not optimized for prototype beans or scoped beans at all. Since registerDependentBean only really has an effect on first-time creation of an affected bean, we could try to avoid unnecessary calls on repeated creation of non-singleton beans, not hitting those data structures at all in that case.

[SameerKhan15]

Thanks for the explanation. I will get back to you on the singleton / non-singleton question shortly
So if i understand this correctly -
-) The singleton beans should NOT result in registerDependentBean to be a hotspot, because it would only be expected to be invoked once per bean
-) For non-singleton beans, there does exist an opportunity to eliminate repeated invocation of registerDependentBean IF the hotspot use case involves non-singleton beans

Is this correct?

And does this commit relevant to any of the above?

[SameerKhan15]

@jhoeller these are singleton beans.

We saw the hotspot in 6.1.x version. Is it fixed (via this commit) in a later version (6.2.x)?

[jhoeller]

Ah so those are singletons behind a @Lazy injection point which on 6.1.x re-resolves the target bean for every call. While that commit was not meant to address the registerDependentBean hotspot specifically, it does indeed bypass the repeated calls for a cached singleton target.

From that perspective, your hotspot in that particular arrangement can be considered fixed on 6.2.x indeed. Design-wise, avoiding repeated registerDependentBean calls is preferable to optimizing lock contention in the registerDependentBean implementation.

Note that this hotspot is specific to @Lazy injection points, if I understand your scenario correctly. For direct (non-lazy) dependency injection, no such runtime hotspot should emerge on 6.1.x to begin with.

[SameerKhan15]

Yes, all these beans are injected via @lazy annotation. So for now, we would consider this as fixed in 6.2.x. It will be next week before we can validate this. We can keep this open until we have done the verification, or close it and re-open if we still see the issue. Let me know. And thanks for the help.

[jhoeller]

@SameerKhan15 next week is perfectly fine, no hurry. Let's leave this issue open for the time being.

[SameerKhan15]

This is fixed. Closing it.