Reduce the memory footprint of expression classifiers

src/parsing/parser-base.h

Index: src/parsing/parser-base.h
diff --git a/src/parsing/parser-base.h b/src/parsing/parser-base.h
index 6086f7af196b863a590c18a7252fd1f5143a4e5c..cf51ccd58645137b7b2a95584a3a1cc72c9f8d69 100644
--- a/src/parsing/parser-base.h
+++ b/src/parsing/parser-base.h
@@ -385,8 +385,8 @@ class ParserBase : public Traits {
 
     typename Traits::Type::Factory* factory() { return factory_; }
 
-    const List<DestructuringAssignment>& destructuring_assignments_to_rewrite()
-        const {
+    const ZoneList<DestructuringAssignment>&
+        destructuring_assignments_to_rewrite() const {
       return destructuring_assignments_to_rewrite_;
     }
 
@@ -408,6 +408,10 @@ class ParserBase : public Traits {
       }
     }
 
+    ZoneList<typename ExpressionClassifier::Error>* GetReportedErrorList() {
+      return &reported_errors_;
+    }
+
     ReturnExprContext return_expr_context() const {
       return return_expr_context_;
     }
@@ -429,7 +433,7 @@ class ParserBase : public Traits {
 
    private:
     void AddDestructuringAssignment(DestructuringAssignment pair) {
-      destructuring_assignments_to_rewrite_.Add(pair);
+      destructuring_assignments_to_rewrite_.Add(pair, (*scope_stack_)->zone());
     }
 
     V8_INLINE Scope* scope() { return *scope_stack_; }
@@ -466,11 +470,13 @@ class ParserBase : public Traits {
     Scope** scope_stack_;
     Scope* outer_scope_;
 
-    List<DestructuringAssignment> destructuring_assignments_to_rewrite_;
+    ZoneList<DestructuringAssignment> destructuring_assignments_to_rewrite_;

[adamk, 2016/06/06 20:51:02]

Yikes, was this a memory leak before? Maybe pull this into a separate patch?

[nickie, 2016/06/08 11:38:46]

As far as I understand, it was not a memory leak.  It was just using the heap,
instead of the zone, for storing the list of DestructuringAssignments.  The list
destructor clears the space.

[adamk, 2016/06/09 08:25:36]

Ah, I forgot this was in FunctionState, not some ZoneObject.

     TailCallExpressionList tail_call_expressions_;
     ReturnExprContext return_expr_context_;
     ZoneList<ExpressionT> non_patterns_to_rewrite_;
 
+    ZoneList<typename ExpressionClassifier::Error> reported_errors_;
+
     typename Traits::Type::Factory* factory_;
 
     // If true, the next (and immediately following) function literal is
@@ -1193,9 +1199,11 @@ ParserBase<Traits>::FunctionState::FunctionState(
       outer_function_state_(*function_state_stack),
       scope_stack_(scope_stack),
       outer_scope_(*scope_stack),
+      destructuring_assignments_to_rewrite_(16, scope->zone()),
       tail_call_expressions_(scope->zone()),
       return_expr_context_(ReturnExprContext::kInsideValidBlock),
       non_patterns_to_rewrite_(0, scope->zone()),
+      reported_errors_(16, scope->zone()),
       factory_(factory),
       next_function_is_parenthesized_(false),
       this_function_is_parenthesized_(false) {
@@ -1552,12 +1560,11 @@ ParserBase<Traits>::ParsePrimaryExpression(ExpressionClassifier* classifier,
       // Parentheses are not valid on the LHS of a BindingPattern, so we use the
       // is_valid_binding_pattern() check to detect multiple levels of
       // parenthesization.
-      if (!classifier->is_valid_binding_pattern()) {
-        ArrowFormalParametersUnexpectedToken(classifier);
-      }
+      bool pattern_error = !classifier->is_valid_binding_pattern();
       classifier->RecordPatternError(scanner()->peek_location(),
                                      MessageTemplate::kUnexpectedToken,
                                      Token::String(Token::LPAREN));
+      if (pattern_error) ArrowFormalParametersUnexpectedToken(classifier);

[adamk, 2016/06/06 20:51:02]

Why was this reordering necessary? I think I'm missing exactly what you mean by
using the classifier in a "stack-based fashion".

[nickie, 2016/06/08 11:38:46]

This one is not related to the "stack-based fashion", it's just an optimization.
 If you put the AFP before the BP in the list, when accumulating the AFP will
not be copied and will force you to shift the BP one place to the left.  This
will save the trouble.  It actually occurs several hundred thousand times.  :-)

       Consume(Token::LPAREN);
       if (Check(Token::RPAREN)) {
         // ()=>x.  The continuation that looks for the => is in
@@ -1690,6 +1697,7 @@ typename ParserBase<Traits>::ExpressionT ParserBase<Traits>::ParseExpression(
       seen_rest = is_rest = true;
     }
     int pos = position(), expr_pos = peek_position();
+    ExpressionClassifier binding_classifier(this);

[nickie, 2016/06/08 11:38:46]

Same thing; a block could be introduced here.

     ExpressionT right = this->ParseAssignmentExpression(
         accept_IN, &binding_classifier, CHECK_OK);
     classifier->Accumulate(&binding_classifier,
@@ -2224,9 +2232,14 @@ ParserBase<Traits>::ParseAssignmentExpression(bool accept_IN,
   }
 
   if (peek() == Token::ARROW) {
-    classifier->RecordPatternError(scanner()->peek_location(),
-                                   MessageTemplate::kUnexpectedToken,
-                                   Token::String(Token::ARROW));
+    typename ExpressionClassifier::Error e1 =
+        ExpressionClassifier::BindingPatternError(
+            scanner()->peek_location(), MessageTemplate::kUnexpectedToken,

[adamk, 2016/06/06 20:51:02]

Can you just store the location here, and move the call to RecordPatternError
below? That'd save a good amount of boilerplate.

[nickie, 2016/06/08 11:38:46]

Done.

+            Token::String(Token::ARROW));
+    typename ExpressionClassifier::Error e2 =
+        ExpressionClassifier::AssignmentPatternError(
+            scanner()->peek_location(), MessageTemplate::kUnexpectedToken,
+            Token::String(Token::ARROW));
     ValidateArrowFormalParameters(&arrow_formals_classifier, expression,
                                   parenthesized_formals, is_async, CHECK_OK);
     // This reads strangely, but is correct: it checks whether any
@@ -2263,6 +2276,9 @@ ParserBase<Traits>::ParseAssignmentExpression(bool accept_IN,
     }
     expression = this->ParseArrowFunctionLiteral(
         accept_IN, parameters, is_async, arrow_formals_classifier, CHECK_OK);
+    arrow_formals_classifier.Discard();
+    classifier->RecordBindingPatternError(e1);
+    classifier->RecordAssignmentPatternError(e2);
 
     if (fni_ != nullptr) fni_->Infer();
 
@@ -2502,8 +2518,8 @@ ParserBase<Traits>::ParseConditionalExpression(bool accept_IN,
   if (peek() != Token::CONDITIONAL) return expression;
   CheckNoTailCallExpressions(classifier, CHECK_OK);
   Traits::RewriteNonPattern(classifier, CHECK_OK);
-  ArrowFormalParametersUnexpectedToken(classifier);
   BindingPatternUnexpectedToken(classifier);
+  ArrowFormalParametersUnexpectedToken(classifier);
   Consume(Token::CONDITIONAL);
   // In parsing the first assignment expression in conditional
   // expressions we always accept the 'in' keyword; see ECMA-262,