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

[Aleksey Shipilev]

On 07/22/2012 03:45 AM, John Rose wrote:
> On Jul 18, 2012, at 1:43 AM, Aleksey Shipilev wrote:
> Yes.

Thanks John. I'm having a glance over the fix in new webrev, and this
feels even worse:

+   private static SpeciesData get(String types) {
+            // Acquire cache lock for query.
+            SpeciesData d = lookupCache(types);
+            if (!d.isPlaceholder())
+                return d;
+            Class<? extends BoundMethodHandle> cbmh;
+            synchronized (d) {
+                // Use synch. on the placeholder to prevent multiple
instantiation of one species.
+                // Creating this class forces a recursive call to
getForClass.
+                cbmh = Factory.generateConcreteBMHClass(types);
+            }
+            // Reacquire cache lock.
+            d = lookupCache(types);
+            // Class loading must have upgraded the cache.
+            assert(d != null && !d.isPlaceholder());
+            return d;
+        }

+   static SpeciesData getForClass(String types, Class<? extends
BoundMethodHandle> clazz) {
+            // clazz is a new class which is initializing its
SPECIES_DATA field
+            return updateCache(types, new SpeciesData(types, clazz));
+        }

+   private static synchronized SpeciesData lookupCache(String types) {
+            SpeciesData d = CACHE.get(types);
+            if (d != null)  return d;
+            d = new SpeciesData(types);
+            assert(d.isPlaceholder());
+            CACHE.put(types, d);
+            return d;
+        }

+   private static synchronized SpeciesData updateCache(String types,
SpeciesData d) {
+            SpeciesData d2;
+            assert((d2 = CACHE.get(types)) == null || d2.isPlaceholder());
+            assert(!d.isPlaceholder());
+            CACHE.put(types, d);
+            return d;
+        }

Global synchronization is the performance smell, and this looks to be
potential scalability bottleneck (it sends shivers down my spine every
time I see "static synchronized" in the same line. That is not to
mention synchronizing on placeholder looks suspicious and error-prone.

Do you need help rewriting this with CHM?

-Aleksey.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
Url : http://mail.openjdk.java.net/pipermail/mlvm-dev/attachments/20120723/fad99c5a/signature.asc