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
