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

[Aleksey Shipilev]

(sorry for not replying to original message, just subscribed)

This feels outright wrong:

+        static Map<String, Data> CACHE = new IdentityHashMap<>();
+
+        static Data get(String types) {
+            final String key = types.intern();
+            Data d = CACHE.get(key);
+            if (d == null) {
+                d = make(types);
+                Data e = CACHE.get(key);
+                if (e != null) {
+                    d = e;
+                } else {
+                    CACHE.put(key, d);
+                }
+            }
+            return d;
+        }

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

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.

-Aleksey.