8074556: Functions should not share allocator maps

Reviewed-by: lagergren, sundar

nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/FindScopeDepths.java

@@ -32,7 +32,6 @@ import java.util.HashSet;
 import java.util.Iterator;
 import java.util.Map;
 import java.util.Set;
-import jdk.nashorn.internal.codegen.ObjectClassGenerator.AllocatorDescriptor;
 import jdk.nashorn.internal.ir.Block;
 import jdk.nashorn.internal.ir.FunctionNode;
 import jdk.nashorn.internal.ir.FunctionNode.CompilationState;
@@ -208,7 +207,7 @@ final class FindScopeDepths extends NodeVisitor<LexicalContext> implements Logga
         final RecompilableScriptFunctionData data = new RecompilableScriptFunctionData(
                 newFunctionNode,
                 compiler.getCodeInstaller(),
-                new AllocatorDescriptor(newFunctionNode.getThisProperties()),
+                ObjectClassGenerator.createAllocationStrategy(newFunctionNode.getThisProperties()),
                 nestedFunctions,
                 externalSymbolDepths.get(fnId),
                 internalSymbols.get(fnId),

nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/ObjectClassGenerator.java

@@ -56,6 +56,7 @@ import java.util.List;
 import jdk.nashorn.internal.codegen.ClassEmitter.Flag;
 import jdk.nashorn.internal.codegen.types.Type;
 import jdk.nashorn.internal.runtime.AccessorProperty;
+import jdk.nashorn.internal.runtime.AllocationStrategy;
 import jdk.nashorn.internal.runtime.Context;
 import jdk.nashorn.internal.runtime.FunctionScope;
 import jdk.nashorn.internal.runtime.JSType;
@@ -826,44 +827,15 @@ public final class ObjectClassGenerator implements Loggable {
     }
 
     /**
-     * Describes the allocator class name and property map for a constructor function with the specified
+     * Creates the allocator class name and property map for a constructor function with the specified
      * number of "this" properties that it initializes.
-     *
+     * @param thisProperties number of properties assigned to "this"
+     * @return the allocation strategy
      */
-    public static class AllocatorDescriptor {
-        private final String allocatorClassName;
-        private final PropertyMap allocatorMap;
-
-        /**
-         * Creates a new allocator descriptor
-         * @param thisProperties the number of "this" properties that the function initializes
-         */
-        public AllocatorDescriptor(final int thisProperties) {
-            final int paddedFieldCount = getPaddedFieldCount(thisProperties);
-            this.allocatorClassName = Compiler.binaryName(getClassName(paddedFieldCount));
-            this.allocatorMap = PropertyMap.newMap(null, allocatorClassName, 0, paddedFieldCount, 0);
-        }
-
-        /**
-         * Returns the name of the class that the function allocates
-         * @return the name of the class that the function allocates
-         */
-        public String getAllocatorClassName() {
-            return allocatorClassName;
-        }
-
-        /**
-         * Returns the allocator map for the function.
-         * @return the allocator map for the function.
-         */
-        public PropertyMap getAllocatorMap() {
-            return allocatorMap;
-        }
-
-        @Override
-        public String toString() {
-            return "AllocatorDescriptor[allocatorClassName=" + allocatorClassName + ", allocatorMap.size=" +
-                    allocatorMap.size() + "]";
-        }
+    static AllocationStrategy createAllocationStrategy(final int thisProperties) {
+        final int paddedFieldCount = getPaddedFieldCount(thisProperties);
+        final String allocatorClassName = Compiler.binaryName(getClassName(paddedFieldCount));
+        final PropertyMap allocatorMap = PropertyMap.newMap(null, allocatorClassName, 0, paddedFieldCount, 0);
+        return new AllocationStrategy(allocatorMap, allocatorClassName);
     }
 }

nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/runtime/AllocationStrategy.java

@@ -30,22 +30,15 @@ import java.io.Serializable;
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodHandles;
 import jdk.nashorn.internal.codegen.CompilerConstants;
-import jdk.nashorn.internal.codegen.ObjectClassGenerator.AllocatorDescriptor;
 
 /**
- * Encapsulates the allocation strategy for a function when used as a constructor. Basically the same as
- * {@link AllocatorDescriptor}, but with an additionally cached resolved method handle. There is also a
- * canonical default allocation strategy for functions that don't assign any "this" properties (vast majority
- * of all functions), therefore saving some storage space in {@link RecompilableScriptFunctionData} that would
- * otherwise be lost to identical tuples of (map, className, handle) fields.
+ * Encapsulates the allocation strategy for a function when used as a constructor.
  */
-final class AllocationStrategy implements Serializable {
+final public class AllocationStrategy implements Serializable {
     private static final long serialVersionUID = 1L;
 
     private static final MethodHandles.Lookup LOOKUP = MethodHandles.lookup();
 
-    private static final AllocationStrategy DEFAULT_STRATEGY = new AllocationStrategy(new AllocatorDescriptor(0));
-
     /** Allocator map from allocator descriptor */
     private final PropertyMap allocatorMap;
 
@@ -55,19 +48,15 @@ final class AllocationStrategy implements Serializable {
     /** lazily generated allocator */
     private transient MethodHandle allocator;
 
-    private AllocationStrategy(final AllocatorDescriptor desc) {
-        this.allocatorMap = desc.getAllocatorMap();
+    /**
+     * Construct an allocation strategy with the given map and class name.
+     * @param allocatorMap the property map
+     * @param className the class name
+     */
+    public AllocationStrategy(final PropertyMap allocatorMap, final String className) {
+        this.allocatorMap = allocatorMap;
         // These classes get loaded, so an interned variant of their name is most likely around anyway.
-        this.allocatorClassName = desc.getAllocatorClassName().intern();
-    }
-
-    private boolean matches(final AllocatorDescriptor desc) {
-        return desc.getAllocatorMap().size() == allocatorMap.size() &&
-                desc.getAllocatorClassName().equals(allocatorClassName);
-    }
-
-    static AllocationStrategy get(final AllocatorDescriptor desc) {
-        return DEFAULT_STRATEGY.matches(desc) ? DEFAULT_STRATEGY : new AllocationStrategy(desc);
+        this.allocatorClassName = className.intern();
     }
 
     PropertyMap getAllocatorMap() {
@@ -88,14 +77,6 @@ final class AllocationStrategy implements Serializable {
         }
     }
 
-    private Object readResolve() {
-        if(allocatorMap.size() == DEFAULT_STRATEGY.allocatorMap.size() &&
-                allocatorClassName.equals(DEFAULT_STRATEGY.allocatorClassName)) {
-            return DEFAULT_STRATEGY;
-        }
-        return this;
-    }
-
     @Override
     public String toString() {
         return "AllocationStrategy[allocatorClassName=" + allocatorClassName + ", allocatorMap.size=" +

nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/runtime/RecompilableScriptFunctionData.java

@@ -43,7 +43,6 @@ import jdk.nashorn.internal.codegen.Compiler.CompilationPhases;
 import jdk.nashorn.internal.codegen.CompilerConstants;
 import jdk.nashorn.internal.codegen.FunctionSignature;
 import jdk.nashorn.internal.codegen.Namespace;
-import jdk.nashorn.internal.codegen.ObjectClassGenerator.AllocatorDescriptor;
 import jdk.nashorn.internal.codegen.OptimisticTypesPersistence;
 import jdk.nashorn.internal.codegen.TypeMap;
 import jdk.nashorn.internal.codegen.types.Type;
@@ -126,7 +125,7 @@ public final class RecompilableScriptFunctionData extends ScriptFunctionData imp
      *
      * @param functionNode        functionNode that represents this function code
      * @param installer           installer for code regeneration versions of this function
-     * @param allocationDescriptor descriptor for the allocation behavior when this function is used as a constructor
+     * @param allocationStrategy  strategy for the allocation behavior when this function is used as a constructor
      * @param nestedFunctions     nested function map
      * @param externalScopeDepths external scope depths
      * @param internalSymbols     internal symbols to method, defined in its scope
@@ -135,7 +134,7 @@ public final class RecompilableScriptFunctionData extends ScriptFunctionData imp
     public RecompilableScriptFunctionData(
         final FunctionNode functionNode,
         final CodeInstaller<ScriptEnvironment> installer,
-        final AllocatorDescriptor allocationDescriptor,
+        final AllocationStrategy allocationStrategy,
         final Map<Integer, RecompilableScriptFunctionData> nestedFunctions,
         final Map<String, Integer> externalScopeDepths,
         final Set<String> internalSymbols,
@@ -153,7 +152,7 @@ public final class RecompilableScriptFunctionData extends ScriptFunctionData imp
         this.endParserState      = functionNode.getEndParserState();
         this.token               = tokenFor(functionNode);
         this.installer           = installer;
-        this.allocationStrategy  = AllocationStrategy.get(allocationDescriptor);
+        this.allocationStrategy  = allocationStrategy;
         this.nestedFunctions     = smallMap(nestedFunctions);
         this.externalScopeDepths = smallMap(externalScopeDepths);
         this.internalSymbols     = smallSet(new HashSet<>(internalSymbols));

nashorn/test/script/basic/JDK-8074556.js

@@ -0,0 +1,52 @@
+/*
+ * Copyright (c) 2015, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ * 
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ * 
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ * 
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ * 
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/**
+ * JDK-8074556: Functions should not share allocator maps
+ *
+ * @test
+ * @run
+ */
+
+function A () {
+    return this;
+}
+
+function B() {
+    return this;
+}
+
+A.prototype.x = "x";
+A.prototype.y = "y";
+B.prototype.y = "y";  // same properties but different order
+B.prototype.x = "x";
+
+function test(o) {
+    Assert.assertEquals(o.x, "x");
+    Assert.assertEquals(o.y, "y");
+}
+
+test(new A());
+test(new B());
+test(new A());
+test(new B());