# HG changeset patch # User attila # Date 1415290016 -3600 # Node ID 981feb6ad9ccc13a62483d7a883b67a16d344ed9 # Parent b49b6786afad9248b4c764a003baa067b3437fd7 8062308: Incorrect constant linkage with multiple Globals in a Context Reviewed-by: lagergren, sundar diff -r b49b6786afad -r 981feb6ad9cc src/jdk/nashorn/internal/objects/Global.java --- a/src/jdk/nashorn/internal/objects/Global.java Thu Nov 06 13:15:52 2014 +0100 +++ b/src/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 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 */ diff -r b49b6786afad -r 981feb6ad9cc src/jdk/nashorn/internal/runtime/Context.java --- a/src/jdk/nashorn/internal/runtime/Context.java Thu Nov 06 13:15:52 2014 +0100 +++ b/src/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 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(); } /** diff -r b49b6786afad -r 981feb6ad9cc src/jdk/nashorn/internal/runtime/GlobalConstants.java --- a/src/jdk/nashorn/internal/runtime/GlobalConstants.java Thu Nov 06 13:15:52 2014 +0100 +++ b/src/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 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); } } diff -r b49b6786afad -r 981feb6ad9cc src/jdk/nashorn/internal/runtime/ScriptObject.java --- a/src/jdk/nashorn/internal/runtime/ScriptObject.java Thu Nov 06 13:15:52 2014 +0100 +++ b/src/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)) {