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
>>
>
