Spring Concurrent access to ParsedSql cache in NamedParameterJdbcTemplate

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 https://github.com/spring-projects/spring-framework/commit/06c6cbb6b92 and https://github.com/spring-projects/spring-framework/issues/7831#issuecomment-453315164

Comment From: 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;

Comment From: 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.

Comment From: 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); } } }

Comment From: benelog

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

https://github.com/spring-projects/spring-framework/issues/7831

Comment From: rengy-github

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

Comment From: 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

Comment From: 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

Comment From: rengy-github

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

Comment From: 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.

Comment From: andrei-ivanov

now #22789 got fixed too 👍