Please review JDK-8059321

[Marcus Lagergren]

Thanks, Attila. I will address this and remeasure startup performance.

/M

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

> For values in VALID_CACHE in regex factory, I'd just use an Object instead of String for VALID_REGEXP: declare WeakHashMap<String, Object> and use "private static final Object VALID_REGEXP = new Object()". 
> 
> That's the lesser problem. A larger problem is that I think - since this is static - it's possible for the map to be accessed on multiple threads, and WeakHashMap is not threadsafe, so you'll need to synchronize around it, which may or may not take away the performance benefits. You can't avoid it, as concurrent puts can corrupt the map.
> 
> Yet another issue is that the keys are Strings - weak reachability is a referential property, and strings have value semantics. If the exact String object being put in the map gets GCd (it is not itself interned etc.) then entries can seemingly mysteriously disappear. This won't cause any trouble correctness wise, of course, but it could be surprising. (E.g. you might still end up validating the same string.)
> 
> Likewise, you need to synchronize around INTERNAL_TYPE_CACHE in Type too. The other issues I mentioned with VALID_CACHE fortunately don't apply here: I don't think synchronization will affect the performance benefit much, as it pays off better, because you're doing one lookup per type and then cache the result. Also, Class objects have identity equality semantics, so they won't unexpectedly vanish from the map either.
> 
> So, at the least, you need to:
> - add one synchronization block around the if(get) { ... put } operation sequence for both weak hash maps
> 
> before I can +1 this.
> 
> I'd also like it if you used:
> - a "new Object()" instead of "valid".intern() for VALID_CACHE values
> - containsKey(key) instead of get(key) == null with VALID_CACHE.
> (these are just niceties)
> 
> Attila.
> 
> 
> On Sep 29, 2014, at 4:36 PM, Marcus Lagergren <marcus.lagergren at oracle.com> wrote:
> 
>> Can I get reviews here, please. Attila? Other people who may be at JavaOne? Jim?
>> 
>> /M
>> 
>> On 28 Sep 2014, at 18:30, Marcus Lagergren <marcus.lagergren at oracle.com> wrote:
>> 
>>> Some very trivial changes / caches to parser logic, regexp engine and type descriptor calculation that had mesurable and significant impact on avatar startup.
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8059321
>>> Webrev is here: http://cr.openjdk.java.net/~lagergren/8059321/
>>> 
>>> /M
>>> 
>> 
>