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 <[email protected]>
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 <[email protected]>
> 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.
>>>>
>>>>
>>>
>>
>>
>
