review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code

[John Rose]

On Jul 18, 2012, at 1:43 AM, Aleksey Shipilev wrote:

> This feels outright wrong:
> 
> +        static Map<String, Data> CACHE = new IdentityHashMap<>();
> I couple of questions pops out in my mind:
> 1. Is this code supposed to be thread-safe? It is then wrong to use
> unguarded IdentityHashMap without external synchronization.
> 2. Do we really need a second .get() check, if code is not thread-safe
> anyway? This code allows two threads to call put() on the same key anyway.
> 3. Do the $types *need* to be interned? Or is this the premature
> optimization (gone wrong, btw)?

You are right on all points.  Fixed.

> I would rather like to see CHM.putIfAbsent()-based cache on plain
> non-interned strings there. Even if interned strings are required, we
> could intern them on successful map update, not every time we look up
> the key.

Yes.

Using a IdentityHashMap here is an anti-pattern; sorry to propagate the bad example.

Thanks,
— John