Revert of Put Scopes into temporary Zone (patchset #12 id:220001 of...

Revert of Put Scopes into temporary Zone (patchset #12 id:220001 of https://codereview.chromium.org/2193793002/ )

Reason for revert:
Broke Node.js tests (test-require-dot etc.)

Original issue's description:
> Put Scopes into temporary Zone
>
> When parsing a eagerly-parsed-but-lazily-compiled function, we
> used to put some of its AST nodes into a discardable Zone. This
> CL puts the function Scope, its inner Scopes and the related AST
> nodes (Declarations, VariableProxys) into the temporary Zone
> too. This reduces peak memory usage and enables future work to
> keep the temporary Zone around for later compilation.
>
> BUG=
>
> Committed: https://crrev.com/eaebdd858b466057ccc39894a172c9b66868e8f7
> Cr-Commit-Position: refs/heads/master@{#38232}

TBR=adamk@chromium.org,titzer@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=

Review-Url: https://codereview.chromium.org/2205013002
Cr-Commit-Position: refs/heads/master@{#38258}

src/ast/ast.cc

@@ -204,18 +204,6 @@ VariableProxy::VariableProxy(Zone* zone, const AstRawString* name,
       raw_name_(name),
       next_unresolved_(nullptr) {}
 
-VariableProxy::VariableProxy(Zone* zone, const VariableProxy* copy_from)
-    : Expression(zone, copy_from->position(), kVariableProxy),
-      bit_field_(copy_from->bit_field_),
-      end_position_(copy_from->end_position_),
-      next_unresolved_(nullptr) {
-  if (copy_from->is_resolved()) {
-    var_ = copy_from->var_;
-  } else {
-    raw_name_ = copy_from->raw_name_;
-  }
-}
-
 void VariableProxy::BindTo(Variable* var) {
   DCHECK((is_this() && var->is_this()) || raw_name() == var->raw_name());
   set_var(var);

src/ast/scopes.cc

@@ -279,7 +279,6 @@ bool Scope::Analyze(ParseInfo* info) {
     scope->Print();
   }
   scope->CheckScopePositions();
-  scope->CheckZones();
 #endif
 
   info->set_scope(scope);
@@ -503,11 +502,9 @@ Variable* Scope::LookupFunctionVar(const AstRawString* name,
     if (index < 0) return NULL;
     Variable* var = new (zone())
         Variable(this, name, mode, Variable::NORMAL, kCreatedInitialized);
-    DCHECK_NOT_NULL(factory);
     VariableProxy* proxy = factory->NewVariableProxy(var);
     VariableDeclaration* declaration =
         factory->NewVariableDeclaration(proxy, mode, this, kNoSourcePosition);
-    DCHECK_EQ(factory->zone(), zone());
     DeclareFunctionVar(declaration);
     var->AllocateTo(VariableLocation::CONTEXT, index);
     return var;
@@ -892,41 +889,6 @@ Handle<StringSet> Scope::CollectNonLocals(Handle<StringSet> non_locals) {
   return non_locals;
 }
 
-void Scope::AnalyzePartially(Scope* migrate_to,
-                             AstNodeFactory* ast_node_factory) {
-  // Gather info from inner scopes.
-  PropagateScopeInfo(false);
-
-  // Try to resolve unresolved variables for this Scope and collect those which
-  // cannot be resolved inside. It doesn't make sense to try to resolve them in
-  // the outer Scopes here, because they are incomplete.
-  VariableProxy* still_unresolved = nullptr;
-  CollectUnresolvableLocals(&still_unresolved, this);
-
-  // Re-create the VariableProxies in the right Zone and insert them into
-  // migrate_to.
-  for (VariableProxy* proxy = still_unresolved; proxy != nullptr;
-       proxy = proxy->next_unresolved()) {
-    // Recreate the VariableProxy.
-    DCHECK(!proxy->is_resolved());
-    VariableProxy* copy = ast_node_factory->CopyVariableProxy(proxy);
-    migrate_to->AddUnresolved(copy);
-  }
-
-  // Push scope data up to migrate_to. Note that migrate_to and this Scope
-  // describe the same Scope, just in different Zones.
-  PropagateUsageFlagsToScope(migrate_to);
-  if (inner_scope_calls_eval_) {
-    migrate_to->inner_scope_calls_eval_ = true;
-  }
-  DCHECK(!force_eager_compilation_);
-  migrate_to->set_start_position(start_position_);
-  migrate_to->set_end_position(end_position_);
-  migrate_to->language_mode_ = language_mode_;
-  outer_scope_->RemoveInnerScope(this);
-  DCHECK_EQ(outer_scope_, migrate_to->outer_scope_);
-  DCHECK_EQ(outer_scope_->zone(), migrate_to->zone());
-}
 
 #ifdef DEBUG
 static const char* Header(ScopeType scope_type, FunctionKind function_kind,
@@ -1135,12 +1097,6 @@ void Scope::CheckScopePositions() {
     scope->CheckScopePositions();
   }
 }
-
-void Scope::CheckZones() {
-  for (Scope* scope = inner_scope_; scope != nullptr; scope = scope->sibling_) {
-    CHECK_EQ(scope->zone(), zone());
-  }
-}
 #endif  // DEBUG
 
 
@@ -1163,10 +1119,10 @@ Variable* Scope::NonLocal(const AstRawString* name, VariableMode mode) {
   return var;
 }
 
+
 Variable* Scope::LookupRecursive(VariableProxy* proxy,
                                  BindingKind* binding_kind,
-                                 AstNodeFactory* factory,
-                                 Scope* max_outer_scope) {
+                                 AstNodeFactory* factory) {
   DCHECK(binding_kind != NULL);
   if (already_resolved() && is_with_scope()) {
     // Short-cut: if the scope is deserialized from a scope info, variable
@@ -1193,14 +1149,13 @@ Variable* Scope::LookupRecursive(VariableProxy* proxy,
   var = LookupFunctionVar(proxy->raw_name(), factory);
   if (var != NULL) {
     *binding_kind = BOUND;
-  } else if (outer_scope_ != nullptr && this != max_outer_scope) {
-    var = outer_scope_->LookupRecursive(proxy, binding_kind, factory,
-                                        max_outer_scope);
+  } else if (outer_scope_ != NULL) {
+    var = outer_scope_->LookupRecursive(proxy, binding_kind, factory);
     if (*binding_kind == BOUND && (is_function_scope() || is_with_scope())) {
       var->ForceContextAllocation();
     }
   } else {
-    DCHECK(is_script_scope() || this == max_outer_scope);
+    DCHECK(is_script_scope());
   }
 
   // "this" can't be shadowed by "eval"-introduced bindings or by "with" scopes.
@@ -1333,26 +1288,6 @@ bool Scope::ResolveVariablesRecursively(ParseInfo* info,
   return true;
 }
 
-void Scope::CollectUnresolvableLocals(VariableProxy** still_unresolved,
-                                      Scope* max_outer_scope) {
-  BindingKind binding_kind;
-  for (VariableProxy *proxy = unresolved_, *next = nullptr; proxy != nullptr;
-       proxy = next) {
-    next = proxy->next_unresolved();
-    // Note that we pass nullptr as AstNodeFactory: this phase should not create
-    // any new AstNodes, since none of the Scopes involved are backed up by
-    // ScopeInfo.
-    if (LookupRecursive(proxy, &binding_kind, nullptr, max_outer_scope) ==
-        nullptr) {
-      proxy->set_next_unresolved(*still_unresolved);
-      *still_unresolved = proxy;
-    }
-  }
-
-  for (Scope* scope = inner_scope_; scope != nullptr; scope = scope->sibling_) {
-    scope->CollectUnresolvableLocals(still_unresolved, max_outer_scope);
-  }
-}
 
 void Scope::PropagateScopeInfo(bool outer_scope_calls_sloppy_eval ) {
   if (outer_scope_calls_sloppy_eval) {

src/ast/scopes.h

@@ -201,7 +201,6 @@ class Scope: public ZoneObject {
     // the same name because they may be removed selectively via
     // RemoveUnresolved().
     DCHECK(!already_resolved());
-    DCHECK_EQ(factory->zone(), zone());
     VariableProxy* proxy =
         factory->NewVariableProxy(name, kind, start_position, end_position);
     proxy->set_next_unresolved(unresolved_);
@@ -610,13 +609,6 @@ class Scope: public ZoneObject {
     return &sloppy_block_function_map_;
   }
 
-  // To be called during parsing. Do just enough scope analysis that we can
-  // discard the Scope for lazily compiled functions. In particular, this
-  // records variables which cannot be resolved inside the Scope (we don't yet
-  // know what they will resolve to since the outer Scopes are incomplete) and
-  // migrates them into migrate_to.
-  void AnalyzePartially(Scope* migrate_to, AstNodeFactory* ast_node_factory);
-
   // ---------------------------------------------------------------------------
   // Debugging.
 
@@ -625,9 +617,6 @@ class Scope: public ZoneObject {
 
   // Check that the scope has positions assigned.
   void CheckScopePositions();
-
-  // Check that all Scopes in the scope tree use the same Zone.
-  void CheckZones();
 #endif
 
   // ---------------------------------------------------------------------------
@@ -788,23 +777,16 @@ class Scope: public ZoneObject {
   };
 
   // Lookup a variable reference given by name recursively starting with this
-  // scope, but only until max_outer_scope (if not nullptr). If the code is
-  // executed because of a call to 'eval', the context parameter should be set
-  // to the calling context of 'eval'.
+  // scope. If the code is executed because of a call to 'eval', the context
+  // parameter should be set to the calling context of 'eval'.
   Variable* LookupRecursive(VariableProxy* proxy, BindingKind* binding_kind,
-                            AstNodeFactory* factory,
-                            Scope* max_outer_scope = nullptr);
+                            AstNodeFactory* factory);
   MUST_USE_RESULT
   bool ResolveVariable(ParseInfo* info, VariableProxy* proxy,
                        AstNodeFactory* factory);
   MUST_USE_RESULT
   bool ResolveVariablesRecursively(ParseInfo* info, AstNodeFactory* factory);
 
-  // Tries to resolve local variables inside max_outer_scope; collects those
-  // which cannot be resolved.
-  void CollectUnresolvableLocals(VariableProxy** still_unresolved,
-                                 Scope* max_outer_scope);
-
   // Scope analysis.
   void PropagateScopeInfo(bool outer_scope_calls_sloppy_eval);
   bool HasTrivialContext() const;

src/parsing/parser-base.h

@@ -464,11 +464,7 @@ class ParserBase : public Traits {
       return &non_patterns_to_rewrite_;
     }
 
-    bool next_function_is_parenthesized() const {
-      return next_function_is_parenthesized_;
-    }
-
-    void set_next_function_is_parenthesized(bool parenthesized) {
+    void next_function_is_parenthesized(bool parenthesized) {
       next_function_is_parenthesized_ = parenthesized;
     }
 
@@ -1241,8 +1237,6 @@ class ParserBase : public Traits {
   bool allow_harmony_async_await_;
   bool allow_harmony_restrictive_generators_;
   bool allow_harmony_trailing_commas_;
-
-  friend class DiscardableZoneScope;
 };
 
 template <class Traits>
@@ -1636,8 +1630,8 @@ ParserBase<Traits>::ParsePrimaryExpression(ExpressionClassifier* classifier,
       }
       // Heuristically try to detect immediately called functions before
       // seeing the call parentheses.
-      function_state_->set_next_function_is_parenthesized(peek() ==
-                                                          Token::FUNCTION);
+      function_state_->next_function_is_parenthesized(peek() ==
+                                                      Token::FUNCTION);
       ExpressionT expr = this->ParseExpression(true, classifier, CHECK_OK);
       Expect(Token::RPAREN, CHECK_OK);
       return expr;

src/parsing/parser.h

@@ -724,6 +724,7 @@ class Parser : public ParserBase<ParserTraits> {
 
  private:
   friend class ParserTraits;
+  friend class DiscardableZoneScope;
 
   // Runtime encoding of different completion modes.
   enum CompletionKind {

test/mjsunit/context-variable-assignments.js

@@ -35,19 +35,3 @@ function foo() {
 }
 
 assertEquals("hello world", foo());
-
-// Also test that it works from more deeply nested inner functions:
-
-var v = (function foo2() {
-  var a, b;
-  var bar = function() {
-    var baz = function() {
-      a = b = "bye world";
-    }
-    baz();
-  }
-  bar();
-  return a;
-})();
-
-assertEquals("bye world", v);