From 55352ac765e3d8f797cc7f8f6f7b088cf6ab2fc4 Mon Sep 17 00:00:00 2001 From: "andrea.bergia" Date: Mon, 9 Dec 2024 11:10:04 +0100 Subject: [PATCH] Get rid of `GETINDEXSUPER` and related Those invokedynamic bridges were never being executed because they where generated only in the combination of `super` with an integer index in optimized functions. However, to make `super` work, the function is marked with "requires activation", which disables the optimizer, which means that the combination `SUPER_PROPERTY_ACCESS` and `ISNUMBER_PROP` actually never occurs. We removed all the dead code to improve coverage. Co-authored-by: Satish Srinivasan --- .../javascript/optimizer/BodyCodegen.java | 48 ++++++++----------- .../javascript/optimizer/Bootstrapper.java | 6 --- .../javascript/optimizer/DefaultLinker.java | 4 -- .../javascript/optimizer/RhinoOperation.java | 2 - .../javascript/optimizer/Signatures.java | 25 ---------- .../org/mozilla/javascript/SuperTest.java | 17 +++++++ 6 files changed, 37 insertions(+), 65 deletions(-) diff --git a/rhino/src/main/java/org/mozilla/javascript/optimizer/BodyCodegen.java b/rhino/src/main/java/org/mozilla/javascript/optimizer/BodyCodegen.java index 447c7f43ec..256858c898 100644 --- a/rhino/src/main/java/org/mozilla/javascript/optimizer/BodyCodegen.java +++ b/rhino/src/main/java/org/mozilla/javascript/optimizer/BodyCodegen.java @@ -1773,11 +1773,7 @@ private void finishGetElemGeneration(Node node, Node child) { if (node.getIntProp(Node.SUPER_PROPERTY_ACCESS, 0) == 1) { cfw.addALoad(thisObjLocal); - if (node.getIntProp(Node.ISNUMBER_PROP, -1) != -1) { - addDynamicInvoke("PROP:GETINDEXSUPER", Signatures.PROP_GET_INDEX_SUPER); - } else { - addDynamicInvoke("PROP:GETELEMENTSUPER", Signatures.PROP_GET_ELEMENT_SUPER); - } + addDynamicInvoke("PROP:GETELEMENTSUPER", Signatures.PROP_GET_ELEMENT_SUPER); } else { if (node.getIntProp(Node.ISNUMBER_PROP, -1) != -1) { addDynamicInvoke("PROP:GETINDEX", Signatures.PROP_GET_INDEX); @@ -4440,48 +4436,44 @@ private void visitSetElem(int type, Node node, Node child) { boolean indexIsNumber = (node.getIntProp(Node.ISNUMBER_PROP, -1) != -1); boolean isSuper = node.getIntProp(Node.SUPER_PROPERTY_ACCESS, 0) == 1; if (type == Token.SETELEM_OP) { - if (indexIsNumber) { + if (isSuper) { + // indexIsNumber will never be true because functions with super always require + // activation, and when they do we not optimize them + + // stack: ... object object indexObject + // -> ... object indexObject object indexObject + cfw.add(ByteCode.DUP_X1); + cfw.addALoad(contextLocal); + cfw.addALoad(variableObjectLocal); + cfw.addALoad(thisObjLocal); + addDynamicInvoke("PROP:GETELEMENTSUPER", Signatures.PROP_GET_ELEMENT_SUPER); + } else if (indexIsNumber) { // stack: ... object object number // -> ... object number object number cfw.add(ByteCode.DUP2_X1); cfw.addALoad(contextLocal); cfw.addALoad(variableObjectLocal); - if (isSuper) { - cfw.addALoad(thisObjLocal); - addDynamicInvoke("PROP:GETINDEXSUPER", Signatures.PROP_GET_INDEX_SUPER); - } else { - addDynamicInvoke("PROP:GETINDEX", Signatures.PROP_GET_INDEX); - } + addDynamicInvoke("PROP:GETINDEX", Signatures.PROP_GET_INDEX); } else { // stack: ... object object indexObject // -> ... object indexObject object indexObject cfw.add(ByteCode.DUP_X1); cfw.addALoad(contextLocal); cfw.addALoad(variableObjectLocal); - if (isSuper) { - cfw.addALoad(thisObjLocal); - addDynamicInvoke("PROP:GETELEMENTSUPER", Signatures.PROP_GET_ELEMENT_SUPER); - } else { - addDynamicInvoke("PROP:GETELEMENT", Signatures.PROP_GET_ELEMENT); - } + addDynamicInvoke("PROP:GETELEMENT", Signatures.PROP_GET_ELEMENT); } } generateExpression(child, node); cfw.addALoad(contextLocal); cfw.addALoad(variableObjectLocal); if (isSuper) { + // Again, we will never have super && indexIsNumber together cfw.addALoad(thisObjLocal); - if (indexIsNumber) { - addDynamicInvoke("PROP:SETINDEXSUPER", Signatures.PROP_SET_INDEX_SUPER); - } else { - addDynamicInvoke("PROP:SETELEMENTSUPER", Signatures.PROP_SET_ELEMENT_SUPER); - } + addDynamicInvoke("PROP:SETELEMENTSUPER", Signatures.PROP_SET_ELEMENT_SUPER); + } else if (indexIsNumber) { + addDynamicInvoke("PROP:SETINDEX", Signatures.PROP_SET_INDEX); } else { - if (indexIsNumber) { - addDynamicInvoke("PROP:SETINDEX", Signatures.PROP_SET_INDEX); - } else { - addDynamicInvoke("PROP:SETELEMENT", Signatures.PROP_SET_ELEMENT); - } + addDynamicInvoke("PROP:SETELEMENT", Signatures.PROP_SET_ELEMENT); } } diff --git a/rhino/src/main/java/org/mozilla/javascript/optimizer/Bootstrapper.java b/rhino/src/main/java/org/mozilla/javascript/optimizer/Bootstrapper.java index b2542fffac..b4426bf827 100644 --- a/rhino/src/main/java/org/mozilla/javascript/optimizer/Bootstrapper.java +++ b/rhino/src/main/java/org/mozilla/javascript/optimizer/Bootstrapper.java @@ -133,9 +133,6 @@ private static Operation parseOperation(String name) throws NoSuchMethodExceptio case "GETINDEX": // Same but the value is definitely a numeric index return RhinoOperation.GETINDEX.withNamespace(StandardNamespace.PROPERTY); - case "GETINDEXSUPER": - // Same but the value is definitely a numeric index - return RhinoOperation.GETINDEXSUPER.withNamespace(StandardNamespace.PROPERTY); case "SET": // Set an object property with a constant name return StandardOperation.SET @@ -155,9 +152,6 @@ private static Operation parseOperation(String name) throws NoSuchMethodExceptio case "SETINDEX": // Same but the property name is definitely a number return RhinoOperation.SETINDEX.withNamespace(StandardNamespace.PROPERTY); - case "SETINDEXSUPER": - // Same but the property name is definitely a number - return RhinoOperation.SETINDEXSUPER.withNamespace(StandardNamespace.PROPERTY); } } else if ("NAME".equals(namespaceName)) { switch (opName) { diff --git a/rhino/src/main/java/org/mozilla/javascript/optimizer/DefaultLinker.java b/rhino/src/main/java/org/mozilla/javascript/optimizer/DefaultLinker.java index cd331757d7..57e0697a98 100644 --- a/rhino/src/main/java/org/mozilla/javascript/optimizer/DefaultLinker.java +++ b/rhino/src/main/java/org/mozilla/javascript/optimizer/DefaultLinker.java @@ -125,16 +125,12 @@ private GuardedInvocation getPropertyInvocation( mh = lookup.findStatic(ScriptRuntime.class, "getSuperElem", mType); } else if (op.isOperation(RhinoOperation.GETINDEX)) { mh = lookup.findStatic(ScriptRuntime.class, "getObjectIndex", mType); - } else if (op.isOperation(RhinoOperation.GETINDEXSUPER)) { - mh = lookup.findStatic(ScriptRuntime.class, "getSuperIndex", mType); } else if (op.isOperation(RhinoOperation.SETELEMENT)) { mh = lookup.findStatic(ScriptRuntime.class, "setObjectElem", mType); } else if (op.isOperation(RhinoOperation.SETELEMENTSUPER)) { mh = lookup.findStatic(ScriptRuntime.class, "setSuperElem", mType); } else if (op.isOperation(RhinoOperation.SETINDEX)) { mh = lookup.findStatic(ScriptRuntime.class, "setObjectIndex", mType); - } else if (op.isOperation(RhinoOperation.SETINDEXSUPER)) { - mh = lookup.findStatic(ScriptRuntime.class, "setSuperIndex", mType); } if (mh != null) { diff --git a/rhino/src/main/java/org/mozilla/javascript/optimizer/RhinoOperation.java b/rhino/src/main/java/org/mozilla/javascript/optimizer/RhinoOperation.java index 9bcd8335ac..a05d41f2bb 100644 --- a/rhino/src/main/java/org/mozilla/javascript/optimizer/RhinoOperation.java +++ b/rhino/src/main/java/org/mozilla/javascript/optimizer/RhinoOperation.java @@ -16,14 +16,12 @@ public enum RhinoOperation implements Operation { GETELEMENT, GETELEMENTSUPER, GETINDEX, - GETINDEXSUPER, SETSTRICT, SETCONST, SETSUPER, SETELEMENT, SETELEMENTSUPER, SETINDEX, - SETINDEXSUPER, ADD, EQ, SHALLOWEQ, diff --git a/rhino/src/main/java/org/mozilla/javascript/optimizer/Signatures.java b/rhino/src/main/java/org/mozilla/javascript/optimizer/Signatures.java index ed8966ffc6..4c0f46a1ba 100644 --- a/rhino/src/main/java/org/mozilla/javascript/optimizer/Signatures.java +++ b/rhino/src/main/java/org/mozilla/javascript/optimizer/Signatures.java @@ -61,18 +61,6 @@ interface Signatures { + "Lorg/mozilla/javascript/Scriptable;" + ")Ljava/lang/Object;"; - /** - * PROP:GETINDEXSUPER: Get a property from super based on a numeric index. Falls back to - * ScriptRuntime.getSuperIndex. - */ - String PROP_GET_INDEX_SUPER = - "(Ljava/lang/Object;" - + "D" - + "Lorg/mozilla/javascript/Context;" - + "Lorg/mozilla/javascript/Scriptable;" - + "Ljava/lang/Object;" - + ")Ljava/lang/Object;"; - /** * PROP:GETELEMENT: Get a property from an object based on an element ID, which could be a * string, number, or symbol. Falls back to ScriptRuntime.getObjectElem. @@ -129,19 +117,6 @@ interface Signatures { + "Lorg/mozilla/javascript/Scriptable;" + ")Ljava/lang/Object;"; - /** - * PROP:SETINDEXSUPER: Set a property on super based on a numeric index. Falls back to - * ScriptRuntime.setSuperIndex. - */ - String PROP_SET_INDEX_SUPER = - "(Ljava/lang/Object;" - + "D" - + "Ljava/lang/Object;" - + "Lorg/mozilla/javascript/Context;" - + "Lorg/mozilla/javascript/Scriptable;" - + "Ljava/lang/Object;" - + ")Ljava/lang/Object;"; - /** * PROP:SETELEMENT: Set a property on an object based on an identifier. Falls back to * ScriptRuntime.setObjectElem. diff --git a/rhino/src/test/java/org/mozilla/javascript/SuperTest.java b/rhino/src/test/java/org/mozilla/javascript/SuperTest.java index 6f2bb3e51b..6dd22df461 100644 --- a/rhino/src/test/java/org/mozilla/javascript/SuperTest.java +++ b/rhino/src/test/java/org/mozilla/javascript/SuperTest.java @@ -768,6 +768,23 @@ void modifyOperatorByName() { Utils.assertWithAllOptimizationLevelsES6("proto1:proto", script); } + @Test + void modifyOperatorByKey() { + // Equivalent to `super[x] = super[x] + 1`, so reads from super, writes in this + String script = + "" + + "const xAsStr = 'x';\n" + + "var proto = { x: 'proto' };" + + "var object = {\n" + + " x: 'obj',\n" + + " f() { super[xAsStr] += '1'; }\n" + + "};\n" + + "Object.setPrototypeOf(object, proto);\n" + + "object.f();" + + "object.x + ':' + proto.x"; + Utils.assertWithAllOptimizationLevelsES6("proto1:proto", script); + } + @Test void deleteNotAllowed() { String script =