Please review JDK-8059321

[Marcus Lagergren]

Done

Thanks.

Another review please.

On 29 Sep 2014, at 11:55, Attila Szegedi <attila.szegedi at oracle.com> wrote:

> Collections.synchronizedMap is okay; I usually go with a single explicit synchronized block encompassing both operations; this way you'll end up with two MONITORENTER/MONITOREXITs, but it shouldn't matter much. Maybe HotSpot does some lock coarsening itself. Again, doesn't matter much - this is fine too.
> 
> lookupInternalType does "cache = INTERNAL_TYPE_CACHE;" and then inconsistently uses both the local variable and the field subsequently. Eliminate one.
> 
> Now that you have a set instead of a map for VALID_CACHE_SET, you could use the nicely atomic
> 
> if (cache.add(key)) {
>   // do stuff 
> }
> 
> idiom instead of
> 
> if (cache.contains(key)) {
>    return;
> }
> cache.add(key);
> // do stuff
> 
> (and then you also don't need a local variable but can just use the field as "if (VALID_CACHE_SET.add(key)) { ... }"
> 
> If you do these, then +1 - no need for a new webrev.
> 
> Attila.
> 
> On Sep 29, 2014, at 8:45 PM, Marcus Lagergren <marcus.lagergren at oracle.com> wrote:
> 
>> OK. New webrev here http://cr.openjdk.java.net/~lagergren/8059321.2/webrev/
>> 
>> I experimented a bit with synchronization methods, and this seems to be the one that gives the least overhead - there is actually very little difference and 95% of the original performance increase is preserved.
>> 
>> (I also experimented with your ‘one extra recompile’ in CompiledFunction, and applied that diff - this brings us down another ~600 ms, which is nice indeed).
>> 
>> Let me know if this is semantically sound. From reading the OpenJDK code, I think it is.
>> 
>> /M
>> 
>> 
>> On 29 Sep 2014, at 11:22, Aleksey Shipilev <aleksey.shipilev at oracle.com> wrote:
>> 
>>> Yes, that's a simple adapter:
>>> http://docs.oracle.com/javase/7/docs/api/java/util/Collections.html#newSetFromMap(java.util.Map)
>>> 
>>> See the example there.
>>> 
>>> -Aleksey.
>>> 
>>> On 09/29/2014 10:10 PM, Marcus Lagergren wrote:
>>>> Aleksey - I still need the weak semantics, because I don’t want to hold on to the strings. Would that work for the WeakHashMap and preserve semantics? #iamnotajavaprogrammer.
>>>> 
>>>>> 
>>>>> The entire shenanigan would go away if you turn the Map into Set with
>>>>> Collections.newSetFromMap(...), and then do add().
>>>>> 
>>>>> -Aleksey.
>>>>> 
>>>>> 
>>>> 
>>> 
>>> 
>> 
>