8062308: Incorrect constant linkage with multiple Globals in a Context
Reviewed-by: lagergren, sundar
--- a/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/objects/Global.java Thu Nov 06 13:17:47 2014 +0100
+++ b/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/objects/Global.java Thu Nov 06 17:06:56 2014 +0100
@@ -29,6 +29,7 @@
import static jdk.nashorn.internal.runtime.ECMAErrors.referenceError;
import static jdk.nashorn.internal.runtime.ECMAErrors.typeError;
import static jdk.nashorn.internal.runtime.ScriptRuntime.UNDEFINED;
+
import java.io.IOException;
import java.io.PrintWriter;
import java.lang.invoke.MethodHandle;
@@ -41,7 +42,6 @@
import java.util.Map;
import java.util.concurrent.Callable;
import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.atomic.AtomicReference;
import javax.script.ScriptContext;
import javax.script.ScriptEngine;
import jdk.internal.dynalink.linker.GuardedInvocation;
@@ -54,7 +54,6 @@
import jdk.nashorn.internal.objects.annotations.ScriptClass;
import jdk.nashorn.internal.runtime.ConsString;
import jdk.nashorn.internal.runtime.Context;
-import jdk.nashorn.internal.runtime.GlobalConstants;
import jdk.nashorn.internal.runtime.GlobalFunctions;
import jdk.nashorn.internal.runtime.JSType;
import jdk.nashorn.internal.runtime.NativeJavaPackage;
@@ -438,9 +437,6 @@
this.scontext = scontext;
}
- // global constants for this global - they can be replaced with MethodHandle.constant until invalidated
- private static AtomicReference<GlobalConstants> gcsInstance = new AtomicReference<>();
-
@Override
protected Context getContext() {
return context;
@@ -470,11 +466,6 @@
super(checkAndGetMap(context));
this.context = context;
this.setIsScope();
- //we can only share one instance of Global constants between globals, or we consume way too much
- //memory - this is good enough for most programs
- while (gcsInstance.get() == null) {
- gcsInstance.compareAndSet(null, new GlobalConstants(context.getLogger(GlobalConstants.class)));
- }
}
/**
@@ -493,15 +484,6 @@
}
/**
- * Return the global constants map for fields that
- * can be accessed as MethodHandle.constant
- * @return constant map
- */
- public static GlobalConstants getConstants() {
- return gcsInstance.get();
- }
-
- /**
* Check if we have a Global instance
* @return true if one exists
*/
--- a/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/runtime/Context.java Thu Nov 06 13:17:47 2014 +0100
+++ b/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/runtime/Context.java Thu Nov 06 17:06:56 2014 +0100
@@ -33,6 +33,7 @@
import static jdk.nashorn.internal.runtime.ECMAErrors.typeError;
import static jdk.nashorn.internal.runtime.ScriptRuntime.UNDEFINED;
import static jdk.nashorn.internal.runtime.Source.sourceFor;
+
import java.io.File;
import java.io.IOException;
import java.io.PrintWriter;
@@ -60,6 +61,7 @@
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.atomic.AtomicReference;
import java.util.function.Consumer;
import java.util.function.Supplier;
import java.util.logging.Level;
@@ -262,6 +264,10 @@
// persistent code store
private CodeStore codeStore;
+ // A factory for linking global properties as constant method handles. It is created when the first Global
+ // is created, and invalidated forever once the second global is created.
+ private final AtomicReference<GlobalConstants> globalConstantsRef = new AtomicReference<>();
+
/**
* Get the current global scope
* @return the current global scope
@@ -293,7 +299,10 @@
assert getGlobal() != global;
//same code can be cached between globals, then we need to invalidate method handle constants
if (global != null) {
- Global.getConstants().invalidateAll();
+ final GlobalConstants globalConstants = getContext(global).getGlobalConstants();
+ if (globalConstants != null) {
+ globalConstants.invalidateAll();
+ }
}
currentGlobal.set(global);
}
@@ -529,6 +538,15 @@
}
/**
+ * Returns the factory for constant method handles for global properties. The returned factory can be
+ * invalidated if this Context has more than one Global.
+ * @return the factory for constant method handles for global properties.
+ */
+ GlobalConstants getGlobalConstants() {
+ return globalConstantsRef.get();
+ }
+
+ /**
* Get the error manager for this context
* @return error manger
*/
@@ -1016,9 +1034,32 @@
* @return the global script object
*/
public Global newGlobal() {
+ createOrInvalidateGlobalConstants();
return new Global(this);
}
+ private void createOrInvalidateGlobalConstants() {
+ for (;;) {
+ final GlobalConstants currentGlobalConstants = getGlobalConstants();
+ if (currentGlobalConstants != null) {
+ // Subsequent invocation; we're creating our second or later Global. GlobalConstants is not safe to use
+ // with more than one Global, as the constant method handle linkages it creates create a coupling
+ // between the Global and the call sites in the compiled code.
+ currentGlobalConstants.invalidateForever();
+ return;
+ }
+ final GlobalConstants newGlobalConstants = new GlobalConstants(getLogger(GlobalConstants.class));
+ if (globalConstantsRef.compareAndSet(null, newGlobalConstants)) {
+ // First invocation; we're creating the first Global in this Context. Create the GlobalConstants object
+ // for this Context.
+ return;
+ }
+
+ // If we reach here, then we started out as the first invocation, but another concurrent invocation won the
+ // CAS race. We'll just let the loop repeat and invalidate the CAS race winner.
+ }
+ }
+
/**
* Initialize given global scope object.
*
@@ -1057,12 +1098,19 @@
* @return current global's context
*/
static Context getContextTrusted() {
- return ((ScriptObject)Context.getGlobal()).getContext();
+ return getContext(getGlobal());
}
static Context getContextTrustedOrNull() {
final Global global = Context.getGlobal();
- return global == null ? null : ((ScriptObject)global).getContext();
+ return global == null ? null : getContext(global);
+ }
+
+ private static Context getContext(final Global global) {
+ // We can't invoke Global.getContext() directly, as it's a protected override, and Global isn't in our package.
+ // In order to access the method, we must cast it to ScriptObject first (which is in our package) and then let
+ // virtual invocation do its thing.
+ return ((ScriptObject)global).getContext();
}
/**
--- a/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/runtime/GlobalConstants.java Thu Nov 06 13:17:47 2014 +0100
+++ b/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/runtime/GlobalConstants.java Thu Nov 06 17:06:56 2014 +0100
@@ -31,12 +31,14 @@
import static jdk.nashorn.internal.runtime.UnwarrantedOptimismException.INVALID_PROGRAM_POINT;
import static jdk.nashorn.internal.runtime.linker.NashornCallSiteDescriptor.getProgramPoint;
import static jdk.nashorn.internal.runtime.logging.DebugLogger.quote;
+
import java.lang.invoke.MethodHandle;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.SwitchPoint;
import java.util.Arrays;
import java.util.HashMap;
import java.util.Map;
+import java.util.concurrent.atomic.AtomicBoolean;
import java.util.logging.Level;
import jdk.internal.dynalink.CallSiteDescriptor;
import jdk.internal.dynalink.DynamicLinker;
@@ -50,7 +52,7 @@
import jdk.nashorn.internal.runtime.logging.Logger;
/**
- * Each global owns one of these. This is basically table of accessors
+ * Each context owns one of these. This is basically table of accessors
* for global properties. A global constant is evaluated to a MethodHandle.constant
* for faster access and to avoid walking to proto chain looking for it.
*
@@ -67,12 +69,19 @@
* reregister the switchpoint. Set twice or more - don't try again forever, or we'd
* just end up relinking our way into megamorphisism.
*
+ * Also it has to be noted that this kind of linking creates a coupling between a Global
+ * and the call sites in compiled code belonging to the Context. For this reason, the
+ * linkage becomes incorrect as soon as the Context has more than one Global. The
+ * {@link #invalidateForever()} is invoked by the Context to invalidate all linkages and
+ * turn off the functionality of this object as soon as the Context's {@link Context#newGlobal()} is invoked
+ * for second time.
+ *
* We can extend this to ScriptObjects in general (GLOBAL_ONLY=false), which requires
* a receiver guard on the constant getter, but it currently leaks memory and its benefits
* have not yet been investigated property.
*
- * As long as all Globals share the same constant instance, we need synchronization
- * whenever we access the instance.
+ * As long as all Globals in a Context share the same GlobalConstants instance, we need synchronization
+ * whenever we access it.
*/
@Logger(name="const")
public final class GlobalConstants implements Loggable {
@@ -82,7 +91,7 @@
* Script objects require a receiver guard, which is memory intensive, so this is currently
* disabled. We might implement a weak reference based approach to this later.
*/
- private static final boolean GLOBAL_ONLY = true;
+ public static final boolean GLOBAL_ONLY = true;
private static final MethodHandles.Lookup LOOKUP = MethodHandles.lookup();
@@ -98,6 +107,8 @@
*/
private final Map<String, Access> map = new HashMap<>();
+ private final AtomicBoolean invalidatedForever = new AtomicBoolean(false);
+
/**
* Constructor - used only by global
* @param log logger, or null if none
@@ -216,10 +227,34 @@
* the same class for a new global, but the builtins and global scoped variables
* will have changed.
*/
- public synchronized void invalidateAll() {
- log.info("New global created - invalidating all constant callsites without increasing invocation count.");
- for (final Access acc : map.values()) {
- acc.invalidateUncounted();
+ public void invalidateAll() {
+ if (!invalidatedForever.get()) {
+ log.info("New global created - invalidating all constant callsites without increasing invocation count.");
+ synchronized (this) {
+ for (final Access acc : map.values()) {
+ acc.invalidateUncounted();
+ }
+ }
+ }
+ }
+
+ /**
+ * To avoid an expensive global guard "is this the same global", similar to the
+ * receiver guard on the ScriptObject level, we invalidate all getters when the
+ * second Global is created by the Context owning this instance. After this
+ * method is invoked, this GlobalConstants instance will both invalidate all the
+ * switch points it produced, and it will stop handing out new method handles
+ * altogether.
+ */
+ public void invalidateForever() {
+ if (invalidatedForever.compareAndSet(false, true)) {
+ log.info("New global created - invalidating all constant callsites.");
+ synchronized (this) {
+ for (final Access acc : map.values()) {
+ acc.invalidateForever();
+ }
+ map.clear();
+ }
}
}
@@ -251,7 +286,7 @@
return obj;
}
- private synchronized Access getOrCreateSwitchPoint(final String name) {
+ private Access getOrCreateSwitchPoint(final String name) {
Access acc = map.get(name);
if (acc != null) {
return acc;
@@ -267,9 +302,13 @@
* @param name name of property
*/
void delete(final String name) {
- final Access acc = map.get(name);
- if (acc != null) {
- acc.invalidateForever();
+ if (!invalidatedForever.get()) {
+ synchronized (this) {
+ final Access acc = map.get(name);
+ if (acc != null) {
+ acc.invalidateForever();
+ }
+ }
}
}
@@ -313,45 +352,45 @@
*
* @return null if failed to set up constant linkage
*/
- synchronized GuardedInvocation findSetMethod(final FindProperty find, final ScriptObject receiver, final GuardedInvocation inv, final CallSiteDescriptor desc, final LinkRequest request) {
- if (GLOBAL_ONLY && !isGlobalSetter(receiver, find)) {
+ GuardedInvocation findSetMethod(final FindProperty find, final ScriptObject receiver, final GuardedInvocation inv, final CallSiteDescriptor desc, final LinkRequest request) {
+ if (invalidatedForever.get() || (GLOBAL_ONLY && !isGlobalSetter(receiver, find))) {
return null;
}
final String name = desc.getNameToken(CallSiteDescriptor.NAME_OPERAND);
- final Access acc = getOrCreateSwitchPoint(name);
+ synchronized (this) {
+ final Access acc = getOrCreateSwitchPoint(name);
- if (log.isEnabled()) {
- log.fine("Trying to link constant SETTER ", acc);
- }
+ if (log.isEnabled()) {
+ log.fine("Trying to link constant SETTER ", acc);
+ }
- if (!acc.mayRetry()) {
- if (log.isEnabled()) {
- log.fine("*** SET: Giving up on " + quote(name) + " - retry count has exceeded " + DynamicLinker.getLinkedCallSiteLocation());
+ if (!acc.mayRetry() || invalidatedForever.get()) {
+ if (log.isEnabled()) {
+ log.fine("*** SET: Giving up on " + quote(name) + " - retry count has exceeded " + DynamicLinker.getLinkedCallSiteLocation());
+ }
+ return null;
}
- return null;
- }
-
- assert acc.mayRetry();
- if (acc.hasBeenInvalidated()) {
- log.info("New chance for " + acc);
- acc.newSwitchPoint();
- }
+ if (acc.hasBeenInvalidated()) {
+ log.info("New chance for " + acc);
+ acc.newSwitchPoint();
+ }
- assert !acc.hasBeenInvalidated();
+ assert !acc.hasBeenInvalidated();
- // if we haven't given up on this symbol, add a switchpoint invalidation filter to the receiver parameter
- final MethodHandle target = inv.getInvocation();
- final Class<?> receiverType = target.type().parameterType(0);
- final MethodHandle boundInvalidator = MH.bindTo(INVALIDATE_SP, this);
- final MethodHandle invalidator = MH.asType(boundInvalidator, boundInvalidator.type().changeParameterType(0, receiverType).changeReturnType(receiverType));
- final MethodHandle mh = MH.filterArguments(inv.getInvocation(), 0, MH.insertArguments(invalidator, 1, acc));
+ // if we haven't given up on this symbol, add a switchpoint invalidation filter to the receiver parameter
+ final MethodHandle target = inv.getInvocation();
+ final Class<?> receiverType = target.type().parameterType(0);
+ final MethodHandle boundInvalidator = MH.bindTo(INVALIDATE_SP, this);
+ final MethodHandle invalidator = MH.asType(boundInvalidator, boundInvalidator.type().changeParameterType(0, receiverType).changeReturnType(receiverType));
+ final MethodHandle mh = MH.filterArguments(inv.getInvocation(), 0, MH.insertArguments(invalidator, 1, acc));
- assert inv.getSwitchPoints() == null : Arrays.asList(inv.getSwitchPoints());
- log.info("Linked setter " + quote(name) + " " + acc.getSwitchPoint());
- return new GuardedInvocation(mh, inv.getGuard(), acc.getSwitchPoint(), inv.getException());
+ assert inv.getSwitchPoints() == null : Arrays.asList(inv.getSwitchPoints());
+ log.info("Linked setter " + quote(name) + " " + acc.getSwitchPoint());
+ return new GuardedInvocation(mh, inv.getGuard(), acc.getSwitchPoint(), inv.getException());
+ }
}
/**
@@ -380,11 +419,11 @@
*
* @return resulting getter, or null if failed to create constant
*/
- synchronized GuardedInvocation findGetMethod(final FindProperty find, final ScriptObject receiver, final CallSiteDescriptor desc) {
+ GuardedInvocation findGetMethod(final FindProperty find, final ScriptObject receiver, final CallSiteDescriptor desc) {
// Only use constant getter for fast scope access, because the receiver may change between invocations
// for slow-scope and non-scope callsites.
// Also return null for user accessor properties as they may have side effects.
- if (!NashornCallSiteDescriptor.isFastScope(desc)
+ if (invalidatedForever.get() || !NashornCallSiteDescriptor.isFastScope(desc)
|| (GLOBAL_ONLY && !find.getOwner().isGlobal())
|| find.getProperty() instanceof UserAccessorProperty) {
return null;
@@ -395,51 +434,53 @@
final Class<?> retType = desc.getMethodType().returnType();
final String name = desc.getNameToken(CallSiteDescriptor.NAME_OPERAND);
- final Access acc = getOrCreateSwitchPoint(name);
+ synchronized (this) {
+ final Access acc = getOrCreateSwitchPoint(name);
- log.fine("Starting to look up object value " + name);
- final Object c = find.getObjectValue();
+ log.fine("Starting to look up object value " + name);
+ final Object c = find.getObjectValue();
- if (log.isEnabled()) {
- log.fine("Trying to link constant GETTER " + acc + " value = " + c);
- }
+ if (log.isEnabled()) {
+ log.fine("Trying to link constant GETTER " + acc + " value = " + c);
+ }
- if (acc.hasBeenInvalidated() || acc.guardFailed()) {
- if (log.isEnabled()) {
- log.info("*** GET: Giving up on " + quote(name) + " - retry count has exceeded " + DynamicLinker.getLinkedCallSiteLocation());
+ if (acc.hasBeenInvalidated() || acc.guardFailed() || invalidatedForever.get()) {
+ if (log.isEnabled()) {
+ log.info("*** GET: Giving up on " + quote(name) + " - retry count has exceeded " + DynamicLinker.getLinkedCallSiteLocation());
+ }
+ return null;
}
- return null;
- }
+
+ final MethodHandle cmh = constantGetter(c);
- final MethodHandle cmh = constantGetter(c);
-
- MethodHandle mh;
- MethodHandle guard;
+ MethodHandle mh;
+ MethodHandle guard;
- if (isOptimistic) {
- if (JSType.getAccessorTypeIndex(cmh.type().returnType()) <= JSType.getAccessorTypeIndex(retType)) {
- //widen return type - this is pessimistic, so it will always work
- mh = MH.asType(cmh, cmh.type().changeReturnType(retType));
+ if (isOptimistic) {
+ if (JSType.getAccessorTypeIndex(cmh.type().returnType()) <= JSType.getAccessorTypeIndex(retType)) {
+ //widen return type - this is pessimistic, so it will always work
+ mh = MH.asType(cmh, cmh.type().changeReturnType(retType));
+ } else {
+ //immediately invalidate - we asked for a too wide constant as a narrower one
+ mh = MH.dropArguments(MH.insertArguments(JSType.THROW_UNWARRANTED.methodHandle(), 0, c, programPoint), 0, Object.class);
+ }
} else {
- //immediately invalidate - we asked for a too wide constant as a narrower one
- mh = MH.dropArguments(MH.insertArguments(JSType.THROW_UNWARRANTED.methodHandle(), 0, c, programPoint), 0, Object.class);
+ //pessimistic return type filter
+ mh = Lookup.filterReturnType(cmh, retType);
}
- } else {
- //pessimistic return type filter
- mh = Lookup.filterReturnType(cmh, retType);
- }
- if (find.getOwner().isGlobal()) {
- guard = null;
- } else {
- guard = MH.insertArguments(RECEIVER_GUARD, 0, acc, receiver);
- }
+ if (find.getOwner().isGlobal()) {
+ guard = null;
+ } else {
+ guard = MH.insertArguments(RECEIVER_GUARD, 0, acc, receiver);
+ }
- if (log.isEnabled()) {
- log.info("Linked getter " + quote(name) + " as MethodHandle.constant() -> " + c + " " + acc.getSwitchPoint());
- mh = MethodHandleFactory.addDebugPrintout(log, Level.FINE, mh, "get const " + acc);
+ if (log.isEnabled()) {
+ log.info("Linked getter " + quote(name) + " as MethodHandle.constant() -> " + c + " " + acc.getSwitchPoint());
+ mh = MethodHandleFactory.addDebugPrintout(log, Level.FINE, mh, "get const " + acc);
+ }
+
+ return new GuardedInvocation(mh, guard, acc.getSwitchPoint(), null);
}
-
- return new GuardedInvocation(mh, guard, acc.getSwitchPoint(), null);
}
}
--- a/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/runtime/ScriptObject.java Thu Nov 06 13:17:47 2014 +0100
+++ b/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/runtime/ScriptObject.java Thu Nov 06 17:06:56 2014 +0100
@@ -47,6 +47,7 @@
import static jdk.nashorn.internal.runtime.arrays.ArrayIndex.getArrayIndex;
import static jdk.nashorn.internal.runtime.arrays.ArrayIndex.isValidArrayIndex;
import static jdk.nashorn.internal.runtime.linker.NashornGuards.explicitInstanceOfCheck;
+
import java.lang.invoke.MethodHandle;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
@@ -922,7 +923,10 @@
if (property instanceof UserAccessorProperty) {
((UserAccessorProperty)property).setAccessors(this, getMap(), null);
}
- Global.getConstants().delete(property.getKey());
+ final GlobalConstants globalConstants = getGlobalConstants();
+ if (globalConstants != null) {
+ globalConstants.delete(property.getKey());
+ }
return true;
}
}
@@ -1983,9 +1987,12 @@
}
}
- final GuardedInvocation cinv = Global.getConstants().findGetMethod(find, this, desc);
- if (cinv != null) {
- return cinv;
+ final GlobalConstants globalConstants = getGlobalConstants();
+ if (globalConstants != null) {
+ final GuardedInvocation cinv = globalConstants.findGetMethod(find, this, desc);
+ if (cinv != null) {
+ return cinv;
+ }
}
final Class<?> returnType = desc.getMethodType().returnType();
@@ -2183,14 +2190,22 @@
final GuardedInvocation inv = new SetMethodCreator(this, find, desc, request).createGuardedInvocation(findBuiltinSwitchPoint(name));
- final GuardedInvocation cinv = Global.getConstants().findSetMethod(find, this, inv, desc, request);
- if (cinv != null) {
- return cinv;
+ final GlobalConstants globalConstants = getGlobalConstants();
+ if (globalConstants != null) {
+ final GuardedInvocation cinv = globalConstants.findSetMethod(find, this, inv, desc, request);
+ if (cinv != null) {
+ return cinv;
+ }
}
return inv;
}
+ private GlobalConstants getGlobalConstants() {
+ // Avoid hitting getContext() which might be costly for a non-Global unless needed.
+ return GlobalConstants.GLOBAL_ONLY && !isGlobal() ? null : getContext().getGlobalConstants();
+ }
+
private GuardedInvocation createEmptySetMethod(final CallSiteDescriptor desc, final boolean explicitInstanceOfCheck, final String strictErrorMessage, final boolean canBeFastScope) {
final String name = desc.getNameToken(CallSiteDescriptor.NAME_OPERAND);
if (NashornCallSiteDescriptor.isStrict(desc)) {