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

[David Holmes]

On 2/03/2018 12:09 PM, Xueming Shen wrote:
> On 3/1/18, 4:35 PM, David Holmes wrote:
> 
>> When you replace synchronized code with concurrent data structures you 
>> introduce race conditions that are precluded in the synchronized code. 
>> These need to be examined carefully to ensure they are safe. For 
>> example, whenever you replace a HashMap with a ConcurrentHashMap you 
>> need to see if put() needs to be replaced by putIfAbsent().
>>
> 
> Hi David,
> 
> The assumption here is that putIfAbsent() does not help/save anything as 
> the value object
> would have been created already when it reaches here. And it appears 
> there is no need here
> to have the check&put to be atomic, replacing any existing key/value 
> pair is fine in this use

As long as create(len) is idempotent that should be fine.

As Claes pointed out offline you're also risking creating more than one 
CHM instance. Again if create(len) is idempotent this should be harmless 
but it is somewhat messy.

Also this:

195         private Map<Integer,WeakReference<CoderResult>> cache = null;

should now be volatile.

Cheers,
David

> scenario.  Was thinking about computIfAbsent(), but concluded it's just 
> little overdone (wonder
> why we decided to use the WeakReference in this case, it probably is not 
> worth it, but keep it as
> is for "compatibility" concern).
> 
> -Sherman