RFR JDK-8187653: Lock in CoderResult.Cache becomes performance bottleneck

[Roger Riggs]

Hi Sherman,

Looks to be a good fix with predictable behavior and performance.

+1, Roger

On 2/23/2018 8:26 PM, Xueming Shen wrote:
> Hi,
>
> Please help review the proposed change to remove the potential 
> performance
> bottleneck in CoderResult caused by the "over" synchronization the cache
> mechanism.
>
> issue: https://bugs.openjdk.java.net/browse/JDK-8187653
> webrev: http://cr.openjdk.java.net/~sherman/8187653/webrev
>
> Notes:
> While the original test case (new String/String.getBytes() with UTF8 
> as showed in the
> stacktrace) described in the bug report might no longer be 
> reproducible in jdk9, as
> we have been optimizing lots of String related  char[]/byte[] coding 
> path away from
> the paths that use CoderResult. But it is still a concern/issue for 
> the "general"
> CharsetDe/Encoder.de/encode() operation, in which the malformed or 
> unmappable
> CoderResult object is returned.
>
> As showed in the "[synchronized]" section in bm.scores [1], which is 
> from the simple
> jmh benchmark test CoderResultBM.java [2], the sores are getting 
> significant worse
> when the number of concurrent de/encoding threads gets bigger.
>
> It appears the easy fix is to replace the sync mechanism from "method 
> synchronized
> + HashMap" to  "ConcurrentHashMap" solves the problem, as showed in 
> the same bm
> result [1] in [ConcurrentHashMap] section, because most of the 
> accesses to the caching
> HashMap is read operation. The ConcurrentHahsMap's "almost non-block 
> for retrieval
> operation" really helps here.
>
> There is another fact that might help us optimize further here. For 
> most of our charsets
> in JDK repository (with couple exceptions), the length of malformed 
> and unmappable
> CoderResult that these charsets might trigger actually is never longer 
> than 4. So we might
> not need to access the ConcurrentHashMap cache at all in normal use 
> scenario. I'm putting
> a CoderResult[4] on top the hashmap cache in proposed webrev. It does 
> not improve the
> performance significantly, but when the thread number gets bigger, a 
> 10%+ improvement
> is observed. So I would assume it might be something worth doing, 
> given its cost is really
> low.
>
> Thanks,
> Sherman
>
>
> [1] http://cr.openjdk.java.net/~sherman/8187653/bm.scores
> [2] http://cr.openjdk.java.net/~sherman/8187653/CoderResultBM.java
> [3] http://cr.openjdk.java.net/~sherman/8187653/bm.scores.ms932
>