Bug 1825722 - Where possible avoid allocating arguments, and use JSOp::ArgumentsLength instead r=arai

Differential Revision: https://phabricator.services.mozilla.com/D203144

js/src/frontend/BytecodeEmitter.cpp

@@ -12514,19 +12514,27 @@ bool BytecodeEmitter::emitTree(
     }
 
     case ParseNodeKind::ArgumentsLength: {
-      PropOpEmitter poe(this, PropOpEmitter::Kind::Get,
-                        PropOpEmitter::ObjKind::Other);
-      if (!poe.prepareForObj()) {
-        return false;
-      }
+      if (sc->isFunctionBox() &&
+          sc->asFunctionBox()->isEligibleForArgumentsLength() &&
+          !sc->asFunctionBox()->needsArgsObj()) {
+        if (!emit1(JSOp::ArgumentsLength)) {
+          return false;
+        }
+      } else {
+        PropOpEmitter poe(this, PropOpEmitter::Kind::Get,
+                          PropOpEmitter::ObjKind::Other);
+        if (!poe.prepareForObj()) {
+          return false;
+        }
 
-      NameOpEmitter noe(this, TaggedParserAtomIndex::WellKnown::arguments(),
-                        NameOpEmitter::Kind::Get);
-      if (!noe.emitGet()) {
-        return false;
-      }
-      if (!poe.emitGet(TaggedParserAtomIndex::WellKnown::length())) {
-        return false;
+        NameOpEmitter noe(this, TaggedParserAtomIndex::WellKnown::arguments(),
+                          NameOpEmitter::Kind::Get);
+        if (!noe.emitGet()) {
+          return false;
+        }
+        if (!poe.emitGet(TaggedParserAtomIndex::WellKnown::length())) {
+          return false;
+        }
       }
       break;
     }

js/src/frontend/FullParseHandler.h

@@ -1162,6 +1162,10 @@ class FullParseHandler {
                TaggedParserAtomIndex::WellKnown::async();
   }
 
+  bool isArgumentsLength(Node node) {
+    return node->isKind(ParseNodeKind::ArgumentsLength);
+  }
+
   bool isPrivateName(Node node) {
     return node->isKind(ParseNodeKind::PrivateName);
   }

js/src/frontend/ParseContext.cpp

@@ -664,12 +664,36 @@ bool ParseContext::declareFunctionArgumentsObject(
   // Time to implement the odd semantics of 'arguments'.
   auto argumentsName = TaggedParserAtomIndex::WellKnown::arguments();
 
-  bool tryDeclareArguments;
-  // When delazifying simply defer to the function box
+  bool tryDeclareArguments = false;
+  bool needsArgsObject = false;
+
+  // When delazifying simply defer to the function box.
   if (canSkipLazyClosedOverBindings) {
     tryDeclareArguments = funbox->shouldDeclareArguments();
+    needsArgsObject = funbox->needsArgsObj();
   } else {
-    tryDeclareArguments = hasUsedFunctionSpecialName(usedNames, argumentsName);
+    // We cannot compute these values when delazifying, hence why we need to
+    // rely on the function box flags instead.
+    bool bindingClosedOver =
+        hasClosedOverFunctionSpecialName(usedNames, argumentsName);
+    bool bindingUsedOnlyHere =
+        hasUsedFunctionSpecialName(usedNames, argumentsName) &&
+        !bindingClosedOver;
+
+    // Declare arguments if there's a closed-over consumer of the binding, or if
+    // there is a non-length use and we will reference the binding during
+    // bytecode emission.
+    tryDeclareArguments =
+        !funbox->isEligibleForArgumentsLength() || bindingClosedOver;
+    // If we have a use and the binding isn't closed over, then we will do
+    // bytecode emission with the arguments intrinsic.
+    if (bindingUsedOnlyHere && funbox->isEligibleForArgumentsLength()) {
+      // If we're using the intrinsic we should not be declaring the binding.
+      MOZ_ASSERT(!tryDeclareArguments);
+      funbox->setUsesArgumentsIntrinsics();
+    } else if (tryDeclareArguments) {
+      needsArgsObject = true;
+    }
   }
 
   // ES 9.2.12 steps 19 and 20 say formal parameters, lexical bindings,
@@ -691,7 +715,9 @@ bool ParseContext::declareFunctionArgumentsObject(
     } else {
       // A binding in the function scope (since varScope and functionScope are
       // the same) exists, so arguments is used.
-      funbox->setNeedsArgsObj();
+      if (needsArgsObject) {
+        funbox->setNeedsArgsObj();
+      }
 
       // There is no point in continuing on below: We know we already have
       // a declaration of arguments in the function scope.
@@ -708,7 +734,9 @@ bool ParseContext::declareFunctionArgumentsObject(
         return false;
       }
       funbox->setShouldDeclareArguments();
-      funbox->setNeedsArgsObj();
+      if (needsArgsObject) {
+        funbox->setNeedsArgsObj();
+      }
     }
   }
   return true;

js/src/frontend/ParseContext.h

@@ -677,6 +677,13 @@ class ParseContext : public Nestable<ParseContext> {
   bool declareDotGeneratorName();
   bool declareTopLevelDotGeneratorName();
 
+  // Used to determine if we have non-length uses of the arguments binding.
+  // This works by incrementing this counter each time we encounter the
+  // arguments name, and decrementing each time it is combined into
+  // arguments.length; as a result, if this is non-zero at the end of parsing,
+  // we have identified a non-length use of the arguments binding.
+  size_t numberOfArgumentsNames = 0;
+
  private:
   [[nodiscard]] bool isVarRedeclaredInInnermostScope(
       TaggedParserAtomIndex name, ParserBase* parser, DeclarationKind kind,

js/src/frontend/Parser.cpp

@@ -2472,6 +2472,11 @@ GeneralParser<ParseHandler, Unit>::functionBody(InHandling inHandling,
     }
   }
 
+  if (pc_->numberOfArgumentsNames > 0 || kind == FunctionSyntaxKind::Arrow) {
+    MOZ_ASSERT(pc_->isFunctionBox());
+    pc_->sc()->setIneligibleForArgumentsLength();
+  }
+
   // Declare the 'arguments', 'this', and 'new.target' bindings if necessary
   // before finishing up the scope so these special bindings get marked as
   // closed over if necessary. Arrow functions don't have these bindings.
@@ -6570,6 +6575,8 @@ bool GeneralParser<ParseHandler, Unit>::forHeadStart(
         return false;
       }
     }
+  } else if (handler_.isArgumentsLength(*forInitialPart)) {
+    pc_->sc()->setIneligibleForArgumentsLength();
   } else if (handler_.isPropertyOrPrivateMemberAccess(*forInitialPart)) {
     // Permitted: no additional testing/fixup needed.
   } else if (handler_.isFunctionCall(*forInitialPart)) {
@@ -10220,6 +10227,8 @@ typename ParseHandler::NodeResult GeneralParser<ParseHandler, Unit>::assignExpr(
         return errorResult();
       }
     }
+  } else if (handler_.isArgumentsLength(lhs)) {
+    pc_->sc()->setIneligibleForArgumentsLength();
   } else if (handler_.isPropertyOrPrivateMemberAccess(lhs)) {
     // Permitted: no additional testing/fixup needed.
   } else if (handler_.isFunctionCall(lhs)) {
@@ -10280,6 +10289,8 @@ bool GeneralParser<ParseHandler, Unit>::checkIncDecOperand(
         return false;
       }
     }
+  } else if (handler_.isArgumentsLength(operand)) {
+    pc_->sc()->setIneligibleForArgumentsLength();
   } else if (handler_.isPropertyOrPrivateMemberAccess(operand)) {
     // Permitted: no additional testing/fixup needed.
   } else if (handler_.isFunctionCall(operand)) {
@@ -10898,6 +10909,9 @@ template <class ParseHandler>
 inline typename ParseHandler::NameNodeResult
 PerHandlerParser<ParseHandler>::newName(TaggedParserAtomIndex name,
                                         TokenPos pos) {
+  if (name == TaggedParserAtomIndex::WellKnown::arguments()) {
+    this->pc_->numberOfArgumentsNames++;
+  }
   return handler_.newName(name, pos);
 }
 
@@ -10928,6 +10942,8 @@ GeneralParser<ParseHandler, Unit>::memberPropertyAccess(
   }
 
   if (handler_.isArgumentsName(lhs) && handler_.isLengthName(name)) {
+    MOZ_ASSERT(pc_->numberOfArgumentsNames > 0);
+    pc_->numberOfArgumentsNames--;
     return handler_.newArgumentsLength(lhs, name);
   }
 
@@ -11489,6 +11505,10 @@ void GeneralParser<ParseHandler, Unit>::checkDestructuringAssignmentName(
     return;
   }
 
+  if (handler_.isArgumentsLength(name)) {
+    pc_->sc()->setIneligibleForArgumentsLength();
+  }
+
   if (pc_->sc()->strict()) {
     if (handler_.isArgumentsName(name)) {
       if (pc_->sc()->strict()) {
@@ -12148,6 +12168,10 @@ GeneralParser<ParseHandler, Unit>::objectLiteral(YieldHandling yieldHandling,
           }
         }
 
+        if (handler_.isArgumentsLength(lhs)) {
+          pc_->sc()->setIneligibleForArgumentsLength();
+        }
+
         Node rhs;
         MOZ_TRY_VAR(rhs,
                     assignExpr(InAllowed, yieldHandling, TripledotProhibited));

js/src/frontend/SharedContext.cpp

@@ -45,7 +45,8 @@ SharedContext::SharedContext(FrontendContext* fc, Kind kind,
       inClass_(false),
       localStrict(false),
       hasExplicitUseStrict_(false),
-      isScriptExtraFieldCopiedToStencil(false) {
+      isScriptExtraFieldCopiedToStencil(false),
+      eligibleForArgumentsLength(true) {
   // Compute the script kind "input" flags.
   if (kind == Kind::FunctionBox) {
     setFlag(ImmutableFlags::IsFunction);

js/src/frontend/SharedContext.h

@@ -180,6 +180,10 @@ class SharedContext {
   // FunctionBox::copyUpdated* methods.
   bool isScriptExtraFieldCopiedToStencil : 1;
 
+  // Indicates this shared context is eligible to use JSOp::ArgumentsLength
+  // when emitting the ArgumentsLength parse node.
+  bool eligibleForArgumentsLength : 1;
+
   // End of fields.
 
   enum class Kind : uint8_t { FunctionBox, Global, Eval, Module };
@@ -273,6 +277,11 @@ class SharedContext {
     return retVal;
   }
 
+  bool isEligibleForArgumentsLength() {
+    return eligibleForArgumentsLength && !bindingsAccessedDynamically();
+  }
+  void setIneligibleForArgumentsLength() { eligibleForArgumentsLength = false; }
+
   void copyScriptExtraFields(ScriptStencilExtra& scriptExtra);
 };
 

js/src/frontend/SyntaxParseHandler.h

@@ -215,9 +215,6 @@ class SyntaxParseHandler {
     if (name == TaggedParserAtomIndex::WellKnown::arguments()) {
       return NodeArgumentsName;
     }
-    if (name == TaggedParserAtomIndex::WellKnown::length()) {
-      return NodeLengthName;
-    }
     if (pos.begin + strlen("async") == pos.end &&
         name == TaggedParserAtomIndex::WellKnown::async()) {
       return NodePotentialAsyncKeyword;
@@ -580,6 +577,9 @@ class SyntaxParseHandler {
   NameNodeResult newPropertyName(TaggedParserAtomIndex name,
                                  const TokenPos& pos) {
     lastAtom = name;
+    if (name == TaggedParserAtomIndex::WellKnown::length()) {
+      return NodeLengthName;
+    }
     return NodeGeneric;
   }
 
@@ -798,6 +798,8 @@ class SyntaxParseHandler {
   bool isEvalName(Node node) { return node == NodeEvalName; }
   bool isAsyncKeyword(Node node) { return node == NodePotentialAsyncKeyword; }
 
+  bool isArgumentsLength(Node node) { return node == NodeArgumentsLength; }
+
   bool isPrivateName(Node node) { return node == NodePrivateName; }
   bool isPrivateMemberAccess(Node node) {
     return node == NodePrivateMemberAccess;

js/src/tests/non262/argumentsLengthOpt.js

@@ -0,0 +1,87 @@
+// Test cases for arguments.length optimization.
+
+function f1() {
+    return arguments.length;
+}
+
+function f2(a, b, c) {
+    return arguments.length;
+}
+
+// arrow functions don't have their own arguments, and so capture the enclosing
+// scope.
+function f3(a, b, c, d) {
+    return (() => arguments.length)();
+}
+
+// Test a function which mutates arguments.length
+function f4(a, b, c, d) {
+    arguments.length = 42;
+    return arguments.length;
+}
+
+// Manually read out arguments; should disable the length opt
+function f5() {
+    for (var i = 0; i < arguments.length; i++) {
+        if (arguments[i] == 10) { return true }
+    }
+    return false;
+}
+
+function f6() {
+    function inner() {
+        return arguments.length;
+    }
+    return inner(1, 2, 3);
+}
+
+// edge cases of the arguments bindings:
+function f7() {
+    var arguments = 42;
+    return arguments;
+}
+
+function f8() {
+    var arguments = [1, 2];
+    return arguments.length;
+}
+
+function f9() {
+    eval("arguments.length = 42");
+    return arguments.length;
+}
+
+function test() {
+    assertEq(f1(), 0);
+    assertEq(f1(1), 1);
+    assertEq(f1(1, 2), 2);
+    assertEq(f1(1, 2, 3), 3);
+
+    assertEq(f2(), 0);
+    assertEq(f2(1, 2, 3), 3);
+
+    assertEq(f3(), 0);
+    assertEq(f3(1, 2, 3), 3);
+
+    assertEq(f4(), 42);
+    assertEq(f4(1, 2, 3), 42);
+
+    assertEq(f5(), false);
+    assertEq(f5(1, 2, 3, 10), true);
+    assertEq(f5(1, 2, 3, 10, 20), true);
+    assertEq(f5(1, 2, 3, 9, 20, 30), false);
+
+    assertEq(f6(), 3)
+    assertEq(f6(1, 2, 3, 4), 3)
+
+    assertEq(f7(), 42);
+
+    assertEq(f8(), 2);
+
+    assertEq(f9(), 42);
+}
+
+test();
+
+if (typeof reportCompare === "function")
+    reportCompare(0, 0, "ok");

js/src/vm/Opcodes.h

@@ -2937,7 +2937,8 @@
     /*
      * Push the number of actual arguments as Int32Value.
      *
-     * This is emitted for the ArgumentsLength() intrinsic in self-hosted code.
+     * This is emitted for the ArgumentsLength() intrinsic in self-hosted code,
+     * and if the script uses only arguments.length.
      *
      *   Category: Variables and scopes
      *   Type: Getting binding values