8043003: Use strongly referenced generic invokers

Reviewed-by: lagergren, sundar

nashorn/src/jdk/nashorn/internal/runtime/ScriptFunction.java

@@ -542,7 +542,7 @@ public abstract class ScriptFunction extends ScriptObject {
         } //else just fall through and link as ordinary function or unstable apply
 
         final int programPoint = NashornCallSiteDescriptor.isOptimistic(desc) ? NashornCallSiteDescriptor.getProgramPoint(desc) : INVALID_PROGRAM_POINT;
-        final CompiledFunction cf = data.getBestInvoker(type, programPoint, scope);
+        final CompiledFunction cf = data.getBestInvoker(type, scope);
         final GuardedInvocation bestInvoker =
                 new GuardedInvocation(
                         cf.createInvoker(type.returnType(), programPoint),

nashorn/src/jdk/nashorn/internal/runtime/ScriptFunctionData.java

@@ -32,12 +32,6 @@ import static jdk.nashorn.internal.runtime.ScriptRuntime.UNDEFINED;
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodHandles;
 import java.lang.invoke.MethodType;
-import java.lang.ref.Reference;
-import java.lang.ref.WeakReference;
-import java.util.Collections;
-import java.util.Map;
-import java.util.WeakHashMap;
-
 import jdk.nashorn.internal.runtime.linker.LinkerCallSite;
 
 /**
@@ -53,9 +47,6 @@ public abstract class ScriptFunctionData {
         assert MAX_ARITY < 256;
     }
 
-    private static final Map<ScriptFunctionData, Reference<MethodHandle>> GENERIC_INVOKERS     = Collections.synchronizedMap(new WeakHashMap<ScriptFunctionData, Reference<MethodHandle>>());
-    private static final Map<ScriptFunctionData, Reference<MethodHandle>> GENERIC_CONSTRUCTORS = Collections.synchronizedMap(new WeakHashMap<ScriptFunctionData, Reference<MethodHandle>>());
-
     /** Name of the function or "" for anonymous functions */
     protected final String name;
 
@@ -71,6 +62,13 @@ public abstract class ScriptFunctionData {
     // value, the function might still be capable of receiving variable number of arguments, see isVariableArity.
     private int arity;
 
+    /**
+     * A pair of method handles used for generic invoker and constructor. Field is volatile as it can be initialized by
+     * multiple threads concurrently, but we still tolerate a race condition in it as all values stored into it are
+     * idempotent.
+     */
+    private volatile GenericInvokers genericInvokers;
+
     private static final MethodHandle BIND_VAR_ARGS = findOwnMH("bindVarArgs", Object[].class, Object[].class, Object[].class);
 
     /** Is this a strict mode function? */
@@ -221,7 +219,7 @@ public abstract class ScriptFunctionData {
      * @return guarded invocation with method handle to best invoker and potentially a switch point guarding optimistic
      * assumptions.
      */
-     final CompiledFunction getBestInvoker(final MethodType callSiteType, final int callerProgramPoint, final ScriptObject runtimeScope) {
+     final CompiledFunction getBestInvoker(final MethodType callSiteType, final ScriptObject runtimeScope) {
         final CompiledFunction cf = getBest(callSiteType, runtimeScope);
         assert cf != null;
         return cf;
@@ -255,16 +253,14 @@ public abstract class ScriptFunctionData {
      * @return generic invoker of this script function
      */
     final MethodHandle getGenericInvoker(final ScriptObject runtimeScope) {
-        MethodHandle invoker;
+        // This method has race conditions both on genericsInvoker and genericsInvoker.invoker, but even if invoked
-        final Reference<MethodHandle> ref = GENERIC_INVOKERS.get(this);
+        // concurrently, they'll create idempotent results, so it doesn't matter. We could alternatively implement this
-        if(ref != null) {
+        // using java.util.concurrent.AtomicReferenceFieldUpdater, but it's hardly worth it.
-            invoker = ref.get();
+        final GenericInvokers lgenericInvokers = ensureGenericInvokers();
-            if(invoker != null) {
+        MethodHandle invoker = lgenericInvokers.invoker;
-                return invoker;
+        if(invoker == null) {
-            }
+            lgenericInvokers.invoker = invoker = createGenericInvoker(runtimeScope);
         }
-        invoker = createGenericInvoker(runtimeScope);
-        GENERIC_INVOKERS.put(this, new WeakReference<>(invoker));
         return invoker;
     }
 
@@ -273,16 +269,14 @@ public abstract class ScriptFunctionData {
     }
 
     final MethodHandle getGenericConstructor(final ScriptObject runtimeScope) {
-        MethodHandle constructor;
+        // This method has race conditions both on genericsInvoker and genericsInvoker.constructor, but even if invoked
-        final Reference<MethodHandle> ref = GENERIC_CONSTRUCTORS.get(this);
+        // concurrently, they'll create idempotent results, so it doesn't matter. We could alternatively implement this
-        if(ref != null) {
+        // using java.util.concurrent.AtomicReferenceFieldUpdater, but it's hardly worth it.
-            constructor = ref.get();
+        final GenericInvokers lgenericInvokers = ensureGenericInvokers();
-            if(constructor != null) {
+        MethodHandle constructor = lgenericInvokers.constructor;
-                return constructor;
+        if(constructor == null) {
-            }
+            lgenericInvokers.constructor = constructor = createGenericConstructor(runtimeScope);
         }
-        constructor = createGenericConstructor(runtimeScope);
-        GENERIC_CONSTRUCTORS.put(this, new WeakReference<>(constructor));
         return constructor;
     }
 
@@ -290,6 +284,14 @@ public abstract class ScriptFunctionData {
         return makeGenericMethod(getGeneric(runtimeScope).createComposableConstructor());
     }
 
+    private GenericInvokers ensureGenericInvokers() {
+        GenericInvokers lgenericInvokers = genericInvokers;
+        if(lgenericInvokers == null) {
+            genericInvokers = lgenericInvokers = new GenericInvokers();
+        }
+        return lgenericInvokers;
+    }
+
     /**
      * Returns the best function for the specified call site type.
      * @param callSiteType The call site type. Call site types are expected to have the form
@@ -781,4 +783,14 @@ public abstract class ScriptFunctionData {
     private static MethodHandle findOwnMH(final String name, final Class<?> rtype, final Class<?>... types) {
         return MH.findStatic(MethodHandles.lookup(), ScriptFunctionData.class, name, MH.type(rtype, types));
     }
+
+    /**
+     * This class is used to hold the generic invoker and generic constructor pair. It is structured in this way since
+     * most functions will never use them, so this way ScriptFunctionData only pays storage cost for one null reference
+     * to the GenericInvokers object, instead of two null references for the two method handles.
+     */
+    private static final class GenericInvokers {
+        volatile MethodHandle invoker;
+        volatile MethodHandle constructor;
+    }
 }