Mercurial > hg > openjdk > aarch64-port > nashorn
changeset 1150:0a5ec176e9d8
8066669: dust.js performance regression caused by primitive field conversion
Reviewed-by: attila, sundar
author | hannesw |
---|---|
date | Thu, 11 Dec 2014 17:46:50 +0100 |
parents | fef78bb8752b |
children | ec1fd6967009 |
files | src/jdk/nashorn/internal/codegen/CodeGenerator.java src/jdk/nashorn/internal/codegen/Lower.java src/jdk/nashorn/internal/codegen/MethodEmitter.java src/jdk/nashorn/internal/codegen/SharedScopeCall.java src/jdk/nashorn/internal/ir/AccessNode.java src/jdk/nashorn/internal/runtime/ScriptObject.java test/script/basic/JDK-8066669.js test/script/basic/JDK-8066669.js.EXPECTED test/script/basic/list.js.EXPECTED |
diffstat | 9 files changed, 147 insertions(+), 24 deletions(-) [+] |
line wrap: on
line diff
--- a/src/jdk/nashorn/internal/codegen/CodeGenerator.java Thu Dec 11 14:32:26 2014 +0100 +++ b/src/jdk/nashorn/internal/codegen/CodeGenerator.java Thu Dec 11 17:46:50 2014 +0100 @@ -465,10 +465,10 @@ // 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 @@ 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 @@ @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 @@ // 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); } @@ -3164,7 +3164,7 @@ if (isFastScope(identSymbol)) { storeFastScopeVar(identSymbol, flags); } else { - method.dynamicSet(identNode.getName(), flags); + method.dynamicSet(identNode.getName(), flags, false); } } else { final Type identType = identNode.getType(); @@ -4268,7 +4268,7 @@ if (isFastScope(symbol)) { storeFastScopeVar(symbol, flags); } else { - method.dynamicSet(node.getName(), flags); + method.dynamicSet(node.getName(), flags, false); } } else { final Type storeType = assignNode.getType(); @@ -4285,7 +4285,7 @@ @Override public boolean enterAccessNode(final AccessNode node) { - method.dynamicSet(node.getProperty(), getCallSiteFlags()); + method.dynamicSet(node.getProperty(), getCallSiteFlags(), node.isIndex()); return false; } @@ -4623,11 +4623,11 @@ * @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(resultBounds.within(expression.getType()), name, nonOptimisticFlags(flags), isMethod); + return method.dynamicGet(getOptimisticCoercedType(), name, getOptimisticFlags(flags), isMethod, isIndex); + } + return method.dynamicGet(resultBounds.within(expression.getType()), name, nonOptimisticFlags(flags), isMethod, isIndex); } MethodEmitter dynamicGetIndex(final int flags, final boolean isMethod) {
--- a/src/jdk/nashorn/internal/codegen/Lower.java Thu Dec 11 14:32:26 2014 +0100 +++ b/src/jdk/nashorn/internal/codegen/Lower.java Thu Dec 11 17:46:50 2014 +0100 @@ -34,6 +34,8 @@ 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.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 @@ 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. */ @@ -140,7 +147,7 @@ } }); - this.log = initLogger(compiler.getContext()); + this.log = initLogger(compiler.getContext()); } @Override @@ -181,6 +188,28 @@ } @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(); ExpressionStatement node = expressionStatement;
--- a/src/jdk/nashorn/internal/codegen/MethodEmitter.java Thu Dec 11 14:32:26 2014 +0100 +++ b/src/jdk/nashorn/internal/codegen/MethodEmitter.java Thu Dec 11 17:46:50 2014 +0100 @@ -2214,10 +2214,10 @@ * @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) { debug("dynamic_get", name, valueType, getProgramPoint(flags)); Type type = valueType; @@ -2226,8 +2226,8 @@ } popType(Type.SCOPE); - method.visitInvokeDynamicInsn((isMethod ? "dyn:getMethod|getProp|getElem:" : "dyn:getProp|getElem|getMethod:") + - NameCodec.encode(name), Type.getMethodDescriptor(type, Type.OBJECT), LINKERBOOTSTRAP, flags); + method.visitInvokeDynamicInsn(dynGetOperation(isMethod, isIndex) + ':' + NameCodec.encode(name), + Type.getMethodDescriptor(type, Type.OBJECT), LINKERBOOTSTRAP, flags); pushType(type); convert(valueType); //most probably a nop @@ -2240,8 +2240,9 @@ * * @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) { assert !isOptimistic(flags); debug("dynamic_set", name, peekType()); @@ -2253,7 +2254,8 @@ 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); } /** @@ -2286,7 +2288,7 @@ 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()) { @@ -2500,6 +2502,18 @@ } } + 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();
--- a/src/jdk/nashorn/internal/codegen/SharedScopeCall.java Thu Dec 11 14:32:26 2014 +0100 +++ b/src/jdk/nashorn/internal/codegen/SharedScopeCall.java Thu Dec 11 17:46:50 2014 +0100 @@ -156,7 +156,7 @@ 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) {
--- a/src/jdk/nashorn/internal/ir/AccessNode.java Thu Dec 11 14:32:26 2014 +0100 +++ b/src/jdk/nashorn/internal/ir/AccessNode.java Thu Dec 11 17:46:50 2014 +0100 @@ -28,6 +28,8 @@ 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 @@ 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;
--- a/src/jdk/nashorn/internal/runtime/ScriptObject.java Thu Dec 11 14:32:26 2014 +0100 +++ b/src/jdk/nashorn/internal/runtime/ScriptObject.java Thu Dec 11 17:46:50 2014 +0100 @@ -2001,12 +2001,11 @@ 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 }
--- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/test/script/basic/JDK-8066669.js Thu Dec 11 17:46:50 2014 +0100 @@ -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);
--- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/test/script/basic/JDK-8066669.js.EXPECTED Thu Dec 11 17:46:50 2014 +0100 @@ -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}
--- a/test/script/basic/list.js.EXPECTED Thu Dec 11 14:32:26 2014 +0100 +++ b/test/script/basic/list.js.EXPECTED Thu Dec 11 17:46:50 2014 +0100 @@ -10,7 +10,7 @@ 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