[parser] Only take Scope::Snapshot when it's more likely we'll have an arrow function

That reduces the overhead of ParseAssignmentExpression at the cost of a few
more branches in the possible arrow head paths.

This also fixes the case where an outer scope of an arrow function didn't call eval
but a parameter initializer does. Previously the outer scope was also marked as
calling eval, causing worse performance. (Unlikely to happen though.)

Change-Id: I5263ef342f14e97372f5037fa659f32ec2ad6d34
Reviewed-on: https://chromium-review.googlesource.com/c/1352275
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57881}

src/ast/ast.h

@@ -2339,9 +2339,11 @@ class FunctionLiteral final : public Expression {
 
   enum IdType { kIdTypeInvalid = -1, kIdTypeTopLevel = 0 };
 
-  enum ParameterFlag { kNoDuplicateParameters, kHasDuplicateParameters };
-
-  enum EagerCompileHint { kShouldEagerCompile, kShouldLazyCompile };
+  enum ParameterFlag : uint8_t {
+    kNoDuplicateParameters,
+    kHasDuplicateParameters
+  };
+  enum EagerCompileHint : uint8_t { kShouldEagerCompile, kShouldLazyCompile };
 
   // Empty handle means that the function does not have a shared name (i.e.
   // the name will be set dynamically after creation of the function closure).

src/ast/scopes.cc

@@ -844,7 +844,7 @@ void DeclarationScope::AddLocal(Variable* var) {
   locals_.Add(var);
 }
 
-void Scope::Snapshot::Reparent(DeclarationScope* new_parent) const {
+void Scope::Snapshot::Reparent(DeclarationScope* new_parent) {
   DCHECK_EQ(new_parent, outer_scope_and_calls_eval_.GetPointer()->inner_scope_);
   DCHECK_EQ(new_parent->outer_scope_, outer_scope_and_calls_eval_.GetPointer());
   DCHECK_EQ(new_parent, new_parent->GetClosureScope());
@@ -888,13 +888,12 @@ void Scope::Snapshot::Reparent(DeclarationScope* new_parent) const {
   outer_closure->locals_.Rewind(top_local_);
 
   // Move eval calls since Snapshot's creation into new_parent.
-  if (outer_scope_->scope_calls_eval_) {
+  if (outer_scope_and_calls_eval_.GetPayload()) {
     new_parent->scope_calls_eval_ = true;
     new_parent->inner_scope_calls_eval_ = true;
   }
-  // Reset the outer_scope's eval state. It will be restored to its
-  // original value as necessary in the destructor of this class.
-  outer_scope_->scope_calls_eval_ = false;
+
+  Clear();
 }
 
 void Scope::ReplaceOuterScope(Scope* outer) {

src/ast/scopes.h

@@ -125,21 +125,72 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
 
   class Snapshot final {
    public:
+    Snapshot()
+        : outer_scope_and_calls_eval_(nullptr, false),
+          top_unresolved_(),
+          top_local_() {}
     inline explicit Snapshot(Scope* scope);
+
     ~Snapshot() {
-      // Restore previous calls_eval bit if needed.
-      if (outer_scope_and_calls_eval_.GetPayload()) {
-        outer_scope_and_calls_eval_->scope_calls_eval_ = true;
+      // If we're still active, there was no arrow function. In that case outer
+      // calls eval if it already called eval before this snapshot started, or
+      // if the code during the snapshot called eval.
+      if (!IsCleared() && outer_scope_and_calls_eval_.GetPayload()) {
+        RestoreEvalFlag();
       }
     }
 
-    void Reparent(DeclarationScope* new_parent) const;
+    void RestoreEvalFlag() {
+      outer_scope_and_calls_eval_->scope_calls_eval_ =
+          outer_scope_and_calls_eval_.GetPayload();
+    }
+
+    Snapshot& operator=(Snapshot&& source) V8_NOEXCEPT {
+      outer_scope_and_calls_eval_.SetPointer(
+          source.outer_scope_and_calls_eval_.GetPointer());
+      outer_scope_and_calls_eval_.SetPayload(
+          outer_scope_and_calls_eval_->scope_calls_eval_);
+      top_inner_scope_ = source.top_inner_scope_;
+      top_unresolved_ = source.top_unresolved_;
+      top_local_ = source.top_local_;
+
+      // We are in the arrow function case. The calls eval we may have recorded
+      // is intended for the inner scope and we should simply restore the
+      // original "calls eval" flag of the outer scope.
+      source.RestoreEvalFlag();
+      source.Clear();
+
+      return *this;
+    }
+
+    void Reparent(DeclarationScope* new_parent);
+    bool IsCleared() const {
+      return outer_scope_and_calls_eval_.GetPointer() == nullptr;
+    }
 
    private:
+    void Clear() {
+      outer_scope_and_calls_eval_.SetPointer(nullptr);
+#ifdef DEBUG
+      outer_scope_and_calls_eval_.SetPayload(false);
+      top_inner_scope_ = nullptr;
+      top_local_ = base::ThreadedList<Variable>::Iterator();
+      top_unresolved_ = UnresolvedList::Iterator();
+#endif
+    }
+
+    // During tracking calls_eval caches whether the outer scope called eval.
+    // Upon move assignment we store whether the new inner scope calls eval into
+    // the move target calls_eval bit, and restore calls eval on the outer
+    // scope.
     PointerWithPayload<Scope, bool, 1> outer_scope_and_calls_eval_;
     Scope* top_inner_scope_;
     UnresolvedList::Iterator top_unresolved_;
     base::ThreadedList<Variable>::Iterator top_local_;
+
+    // Disallow copy and move.
+    Snapshot(const Snapshot&) = delete;
+    Snapshot(Snapshot&&) = delete;
   };
 
   enum class DeserializationMode { kIncludingVariables, kScopesOnly };

src/base/threaded-list.h

@@ -155,6 +155,8 @@ class ThreadedListBase final : public BaseClass {
       return *this;
     }
 
+    Iterator() : entry_(nullptr) {}
+
    private:
     explicit Iterator(T** entry) : entry_(entry) {}
 

src/parsing/parser-base.h

@@ -249,9 +249,9 @@ class ParserBase {
         zone_(zone),
         classifier_(nullptr),
         scanner_(scanner),
-        default_eager_compile_hint_(FunctionLiteral::kShouldLazyCompile),
         function_literal_id_(0),
         script_id_(script_id),
+        default_eager_compile_hint_(FunctionLiteral::kShouldLazyCompile),
         allow_natives_(false),
         allow_harmony_public_fields_(false),
         allow_harmony_static_fields_(false),
@@ -267,6 +267,8 @@ class ParserBase {
   bool allow_##name() const { return allow_##name##_; } \
   void set_allow_##name(bool allow) { allow_##name##_ = allow; }
 
+  void set_rewritable_length(int i) { rewritable_length_ = i; }
+
   ALLOW_ACCESSORS(natives);
   ALLOW_ACCESSORS(harmony_public_fields);
   ALLOW_ACCESSORS(harmony_static_fields);
@@ -1116,12 +1118,7 @@ class ParserBase {
   }
   ExpressionT DoParseMemberExpressionContinuation(ExpressionT expression);
 
-  // `rewritable_length`: length of the destructuring_assignments_to_rewrite()
-  // queue in the parent function state, prior to parsing of formal parameters.
-  // If the arrow function is lazy, any items added during formal parameter
-  // parsing are removed from the queue.
-  ExpressionT ParseArrowFunctionLiteral(const FormalParametersT& parameters,
-                                        int rewritable_length);
+  ExpressionT ParseArrowFunctionLiteral(const FormalParametersT& parameters);
   void ParseAsyncFunctionBody(Scope* scope, StatementListT* body);
   ExpressionT ParseAsyncFunctionLiteral();
   ExpressionT ParseClassLiteral(IdentifierT name,
@@ -1451,11 +1448,17 @@ class ParserBase {
 
   Scanner* scanner_;
 
-  FunctionLiteral::EagerCompileHint default_eager_compile_hint_;
+  Scope::Snapshot scope_snapshot_;
+  // `rewritable_length_`: length of the destructuring_assignments_to_rewrite()
+  // queue in the parent function state, prior to parsing of formal parameters.
+  // If the arrow function is lazy, any items added during formal parameter
+  // parsing are removed from the queue.
+  int rewritable_length_ = -1;
 
   int function_literal_id_;
   int script_id_;
 
+  FunctionLiteral::EagerCompileHint default_eager_compile_hint_;
   FunctionKind next_arrow_function_kind_ = FunctionKind::kArrowFunction;
 
   bool accept_IN_ = true;
@@ -1739,6 +1742,11 @@ ParserBase<Impl>::ParsePrimaryExpression() {
         infer = InferName::kNo;
       }
     }
+
+    if (peek() == Token::ARROW) {
+      scope_snapshot_ = std::move(Scope::Snapshot(scope()));
+      rewritable_length_ = 0;
+    }
     return impl()->ExpressionFromIdentifier(name, beg_pos, infer);
   }
   DCHECK_IMPLIES(Token::IsAnyIdentifier(token), token == Token::ENUM);
@@ -1765,10 +1773,15 @@ ParserBase<Impl>::ParsePrimaryExpression() {
 
     case Token::LPAREN: {
       Consume(Token::LPAREN);
+      Scope::Snapshot scope_snapshot(scope());
+      int rewritable_length = static_cast<int>(
+          function_state_->destructuring_assignments_to_rewrite().size());
       if (Check(Token::RPAREN)) {
         // ()=>x.  The continuation that consumes the => is in
         // ParseAssignmentExpression.
         if (peek() != Token::ARROW) ReportUnexpectedToken(Token::RPAREN);
+        scope_snapshot_ = std::move(scope_snapshot);
+        rewritable_length_ = rewritable_length;
         return factory()->NewEmptyParentheses(beg_pos);
       }
       // Heuristically try to detect immediately called functions before
@@ -1781,6 +1794,12 @@ ParserBase<Impl>::ParsePrimaryExpression() {
       ExpressionT expr = ParseExpressionCoverGrammar();
       expr->mark_parenthesized();
       Expect(Token::RPAREN);
+
+      if (peek() == Token::ARROW) {
+        scope_snapshot_ = std::move(scope_snapshot);
+        rewritable_length_ = rewritable_length;
+      }
+
       return expr;
     }
 
@@ -2632,10 +2651,8 @@ ParserBase<Impl>::ParseAssignmentExpression() {
   FuncNameInferrerState fni_state(&fni_);
   ExpressionClassifier arrow_formals_classifier(this);
 
-  Scope::Snapshot scope_snapshot(scope());
-  int rewritable_length = static_cast<int>(
-      function_state_->destructuring_assignments_to_rewrite().size());
-
+  DCHECK_IMPLIES(!has_error(), -1 == rewritable_length_);
+  DCHECK_IMPLIES(!has_error(), scope_snapshot_.IsCleared());
   DCHECK_IMPLIES(!has_error(),
                  FunctionKind::kArrowFunction == next_arrow_function_kind_);
   ExpressionT expression = ParseConditionalExpression();
@@ -2660,9 +2677,10 @@ ParserBase<Impl>::ParseAssignmentExpression() {
     // Reset to default.
     next_arrow_function_kind_ = FunctionKind::kArrowFunction;
 
+    if (has_error()) return impl()->FailureExpression();
     // Because the arrow's parameters were parsed in the outer scope,
     // we need to fix up the scope chain appropriately.
-    scope_snapshot.Reparent(scope);
+    scope_snapshot_.Reparent(scope);
 
     FormalParametersT parameters(scope);
     if (!classifier()->is_simple_parameter_list()) {
@@ -2673,7 +2691,7 @@ ParserBase<Impl>::ParseAssignmentExpression() {
     scope->set_start_position(lhs_beg_pos);
     impl()->DeclareArrowFunctionFormalParameters(&parameters, expression, loc);
 
-    expression = ParseArrowFunctionLiteral(parameters, rewritable_length);
+    expression = ParseArrowFunctionLiteral(parameters);
     Accumulate(ExpressionClassifier::AsyncArrowFormalParametersProduction);
 
     fni_.Infer();
@@ -3064,6 +3082,10 @@ ParserBase<Impl>::ParseLeftHandSideContinuation(ExpressionT result) {
     DCHECK(impl()->IsAsync(impl()->AsIdentifier(result)));
     int pos = position();
 
+    Scope::Snapshot scope_snapshot(scope());
+    int rewritable_length = static_cast<int>(
+        function_state_->destructuring_assignments_to_rewrite().size());
+
     ExpressionListT args(pointer_buffer());
     bool has_spread;
     ParseArguments(&args, &has_spread, true);
@@ -3075,6 +3097,8 @@ ParserBase<Impl>::ParseLeftHandSideContinuation(ExpressionT result) {
         return impl()->FailureExpression();
       }
       next_arrow_function_kind_ = FunctionKind::kAsyncArrowFunction;
+      scope_snapshot_ = std::move(scope_snapshot);
+      rewritable_length_ = rewritable_length;
       // async () => ...
       if (!args.length()) return factory()->NewEmptyParentheses(pos);
       // async ( Arguments ) => ...
@@ -4011,7 +4035,7 @@ bool ParserBase<Impl>::IsNextLetKeyword() {
 template <typename Impl>
 typename ParserBase<Impl>::ExpressionT
 ParserBase<Impl>::ParseArrowFunctionLiteral(
-    const FormalParametersT& formal_parameters, int rewritable_length) {
+    const FormalParametersT& formal_parameters) {
   const RuntimeCallCounterId counters[2][2] = {
       {RuntimeCallCounterId::kParseBackgroundArrowFunctionLiteral,
        RuntimeCallCounterId::kParseArrowFunctionLiteral},
@@ -4052,10 +4076,12 @@ ParserBase<Impl>::ParseArrowFunctionLiteral(
     FunctionState function_state(&function_state_, &scope_,
                                  formal_parameters.scope);
 
+    DCHECK_IMPLIES(!has_error(), -1 != rewritable_length_);
     // Move any queued destructuring assignments which appeared
     // in this function's parameter list into its own function_state.
     function_state.AdoptDestructuringAssignmentsFromParentState(
-        rewritable_length);
+        rewritable_length_);
+    rewritable_length_ = -1;
 
     Consume(Token::ARROW);
 

src/parsing/parser.cc

@@ -798,11 +798,8 @@ FunctionLiteral* Parser::DoParseFunction(Isolate* isolate, ParseInfo* info,
         SkipFunctionLiterals(info->function_literal_id() - 1);
       }
 
-      // Any destructuring assignments in the current FunctionState
-      // actually belong to the arrow function itself.
-      const int rewritable_length = 0;
-      Expression* expression =
-          ParseArrowFunctionLiteral(formals, rewritable_length);
+      set_rewritable_length(0);
+      Expression* expression = ParseArrowFunctionLiteral(formals);
       // Scanning must end at the same position that was recorded
       // previously. If not, parsing has been interrupted due to a stack
       // overflow, at which point the partially parsed arrow function