Concurrent access to ParsedSql cache in NamedParameterJdbcTemplate

[benelog]

NamedParameterJdbcTemplate.getParsedSql(Sting) can be a bottleneck under high load as it holds a global lock by this.parsedSqlCache.

		synchronized (this.parsedSqlCache) {
			ParsedSql parsedSql = this.parsedSqlCache.get(sql);
			if (parsedSql == null) {
				parsedSql = NamedParameterUtils.parseSqlStatement(sql);
				this.parsedSqlCache.put(sql, parsedSql);
			}
			return parsedSql;
		}

This PR tried to improve the performance by the combination of a ConcurrentHashMap and a LinkedHashMap.

I referenced 06c6cbb6b92 and
#7831 (comment)

[rengy-github]

Is it necessary to cache twice
why need to use synchronized when using concurrenthashmap
Can just modify synchronized

		ParsedSql parsedSql = this.parsedSqlCache.get(sql);
		if (parsedSql == null) {
                            synchronized (this.parsedSqlCache) {
			    parsedSql = NamedParameterUtils.parseSqlStatement(sql);
			    this.parsedSqlCache.put(sql, parsedSql);
                           }
		}
		return parsedSql;

[benelog]

@rengy-github
LinkedHashMap.get() is not thread-safe when LinkedHashMap.put() is called from other threads, even if LinkedHashMap.put() is called from synchronized blocks.

[rengy-github]

@benelog
get doesn't need synchronization
ParsedSql parsedSql = this.parsedSqlCache.get(sql);
if (parsedSql == null) {
synchronized (this.parsedSqlCache) {
parsedSql = this.parsedSqlCache.get(sql);
if(parsedSql==null) {
parsedSql = NamedParameterUtils.parseSqlStatement(sql);
this.parsedSqlCache.put(sql, parsedSql);
}
}
}

[benelog]

@rengy-github
Reading the following issue will help you understand my PR.

#7831

[rengy-github]

I think don't use sync or just put
because There is no problem that SQL is parsed twice occasionally

[rengy-github]

My puzzle is whether there is problem with the other thread get when one thread puts

Could you send me a link about this, thank you

[benelog]

@rengy-github
The Javadoc of LinkedHashMap will be helpful for you.

Note that this implementation is not synchronized. If multiple threads access a linked hash map concurrently, and at least one of the threads modifies the map structurally, it must be synchronized externally.
....
A structural modification is any operation that adds or deletes one or more mappings or, in the case of access-ordered linked hash maps, affects iteration order. In insertion-ordered linked hash maps, merely changing the value associated with a key that is already contained in the map is not a structural modification. In access-ordered linked hash maps, merely querying the map with get is a structural modification. )

For the above reason, Collections.synchronizedMap(Map<K,V> m) returns a wrapper to call Map.get(Object key) from a synchronized block.

https://github.com/AdoptOpenJDK/openjdk-jdk/blob/master/src/java.base/share/classes/java/util/Collections.java#L2634

        public V get(Object key) {
            synchronized (mutex) {return m.get(key);}
        }

Calling LinkedHashMap.get() from outside of synchronized blocks can cause memory leaks, the problem actually happened in our company.
The following article explains the incident.

https://d2.naver.com/helloworld/1326256
(Unfortunately, it was written in Korean language)

The following artice is about similar issues in HashMap.

https://mailinator.blogspot.com/2009/06/beautiful-race-condition.html

[rengy-github]

@benelog
thank you very much，Now I understand why use a ConcurrentHashMap and a LinkedHashMap

[jhoeller]

I've addressed this using a revised version our own ConcurrentLruCache for 5.3 RC1 now, extracted from MimeTypeUtils (where it got introduced in 5.2), also available for reuse in other classes. To be committed ASAP.

[andrei-ivanov]

now #22789 got fixed too 👍