Thu, 11 Dec 2014 17:46:50 +0100
8066669: dust.js performance regression caused by primitive field conversion
Reviewed-by: attila, sundar
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