Done Thanks.
Another review please. On 29 Sep 2014, at 11:55, Attila Szegedi <[email protected]> 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 <[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. >>>>> >>>>> >>>> >>> >>> >> >
