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

Thu, 11 Dec 2014 17:46:50 +0100

author
hannesw
date
Thu, 11 Dec 2014 17:46:50 +0100
changeset 1126
0a5ec176e9d8
parent 1125
fef78bb8752b
child 1127
ec1fd6967009

8066669: dust.js performance regression caused by primitive field conversion
Reviewed-by: attila, sundar

src/jdk/nashorn/internal/codegen/CodeGenerator.java file | annotate | diff | comparison | revisions
src/jdk/nashorn/internal/codegen/Lower.java file | annotate | diff | comparison | revisions
src/jdk/nashorn/internal/codegen/MethodEmitter.java file | annotate | diff | comparison | revisions
src/jdk/nashorn/internal/codegen/SharedScopeCall.java file | annotate | diff | comparison | revisions
src/jdk/nashorn/internal/ir/AccessNode.java file | annotate | diff | comparison | revisions
src/jdk/nashorn/internal/runtime/ScriptObject.java file | annotate | diff | comparison | revisions
test/script/basic/JDK-8066669.js file | annotate | diff | comparison | revisions
test/script/basic/JDK-8066669.js.EXPECTED file | annotate | diff | comparison | revisions
test/script/basic/list.js.EXPECTED file | annotate | diff | comparison | revisions
     1.1 --- a/src/jdk/nashorn/internal/codegen/CodeGenerator.java	Thu Dec 11 14:32:26 2014 +0100
     1.2 +++ b/src/jdk/nashorn/internal/codegen/CodeGenerator.java	Thu Dec 11 17:46:50 2014 +0100
     1.3 @@ -465,10 +465,10 @@
     1.4              // If this is either __FILE__, __DIR__, or __LINE__ then load the property initially as Object as we'd convert
     1.5              // it anyway for replaceLocationPropertyPlaceholder.
     1.6              if(identNode.isCompileTimePropertyName()) {
     1.7 -                method.dynamicGet(Type.OBJECT, identNode.getSymbol().getName(), flags, identNode.isFunction());
     1.8 +                method.dynamicGet(Type.OBJECT, identNode.getSymbol().getName(), flags, identNode.isFunction(), false);
     1.9                  replaceCompileTimeProperty();
    1.10              } else {
    1.11 -                dynamicGet(identNode.getSymbol().getName(), flags, identNode.isFunction());
    1.12 +                dynamicGet(identNode.getSymbol().getName(), flags, identNode.isFunction(), false);
    1.13              }
    1.14          }
    1.15      }
    1.16 @@ -486,7 +486,7 @@
    1.17  
    1.18      private MethodEmitter storeFastScopeVar(final Symbol symbol, final int flags) {
    1.19          loadFastScopeProto(symbol, true);
    1.20 -        method.dynamicSet(symbol.getName(), flags | CALLSITE_FAST_SCOPE);
    1.21 +        method.dynamicSet(symbol.getName(), flags | CALLSITE_FAST_SCOPE, false);
    1.22          return method;
    1.23      }
    1.24  
    1.25 @@ -745,7 +745,7 @@
    1.26                      @Override
    1.27                      void consumeStack() {
    1.28                          final int flags = getCallSiteFlags();
    1.29 -                        dynamicGet(accessNode.getProperty(), flags, accessNode.isFunction());
    1.30 +                        dynamicGet(accessNode.getProperty(), flags, accessNode.isFunction(), accessNode.isIndex());
    1.31                      }
    1.32                  }.emit(baseAlreadyOnStack ? 1 : 0);
    1.33                  return false;
    1.34 @@ -1449,7 +1449,7 @@
    1.35                          // NOTE: not using a nested OptimisticOperation on this dynamicGet, as we expect to get back
    1.36                          // a callable object. Nobody in their right mind would optimistically type this call site.
    1.37                          assert !node.isOptimistic();
    1.38 -                        method.dynamicGet(node.getType(), node.getProperty(), flags, true);
    1.39 +                        method.dynamicGet(node.getType(), node.getProperty(), flags, true, node.isIndex());
    1.40                          method.swap();
    1.41                          argCount = loadArgs(args);
    1.42                      }
    1.43 @@ -3164,7 +3164,7 @@
    1.44              if (isFastScope(identSymbol)) {
    1.45                  storeFastScopeVar(identSymbol, flags);
    1.46              } else {
    1.47 -                method.dynamicSet(identNode.getName(), flags);
    1.48 +                method.dynamicSet(identNode.getName(), flags, false);
    1.49              }
    1.50          } else {
    1.51              final Type identType = identNode.getType();
    1.52 @@ -4268,7 +4268,7 @@
    1.53                          if (isFastScope(symbol)) {
    1.54                              storeFastScopeVar(symbol, flags);
    1.55                          } else {
    1.56 -                            method.dynamicSet(node.getName(), flags);
    1.57 +                            method.dynamicSet(node.getName(), flags, false);
    1.58                          }
    1.59                      } else {
    1.60                          final Type storeType = assignNode.getType();
    1.61 @@ -4285,7 +4285,7 @@
    1.62  
    1.63                  @Override
    1.64                  public boolean enterAccessNode(final AccessNode node) {
    1.65 -                    method.dynamicSet(node.getProperty(), getCallSiteFlags());
    1.66 +                    method.dynamicSet(node.getProperty(), getCallSiteFlags(), node.isIndex());
    1.67                      return false;
    1.68                  }
    1.69  
    1.70 @@ -4623,11 +4623,11 @@
    1.71           * @param isMethod whether we're preferrably retrieving a function
    1.72           * @return the current method emitter
    1.73           */
    1.74 -        MethodEmitter dynamicGet(final String name, final int flags, final boolean isMethod) {
    1.75 +        MethodEmitter dynamicGet(final String name, final int flags, final boolean isMethod, final boolean isIndex) {
    1.76              if(isOptimistic) {
    1.77 -                return method.dynamicGet(getOptimisticCoercedType(), name, getOptimisticFlags(flags), isMethod);
    1.78 -            }
    1.79 -            return method.dynamicGet(resultBounds.within(expression.getType()), name, nonOptimisticFlags(flags), isMethod);
    1.80 +                return method.dynamicGet(getOptimisticCoercedType(), name, getOptimisticFlags(flags), isMethod, isIndex);
    1.81 +            }
    1.82 +            return method.dynamicGet(resultBounds.within(expression.getType()), name, nonOptimisticFlags(flags), isMethod, isIndex);
    1.83          }
    1.84  
    1.85          MethodEmitter dynamicGetIndex(final int flags, final boolean isMethod) {
     2.1 --- a/src/jdk/nashorn/internal/codegen/Lower.java	Thu Dec 11 14:32:26 2014 +0100
     2.2 +++ b/src/jdk/nashorn/internal/codegen/Lower.java	Thu Dec 11 17:46:50 2014 +0100
     2.3 @@ -34,6 +34,8 @@
     2.4  import java.util.Collections;
     2.5  import java.util.List;
     2.6  import java.util.ListIterator;
     2.7 +import java.util.regex.Pattern;
     2.8 +import jdk.nashorn.internal.ir.AccessNode;
     2.9  import jdk.nashorn.internal.ir.BaseNode;
    2.10  import jdk.nashorn.internal.ir.BinaryNode;
    2.11  import jdk.nashorn.internal.ir.Block;
    2.12 @@ -52,6 +54,7 @@
    2.13  import jdk.nashorn.internal.ir.FunctionNode.CompilationState;
    2.14  import jdk.nashorn.internal.ir.IdentNode;
    2.15  import jdk.nashorn.internal.ir.IfNode;
    2.16 +import jdk.nashorn.internal.ir.IndexNode;
    2.17  import jdk.nashorn.internal.ir.JumpStatement;
    2.18  import jdk.nashorn.internal.ir.LabelNode;
    2.19  import jdk.nashorn.internal.ir.LexicalContext;
    2.20 @@ -93,6 +96,10 @@
    2.21  
    2.22      private final DebugLogger log;
    2.23  
    2.24 +    // Conservative pattern to test if element names consist of characters valid for identifiers.
    2.25 +    // This matches any non-zero length alphanumeric string including _ and $ and not starting with a digit.
    2.26 +    private static Pattern SAFE_PROPERTY_NAME = Pattern.compile("[a-zA-Z_$][\\w$]*");
    2.27 +
    2.28      /**
    2.29       * Constructor.
    2.30       */
    2.31 @@ -140,7 +147,7 @@
    2.32              }
    2.33          });
    2.34  
    2.35 -        this.log       = initLogger(compiler.getContext());
    2.36 +        this.log = initLogger(compiler.getContext());
    2.37      }
    2.38  
    2.39      @Override
    2.40 @@ -181,6 +188,28 @@
    2.41      }
    2.42  
    2.43      @Override
    2.44 +    public Node leaveIndexNode(final IndexNode indexNode) {
    2.45 +        final String name = getConstantPropertyName(indexNode.getIndex());
    2.46 +        if (name != null) {
    2.47 +            // If index node is a constant property name convert index node to access node.
    2.48 +            assert Token.descType(indexNode.getToken()) == TokenType.LBRACKET;
    2.49 +            return new AccessNode(indexNode.getToken(), indexNode.getFinish(), indexNode.getBase(), name);
    2.50 +        }
    2.51 +        return super.leaveIndexNode(indexNode);
    2.52 +    }
    2.53 +
    2.54 +    // If expression is a primitive literal that is not an array index and does return its string value. Else return null.
    2.55 +    private static String getConstantPropertyName(final Expression expression) {
    2.56 +        if (expression instanceof LiteralNode.PrimitiveLiteralNode) {
    2.57 +            final Object value = ((LiteralNode) expression).getValue();
    2.58 +            if (value instanceof String && SAFE_PROPERTY_NAME.matcher((String) value).matches()) {
    2.59 +                return (String) value;
    2.60 +            }
    2.61 +        }
    2.62 +        return null;
    2.63 +    }
    2.64 +
    2.65 +    @Override
    2.66      public Node leaveExpressionStatement(final ExpressionStatement expressionStatement) {
    2.67          final Expression expr = expressionStatement.getExpression();
    2.68          ExpressionStatement node = expressionStatement;
     3.1 --- a/src/jdk/nashorn/internal/codegen/MethodEmitter.java	Thu Dec 11 14:32:26 2014 +0100
     3.2 +++ b/src/jdk/nashorn/internal/codegen/MethodEmitter.java	Thu Dec 11 17:46:50 2014 +0100
     3.3 @@ -2214,10 +2214,10 @@
     3.4       * @param name      name of property
     3.5       * @param flags     call site flags
     3.6       * @param isMethod  should it prefer retrieving methods
     3.7 -     *
     3.8 +     * @param isIndex   is this an index operation?
     3.9       * @return the method emitter
    3.10       */
    3.11 -    MethodEmitter dynamicGet(final Type valueType, final String name, final int flags, final boolean isMethod) {
    3.12 +    MethodEmitter dynamicGet(final Type valueType, final String name, final int flags, final boolean isMethod, final boolean isIndex) {
    3.13          debug("dynamic_get", name, valueType, getProgramPoint(flags));
    3.14  
    3.15          Type type = valueType;
    3.16 @@ -2226,8 +2226,8 @@
    3.17          }
    3.18  
    3.19          popType(Type.SCOPE);
    3.20 -        method.visitInvokeDynamicInsn((isMethod ? "dyn:getMethod|getProp|getElem:" : "dyn:getProp|getElem|getMethod:") +
    3.21 -                NameCodec.encode(name), Type.getMethodDescriptor(type, Type.OBJECT), LINKERBOOTSTRAP, flags);
    3.22 +        method.visitInvokeDynamicInsn(dynGetOperation(isMethod, isIndex) + ':' + NameCodec.encode(name),
    3.23 +                Type.getMethodDescriptor(type, Type.OBJECT), LINKERBOOTSTRAP, flags);
    3.24  
    3.25          pushType(type);
    3.26          convert(valueType); //most probably a nop
    3.27 @@ -2240,8 +2240,9 @@
    3.28       *
    3.29       * @param name  name of property
    3.30       * @param flags call site flags
    3.31 +     * @param isIndex is this an index operation?
    3.32       */
    3.33 -     void dynamicSet(final String name, final int flags) {
    3.34 +    void dynamicSet(final String name, final int flags, final boolean isIndex) {
    3.35           assert !isOptimistic(flags);
    3.36           debug("dynamic_set", name, peekType());
    3.37  
    3.38 @@ -2253,7 +2254,8 @@
    3.39          popType(type);
    3.40          popType(Type.SCOPE);
    3.41  
    3.42 -        method.visitInvokeDynamicInsn("dyn:setProp|setElem:" + NameCodec.encode(name), methodDescriptor(void.class, Object.class, type.getTypeClass()), LINKERBOOTSTRAP, flags);
    3.43 +        method.visitInvokeDynamicInsn(dynSetOperation(isIndex) + ':' + NameCodec.encode(name),
    3.44 +                methodDescriptor(void.class, Object.class, type.getTypeClass()), LINKERBOOTSTRAP, flags);
    3.45      }
    3.46  
    3.47       /**
    3.48 @@ -2286,7 +2288,7 @@
    3.49  
    3.50          final String signature = Type.getMethodDescriptor(resultType, Type.OBJECT /*e.g STRING->OBJECT*/, index);
    3.51  
    3.52 -        method.visitInvokeDynamicInsn(isMethod ? "dyn:getMethod|getElem|getProp" : "dyn:getElem|getProp|getMethod", signature, LINKERBOOTSTRAP, flags);
    3.53 +        method.visitInvokeDynamicInsn(dynGetOperation(isMethod, true), signature, LINKERBOOTSTRAP, flags);
    3.54          pushType(resultType);
    3.55  
    3.56          if (result.isBoolean()) {
    3.57 @@ -2500,6 +2502,18 @@
    3.58          }
    3.59      }
    3.60  
    3.61 +    private static String dynGetOperation(final boolean isMethod, final boolean isIndex) {
    3.62 +        if (isMethod) {
    3.63 +            return isIndex ? "dyn:getMethod|getElem|getProp" : "dyn:getMethod|getProp|getElem";
    3.64 +        } else {
    3.65 +            return isIndex ? "dyn:getElem|getProp|getMethod" : "dyn:getProp|getElem|getMethod";
    3.66 +        }
    3.67 +    }
    3.68 +
    3.69 +    private static String dynSetOperation(final boolean isIndex) {
    3.70 +        return isIndex ? "dyn:setElem|setProp" : "dyn:setProp|setElem";
    3.71 +    }
    3.72 +
    3.73      private Type emitLocalVariableConversion(final LocalVariableConversion conversion, final boolean onlySymbolLiveValue) {
    3.74          final Type from = conversion.getFrom();
    3.75          final Type to = conversion.getTo();
     4.1 --- a/src/jdk/nashorn/internal/codegen/SharedScopeCall.java	Thu Dec 11 14:32:26 2014 +0100
     4.2 +++ b/src/jdk/nashorn/internal/codegen/SharedScopeCall.java	Thu Dec 11 17:46:50 2014 +0100
     4.3 @@ -156,7 +156,7 @@
     4.4          assert !isCall || valueType.isObject(); // Callables are always objects
     4.5          // If flags are optimistic, but we're doing a call, remove optimistic flags from the getter, as they obviously
     4.6          // only apply to the call.
     4.7 -        method.dynamicGet(valueType, symbol.getName(), isCall ? CodeGenerator.nonOptimisticFlags(flags) : flags, isCall);
     4.8 +        method.dynamicGet(valueType, symbol.getName(), isCall ? CodeGenerator.nonOptimisticFlags(flags) : flags, isCall, false);
     4.9  
    4.10          // If this is a get we're done, otherwise call the value as function.
    4.11          if (isCall) {
     5.1 --- a/src/jdk/nashorn/internal/ir/AccessNode.java	Thu Dec 11 14:32:26 2014 +0100
     5.2 +++ b/src/jdk/nashorn/internal/ir/AccessNode.java	Thu Dec 11 17:46:50 2014 +0100
     5.3 @@ -28,6 +28,8 @@
     5.4  import jdk.nashorn.internal.codegen.types.Type;
     5.5  import jdk.nashorn.internal.ir.annotations.Immutable;
     5.6  import jdk.nashorn.internal.ir.visitor.NodeVisitor;
     5.7 +import jdk.nashorn.internal.parser.Token;
     5.8 +import jdk.nashorn.internal.parser.TokenType;
     5.9  
    5.10  /**
    5.11   * IR representation of a property access (period operator.)
    5.12 @@ -101,6 +103,14 @@
    5.13          return property;
    5.14      }
    5.15  
    5.16 +    /**
    5.17 +     * Return true if this node represents an index operation normally represented as {@link IndexNode}.
    5.18 +     * @return true if an index access.
    5.19 +     */
    5.20 +    public boolean isIndex() {
    5.21 +        return Token.descType(getToken()) == TokenType.LBRACKET;
    5.22 +    }
    5.23 +
    5.24      private AccessNode setBase(final Expression base) {
    5.25          if (this.base == base) {
    5.26              return this;
     6.1 --- a/src/jdk/nashorn/internal/runtime/ScriptObject.java	Thu Dec 11 14:32:26 2014 +0100
     6.2 +++ b/src/jdk/nashorn/internal/runtime/ScriptObject.java	Thu Dec 11 17:46:50 2014 +0100
     6.3 @@ -2001,12 +2001,11 @@
     6.4  
     6.5          if (find == null) {
     6.6              switch (operator) {
     6.7 +            case "getElem": // getElem only gets here if element name is constant, so treat it like a property access
     6.8              case "getProp":
     6.9                  return noSuchProperty(desc, request);
    6.10              case "getMethod":
    6.11                  return noSuchMethod(desc, request);
    6.12 -            case "getElem":
    6.13 -                return createEmptyGetter(desc, explicitInstanceOfCheck, name);
    6.14              default:
    6.15                  throw new AssertionError(operator); // never invoked with any other operation
    6.16              }
     7.1 --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
     7.2 +++ b/test/script/basic/JDK-8066669.js	Thu Dec 11 17:46:50 2014 +0100
     7.3 @@ -0,0 +1,58 @@
     7.4 +/*
     7.5 + * Copyright (c) 2010, 2014, Oracle and/or its affiliates. All rights reserved.
     7.6 + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
     7.7 + * 
     7.8 + * This code is free software; you can redistribute it and/or modify it
     7.9 + * under the terms of the GNU General Public License version 2 only, as
    7.10 + * published by the Free Software Foundation.
    7.11 + * 
    7.12 + * This code is distributed in the hope that it will be useful, but WITHOUT
    7.13 + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
    7.14 + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
    7.15 + * version 2 for more details (a copy is included in the LICENSE file that
    7.16 + * accompanied this code).
    7.17 + * 
    7.18 + * You should have received a copy of the GNU General Public License version
    7.19 + * 2 along with this work; if not, write to the Free Software Foundation,
    7.20 + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
    7.21 + * 
    7.22 + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
    7.23 + * or visit www.oracle.com if you need additional information or have any
    7.24 + * questions.
    7.25 + */
    7.26 +
    7.27 +/**
    7.28 + * JDK-8066669: dust.js performance regression caused by primitive field conversion
    7.29 + *
    7.30 + * @test
    7.31 + * @run
    7.32 + */
    7.33 +
    7.34 +// Make sure index access on Java objects is working as expected.
    7.35 +var map = new java.util.HashMap();
    7.36 +
    7.37 +map["foo"] = "bar";
    7.38 +map[1] = 2;
    7.39 +map[false] = true;
    7.40 +map[null] = 0;
    7.41 +
    7.42 +print(map);
    7.43 +
    7.44 +var keys =  map.keySet().iterator();
    7.45 +
    7.46 +while(keys.hasNext()) {
    7.47 +    var key = keys.next();
    7.48 +    print(typeof key, key);
    7.49 +}
    7.50 +
    7.51 +print(typeof map["foo"], map["foo"]);
    7.52 +print(typeof map[1], map[1]);
    7.53 +print(typeof map[false], map[false]);
    7.54 +print(typeof map[null], map[null]);
    7.55 +
    7.56 +print(map.foo);
    7.57 +print(map.false);
    7.58 +print(map.null);
    7.59 +
    7.60 +map.foo = "baz";
    7.61 +print(map);
     8.1 --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
     8.2 +++ b/test/script/basic/JDK-8066669.js.EXPECTED	Thu Dec 11 17:46:50 2014 +0100
     8.3 @@ -0,0 +1,13 @@
     8.4 +{null=0, 1=2, false=true, foo=bar}
     8.5 +object null
     8.6 +number 1
     8.7 +boolean false
     8.8 +string foo
     8.9 +string bar
    8.10 +number 2
    8.11 +boolean true
    8.12 +number 0
    8.13 +bar
    8.14 +null
    8.15 +null
    8.16 +{null=0, 1=2, false=true, foo=baz}
     9.1 --- a/test/script/basic/list.js.EXPECTED	Thu Dec 11 14:32:26 2014 +0100
     9.2 +++ b/test/script/basic/list.js.EXPECTED	Thu Dec 11 17:46:50 2014 +0100
     9.3 @@ -10,7 +10,7 @@
     9.4  l[0]=foo
     9.5  l[1]=a
     9.6  l[0.9]=null
     9.7 -l['blah']=null
     9.8 +l['blah']=undefined
     9.9  l[size_name]()=2
    9.10  java.lang.IndexOutOfBoundsException: Index: 2, Size: 2
    9.11  java.lang.IndexOutOfBoundsException: Index: Infinity, Size: 2

mercurial