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.
Comment From: SameerKhan15
And, if this commit is meant to solve above hotspot, let me know that as well. Thanks
Comment From: 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.
Comment From: 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?
Comment From: 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)?
Comment From: 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.
Comment From: 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.
Comment From: jhoeller
@SameerKhan15 next week is perfectly fine, no hurry. Let's leave this issue open for the time being.