8066669: dust.js performance regression caused by primitive field conversion

Reviewed-by: attila, sundar

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

@@ -465,10 +465,10 @@ final class CodeGenerator extends NodeOperatorVisitor<CodeGeneratorLexicalContex
             // If this is either __FILE__, __DIR__, or __LINE__ then load the property initially as Object as we'd convert
             // it anyway for replaceLocationPropertyPlaceholder.
             if(identNode.isCompileTimePropertyName()) {
-                method.dynamicGet(Type.OBJECT, identNode.getSymbol().getName(), flags, identNode.isFunction());
+                method.dynamicGet(Type.OBJECT, identNode.getSymbol().getName(), flags, identNode.isFunction(), false);
                 replaceCompileTimeProperty();
             } else {
-                dynamicGet(identNode.getSymbol().getName(), flags, identNode.isFunction());
+                dynamicGet(identNode.getSymbol().getName(), flags, identNode.isFunction(), false);
             }
         }
     }
@@ -486,7 +486,7 @@ final class CodeGenerator extends NodeOperatorVisitor<CodeGeneratorLexicalContex
 
     private MethodEmitter storeFastScopeVar(final Symbol symbol, final int flags) {
         loadFastScopeProto(symbol, true);
-        method.dynamicSet(symbol.getName(), flags | CALLSITE_FAST_SCOPE);
+        method.dynamicSet(symbol.getName(), flags | CALLSITE_FAST_SCOPE, false);
         return method;
     }
 
@@ -745,7 +745,7 @@ final class CodeGenerator extends NodeOperatorVisitor<CodeGeneratorLexicalContex
                     @Override
                     void consumeStack() {
                         final int flags = getCallSiteFlags();
-                        dynamicGet(accessNode.getProperty(), flags, accessNode.isFunction());
+                        dynamicGet(accessNode.getProperty(), flags, accessNode.isFunction(), accessNode.isIndex());
                     }
                 }.emit(baseAlreadyOnStack ? 1 : 0);
                 return false;
@@ -1449,7 +1449,7 @@ final class CodeGenerator extends NodeOperatorVisitor<CodeGeneratorLexicalContex
                         // NOTE: not using a nested OptimisticOperation on this dynamicGet, as we expect to get back
                         // a callable object. Nobody in their right mind would optimistically type this call site.
                         assert !node.isOptimistic();
-                        method.dynamicGet(node.getType(), node.getProperty(), flags, true);
+                        method.dynamicGet(node.getType(), node.getProperty(), flags, true, node.isIndex());
                         method.swap();
                         argCount = loadArgs(args);
                     }
@@ -3165,7 +3165,7 @@ final class CodeGenerator extends NodeOperatorVisitor<CodeGeneratorLexicalContex
             if (isFastScope(identSymbol)) {
                 storeFastScopeVar(identSymbol, flags);
             } else {
-                method.dynamicSet(identNode.getName(), flags);
+                method.dynamicSet(identNode.getName(), flags, false);
             }
         } else {
             final Type identType = identNode.getType();
@@ -4269,7 +4269,7 @@ final class CodeGenerator extends NodeOperatorVisitor<CodeGeneratorLexicalContex
                         if (isFastScope(symbol)) {
                             storeFastScopeVar(symbol, flags);
                         } else {
-                            method.dynamicSet(node.getName(), flags);
+                            method.dynamicSet(node.getName(), flags, false);
                         }
                     } else {
                         final Type storeType = assignNode.getType();
@@ -4286,7 +4286,7 @@ final class CodeGenerator extends NodeOperatorVisitor<CodeGeneratorLexicalContex
 
                 @Override
                 public boolean enterAccessNode(final AccessNode node) {
-                    method.dynamicSet(node.getProperty(), getCallSiteFlags());
+                    method.dynamicSet(node.getProperty(), getCallSiteFlags(), node.isIndex());
                     return false;
                 }
 
@@ -4624,11 +4624,11 @@ final class CodeGenerator extends NodeOperatorVisitor<CodeGeneratorLexicalContex
          * @param isMethod whether we're preferrably retrieving a function
          * @return the current method emitter
          */
-        MethodEmitter dynamicGet(final String name, final int flags, final boolean isMethod) {
+        MethodEmitter dynamicGet(final String name, final int flags, final boolean isMethod, final boolean isIndex) {
             if(isOptimistic) {
-                return method.dynamicGet(getOptimisticCoercedType(), name, getOptimisticFlags(flags), isMethod);
+                return method.dynamicGet(getOptimisticCoercedType(), name, getOptimisticFlags(flags), isMethod, isIndex);
             }
-            return method.dynamicGet(resultBounds.within(expression.getType()), name, nonOptimisticFlags(flags), isMethod);
+            return method.dynamicGet(resultBounds.within(expression.getType()), name, nonOptimisticFlags(flags), isMethod, isIndex);
         }
 
         MethodEmitter dynamicGetIndex(final int flags, final boolean isMethod) {

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

@@ -34,6 +34,8 @@ import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.ListIterator;
+import java.util.regex.Pattern;
+import jdk.nashorn.internal.ir.AccessNode;
 import jdk.nashorn.internal.ir.BaseNode;
 import jdk.nashorn.internal.ir.BinaryNode;
 import jdk.nashorn.internal.ir.Block;
@@ -52,6 +54,7 @@ import jdk.nashorn.internal.ir.FunctionNode;
 import jdk.nashorn.internal.ir.FunctionNode.CompilationState;
 import jdk.nashorn.internal.ir.IdentNode;
 import jdk.nashorn.internal.ir.IfNode;
+import jdk.nashorn.internal.ir.IndexNode;
 import jdk.nashorn.internal.ir.JumpStatement;
 import jdk.nashorn.internal.ir.LabelNode;
 import jdk.nashorn.internal.ir.LexicalContext;
@@ -93,6 +96,10 @@ final class Lower extends NodeOperatorVisitor<BlockLexicalContext> implements Lo
 
     private final DebugLogger log;
 
+    // Conservative pattern to test if element names consist of characters valid for identifiers.
+    // This matches any non-zero length alphanumeric string including _ and $ and not starting with a digit.
+    private static Pattern SAFE_PROPERTY_NAME = Pattern.compile("[a-zA-Z_$][\\w$]*");
+
     /**
      * Constructor.
      */
@@ -180,6 +187,28 @@ final class Lower extends NodeOperatorVisitor<BlockLexicalContext> implements Lo
         return false;
     }
 
+    @Override
+    public Node leaveIndexNode(final IndexNode indexNode) {
+        final String name = getConstantPropertyName(indexNode.getIndex());
+        if (name != null) {
+            // If index node is a constant property name convert index node to access node.
+            assert Token.descType(indexNode.getToken()) == TokenType.LBRACKET;
+            return new AccessNode(indexNode.getToken(), indexNode.getFinish(), indexNode.getBase(), name);
+        }
+        return super.leaveIndexNode(indexNode);
+    }
+
+    // If expression is a primitive literal that is not an array index and does return its string value. Else return null.
+    private static String getConstantPropertyName(final Expression expression) {
+        if (expression instanceof LiteralNode.PrimitiveLiteralNode) {
+            final Object value = ((LiteralNode) expression).getValue();
+            if (value instanceof String && SAFE_PROPERTY_NAME.matcher((String) value).matches()) {
+                return (String) value;
+            }
+        }
+        return null;
+    }
+
     @Override
     public Node leaveExpressionStatement(final ExpressionStatement expressionStatement) {
         final Expression expr = expressionStatement.getExpression();

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

@@ -2213,10 +2213,10 @@ public class MethodEmitter implements Emitter {
      * @param name      name of property
      * @param flags     call site flags
      * @param isMethod  should it prefer retrieving methods
-     *
+     * @param isIndex   is this an index operation?
      * @return the method emitter
      */
-    MethodEmitter dynamicGet(final Type valueType, final String name, final int flags, final boolean isMethod) {
+    MethodEmitter dynamicGet(final Type valueType, final String name, final int flags, final boolean isMethod, final boolean isIndex) {
         if (name.length() > LARGE_STRING_THRESHOLD) { // use getIndex for extremely long names
             return load(name).dynamicGetIndex(valueType, flags, isMethod);
         }
@@ -2229,8 +2229,8 @@ public class MethodEmitter implements Emitter {
         }
 
         popType(Type.SCOPE);
-        method.visitInvokeDynamicInsn((isMethod ? "dyn:getMethod|getProp|getElem:" : "dyn:getProp|getElem|getMethod:") +
+        method.visitInvokeDynamicInsn(dynGetOperation(isMethod, isIndex) + ':' + NameCodec.encode(name),
-                NameCodec.encode(name), Type.getMethodDescriptor(type, Type.OBJECT), LINKERBOOTSTRAP, flags);
+                Type.getMethodDescriptor(type, Type.OBJECT), LINKERBOOTSTRAP, flags);
 
         pushType(type);
         convert(valueType); //most probably a nop
@@ -2243,8 +2243,9 @@ public class MethodEmitter implements Emitter {
      *
      * @param name  name of property
      * @param flags call site flags
+     * @param isIndex is this an index operation?
      */
-    void dynamicSet(final String name, final int flags) {
+    void dynamicSet(final String name, final int flags, final boolean isIndex) {
         if (name.length() > LARGE_STRING_THRESHOLD) { // use setIndex for extremely long names
             load(name).swap().dynamicSetIndex(flags);
             return;
@@ -2261,7 +2262,8 @@ public class MethodEmitter implements Emitter {
         popType(type);
         popType(Type.SCOPE);
 
-        method.visitInvokeDynamicInsn("dyn:setProp|setElem:" + NameCodec.encode(name), methodDescriptor(void.class, Object.class, type.getTypeClass()), LINKERBOOTSTRAP, flags);
+        method.visitInvokeDynamicInsn(dynSetOperation(isIndex) + ':' + NameCodec.encode(name),
+                methodDescriptor(void.class, Object.class, type.getTypeClass()), LINKERBOOTSTRAP, flags);
     }
 
      /**
@@ -2294,7 +2296,7 @@ public class MethodEmitter implements Emitter {
 
         final String signature = Type.getMethodDescriptor(resultType, Type.OBJECT /*e.g STRING->OBJECT*/, index);
 
-        method.visitInvokeDynamicInsn(isMethod ? "dyn:getMethod|getElem|getProp" : "dyn:getElem|getProp|getMethod", signature, LINKERBOOTSTRAP, flags);
+        method.visitInvokeDynamicInsn(dynGetOperation(isMethod, true), signature, LINKERBOOTSTRAP, flags);
         pushType(resultType);
 
         if (result.isBoolean()) {
@@ -2508,6 +2510,18 @@ public class MethodEmitter implements Emitter {
         }
     }
 
+    private static String dynGetOperation(final boolean isMethod, final boolean isIndex) {
+        if (isMethod) {
+            return isIndex ? "dyn:getMethod|getElem|getProp" : "dyn:getMethod|getProp|getElem";
+        } else {
+            return isIndex ? "dyn:getElem|getProp|getMethod" : "dyn:getProp|getElem|getMethod";
+        }
+    }
+
+    private static String dynSetOperation(final boolean isIndex) {
+        return isIndex ? "dyn:setElem|setProp" : "dyn:setProp|setElem";
+    }
+
     private Type emitLocalVariableConversion(final LocalVariableConversion conversion, final boolean onlySymbolLiveValue) {
         final Type from = conversion.getFrom();
         final Type to = conversion.getTo();

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

@@ -156,7 +156,7 @@ class SharedScopeCall {
         assert !isCall || valueType.isObject(); // Callables are always objects
         // If flags are optimistic, but we're doing a call, remove optimistic flags from the getter, as they obviously
         // only apply to the call.
-        method.dynamicGet(valueType, symbol.getName(), isCall ? CodeGenerator.nonOptimisticFlags(flags) : flags, isCall);
+        method.dynamicGet(valueType, symbol.getName(), isCall ? CodeGenerator.nonOptimisticFlags(flags) : flags, isCall, false);
 
         // If this is a get we're done, otherwise call the value as function.
         if (isCall) {

nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/ir/AccessNode.java

@@ -28,6 +28,8 @@ package jdk.nashorn.internal.ir;
 import jdk.nashorn.internal.codegen.types.Type;
 import jdk.nashorn.internal.ir.annotations.Immutable;
 import jdk.nashorn.internal.ir.visitor.NodeVisitor;
+import jdk.nashorn.internal.parser.Token;
+import jdk.nashorn.internal.parser.TokenType;
 
 /**
  * IR representation of a property access (period operator.)
@@ -101,6 +103,14 @@ public final class AccessNode extends BaseNode {
         return property;
     }
 
+    /**
+     * Return true if this node represents an index operation normally represented as {@link IndexNode}.
+     * @return true if an index access.
+     */
+    public boolean isIndex() {
+        return Token.descType(getToken()) == TokenType.LBRACKET;
+    }
+
     private AccessNode setBase(final Expression base) {
         if (this.base == base) {
             return this;

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

@@ -2001,12 +2001,11 @@ public abstract class ScriptObject implements PropertyAccess, Cloneable {
 
         if (find == null) {
             switch (operator) {
+            case "getElem": // getElem only gets here if element name is constant, so treat it like a property access
             case "getProp":
                 return noSuchProperty(desc, request);
             case "getMethod":
                 return noSuchMethod(desc, request);
-            case "getElem":
-                return createEmptyGetter(desc, explicitInstanceOfCheck, name);
             default:
                 throw new AssertionError(operator); // never invoked with any other operation
             }

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

@@ -0,0 +1,58 @@
+/*
+ * Copyright (c) 2010, 2014, 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-8066669: dust.js performance regression caused by primitive field conversion
+ *
+ * @test
+ * @run
+ */
+
+// Make sure index access on Java objects is working as expected.
+var map = new java.util.HashMap();
+
+map["foo"] = "bar";
+map[1] = 2;
+map[false] = true;
+map[null] = 0;
+
+print(map);
+
+var keys =  map.keySet().iterator();
+
+while(keys.hasNext()) {
+    var key = keys.next();
+    print(typeof key, key);
+}
+
+print(typeof map["foo"], map["foo"]);
+print(typeof map[1], map[1]);
+print(typeof map[false], map[false]);
+print(typeof map[null], map[null]);
+
+print(map.foo);
+print(map.false);
+print(map.null);
+
+map.foo = "baz";
+print(map);

nashorn/test/script/basic/JDK-8066669.js.EXPECTED

@@ -0,0 +1,13 @@
+{null=0, 1=2, false=true, foo=bar}
+object null
+number 1
+boolean false
+string foo
+string bar
+number 2
+boolean true
+number 0
+bar
+null
+null
+{null=0, 1=2, false=true, foo=baz}

nashorn/test/script/basic/list.js.EXPECTED

@@ -10,7 +10,7 @@ bar
 l[0]=foo
 l[1]=a
 l[0.9]=null
-l['blah']=null
+l['blah']=undefined
 l[size_name]()=2
 java.lang.IndexOutOfBoundsException: Index: 2, Size: 2
 java.lang.IndexOutOfBoundsException: Index: Infinity, Size: 2