Performance improvement on AbstractCachingViewResolver [SPR-3145]

[spring-projects-issues]

Reza Aliakbari opened SPR-3145 and commented

I got Spring 2.0.1 and I found a performance issue on this class that can slightly improve the performance of this object especially when there is a high load on this object.
On the method resolveViewName the code could be changed as follow

View view = (View) this.viewCache.get(cacheKey);
if (view == null) {
     synchronized (this.viewCache) {
          View view = (View) this.viewCache.get(cacheKey);
          if (view == null) {
           ......
          }
         .....
    }
}

I also see the following comment // No synchronization, as we can live with occasional double caching. That is correct, but with the upper solution we don't need to be worry of extra object problem and also we have good performance.

Cheers,
Reza

---

Affects: 2.0.1

Issue Links:

• AbstractCachingViewResolver - caching redirect views leads to memory leak [SPR-10065] #14698 AbstractCachingViewResolver - caching redirect views leads to memory leak

Referenced from: commits 06c6cbb

1 votes, 3 watchers

[spring-projects-issues]

Juergen Hoeller commented

Actually, the only 'bug' is that the old "no synchronization" comment is still in there... We do need to access the viewCache in a synchronized manner there, since the internal structure of the Map could else get corrupted. While we could try to do the View creation outside of a synchronized block, there isn't much to gain there, since all Views are usually already created in a high-load scenario anyway. Hence the present solution

Juergen

[spring-projects-issues]

Rob Harrop commented

The pattern (called double checked locking) is only guaranteed to work under extremely narrow conditions, and then only when running on a 5.0+ JVM. In this scenario it doesn't dramatically reduce the chance of getting double caching (it certainly doesn't remove it), but in my mind it is certainly not as clear.

The code is interesting since it says there is no synchronization when in fact there is.

A better solution to improved performance here is either a concurrent data structure like ConcurrentHashMap or to use a lock stripe to enable concurrent accesses to the cache map for different views.

Rob

[spring-projects-issues]

Lari Hotari commented

Since Spring 3.x is Java5+, could AbstractCachingViewResolver now use ConcurrentHashMap to improve performance?

[spring-projects-issues]

Rossen Stoyanchev commented

This issue has been resolved through a selective bulk update, as part of a larger effort to better manage unresolved issues. To qualify for the update, the issue was either created before Spring 3.0 or affects a version older than Spring 3.0 and is not a bug.

There is a good chance the request was made obsolete, or at least partly outdated, by changes in later versions of Spring including deprecations. It is also possible it didn't get enough traction or we didn't have enough time to address it. One way or another, we didn't get to it.

If you believe the issue, or some aspects of it, are still relevant and worth pursuing at present you may re-open this issue or create a new one with a more up-to-date description.

We thank you for your contributions and encourage you to become familiar with the current process of managing Spring Framework JIRA issues that has been in use for over a year.

[spring-projects-issues]

Donnchadh O Donnabhain commented

I think using ConcurrentHashMap would be nicer than the current situation where we have a big global lock (for a given ViewResolver).
It would be at the expense of potentially loading a view multiple times, but that seems better than blocking all other requests while loading a view.

In general I suppose this isn't a major problem since view are loaded relatively quickly but we've had an instance where one FreeMarkerView blocked all other requests as it was loading a Template.

[spring-projects-issues]

Donnchadh O Donnabhain commented

Since a LinkedHashMap is now used a striped lock might be the nicest approach. (I was looking at 3.1.3 when I added the above comment).
For now we have extended FreeMarkerViewResolver and overridden AbstractCachingViewResolver.resolveViewName with a version that uses a striped lock.

[spring-projects-issues]

Juergen Hoeller commented

I currently favor a combination of a ConcurrentHashMap and our existing LinkedHashMap, with only the latter one being synchronized. LinkedHashMap's removeEldestEntry simply removes the entry from the ConcurrentHashMap as well in case of the cache limit being reached. And for access to already cached instances, we perform a check on the ConcurrentHashMap first and fall back to the synchronized block around the LinkedHashMap only if not found initially. Of course, within that synchronized block, we need to perform yet another check whether the instance exists already; however, that'll only apply to non-existing instances anyway which is not the case we're optimizing for.

This seems like a good enough and quite maintainable compromise between fast access to existing instances, synchronized creation of View instances (with a guarantee to not create an instance twice unless evicted from the cache before), and enforcing the cache limit (as per #14698). There might still be lock contention when the cache is empty initially and several View instances need to be created - but it'll go away very quickly as the cache becomes populated with the most important instances. A few View instances created later on won't prevent fast access to already existing instances in the cache. And even with all Views created, there is no global lock for accessing them anymore.

Note that this is quite similar to what our DefaultListableBeanFactory does for access to singleton beans, except that we don't need an extra LinkedHashMap since there is no cache limit to enforce. We simply use a ConcurrentHashMap there that we first check for an existing bean instance, then synchronize on, check again, and create the instance.

The alternative would be manual lock striping, as you suggested. However, we'd still need to do some sort of synchronization around the LinkedHashMap then since that Map implementation isn't guaranteed to remain internally consistent when concurrently populated without global synchronization. So I suppose we'd have to use lock striping in combination with a different Map implementation that supports enforcing a cache limit as well, removing the eldest entry? I'm sure that's possible but given the nature of a 3.2.x release, the above combination of a ConcurrentHashMap with a synchronized LinkedHashMap seems like the lower-risk option while nevertheless providing 80% of the benefit.

Juergen

[spring-projects-issues]

Juergen Hoeller commented

Implemented the approach as outlined above for 3.2.2 now.

Juergen

[spring-projects-issues]

Donnchadh O Donnabhain commented

That looks good. Lock striping would probably not be worth the extra complexity.