Thanks, Attila. I will address this and remeasure startup performance. /M
On 29 Sep 2014, at 10:45, Attila Szegedi <[email protected]> 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 <[email protected]> > 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 <[email protected]> >> 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 >>> >> >
