[scopes] Push unresolved variables at the back so we can MoveTail to rescope

Pushing unresolved variables at the front was an optimization for the case
where we didn't have an end pointer. That forces us to do an O(<new elements>)
walk to rescope variables. The implementation was more generic and even did
O(<all elements>). Now that we have an end pointer we can simply push at the
end and MoveTail which is O(1).

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

src/ast/ast.h

@@ -1639,6 +1639,13 @@ class VariableProxy final : public Expression {
   void BindTo(Variable* var);
 
   V8_INLINE VariableProxy* next_unresolved() { return next_unresolved_; }
+  V8_INLINE bool is_removed_from_unresolved() const {
+    return IsRemovedFromUnresolvedField::decode(bit_field_);
+  }
+
+  void mark_removed_from_unresolved() {
+    bit_field_ = IsRemovedFromUnresolvedField::update(bit_field_, true);
+  }
 
   // Provides an access type for the ThreadedList used by the PreParsers
   // expressions, lists, and formal parameters.
@@ -1646,6 +1653,25 @@ class VariableProxy final : public Expression {
     static VariableProxy** next(VariableProxy* t) {
       return t->pre_parser_expr_next();
     }
+
+    static VariableProxy** start(VariableProxy** head) { return head; }
+  };
+
+  // Provides an access type for the ThreadedList used by the PreParsers
+  // expressions, lists, and formal parameters.
+  struct UnresolvedNext {
+    static VariableProxy** filter(VariableProxy** t) {
+      VariableProxy** n = t;
+      // Skip over possibly removed values.
+      while (*n != nullptr && (*n)->is_removed_from_unresolved()) {
+        n = (*n)->next();
+      }
+      return n;
+    }
+
+    static VariableProxy** start(VariableProxy** head) { return filter(head); }
+
+    static VariableProxy** next(VariableProxy* t) { return filter(t->next()); }
   };
 
  private:
@@ -1662,6 +1688,7 @@ class VariableProxy final : public Expression {
     bit_field_ |= IsThisField::encode(variable_kind == THIS_VARIABLE) |
                   IsAssignedField::encode(false) |
                   IsResolvedField::encode(false) |
+                  IsRemovedFromUnresolvedField::encode(false) |
                   HoleCheckModeField::encode(HoleCheckMode::kElided) |
                   IsPrivateName::encode(false);
   }
@@ -1672,7 +1699,10 @@ class VariableProxy final : public Expression {
   };
   class IsAssignedField : public BitField<bool, IsThisField::kNext, 1> {};
   class IsResolvedField : public BitField<bool, IsAssignedField::kNext, 1> {};
-  class IsNewTargetField : public BitField<bool, IsResolvedField::kNext, 1> {};
+  class IsRemovedFromUnresolvedField
+      : public BitField<bool, IsResolvedField::kNext, 1> {};
+  class IsNewTargetField
+      : public BitField<bool, IsRemovedFromUnresolvedField::kNext, 1> {};
   class HoleCheckModeField
       : public BitField<HoleCheckMode, IsNewTargetField::kNext, 1> {};
   class IsPrivateName : public BitField<bool, HoleCheckModeField::kNext, 1> {};

src/ast/scopes.cc

@@ -873,22 +873,8 @@ void Scope::Snapshot::Reparent(DeclarationScope* new_parent) const {
   }
 
   Scope* outer_scope_ = outer_scope_and_calls_eval_.GetPointer();
-  if (outer_scope_->unresolved_list_.first() != top_unresolved_) {
-    // If the marked VariableProxy (snapshoted) is not the first, we need to
-    // find it and move all VariableProxys up to that point into the new_parent,
-    // then we restore the snapshoted state by reinitializing the outer_scope
-    // list.
-    {
-      auto iter = outer_scope_->unresolved_list_.begin();
-      while (*iter != top_unresolved_) {
-        ++iter;
-      }
-      outer_scope_->unresolved_list_.Rewind(iter);
-    }
-
-    new_parent->unresolved_list_ = std::move(outer_scope_->unresolved_list_);
-    outer_scope_->unresolved_list_.ReinitializeHead(top_unresolved_);
-  }
+  new_parent->unresolved_list_.MoveTail(&outer_scope_->unresolved_list_,
+                                        top_unresolved_);
 
   // Move temporaries allocated for complex parameter initializers.
   DeclarationScope* outer_closure = outer_scope_->GetClosureScope();
@@ -1183,7 +1169,7 @@ void Scope::DeclareCatchVariableName(const AstRawString* name) {
 void Scope::AddUnresolved(VariableProxy* proxy) {
   DCHECK(!already_resolved_);
   DCHECK(!proxy->is_resolved());
-  unresolved_list_.AddFront(proxy);
+  unresolved_list_.Add(proxy);
 }
 
 Variable* DeclarationScope::DeclareDynamicGlobal(const AstRawString* name,
@@ -1199,6 +1185,11 @@ bool Scope::RemoveUnresolved(VariableProxy* var) {
   return unresolved_list_.Remove(var);
 }
 
+void Scope::DeleteUnresolved(VariableProxy* var) {
+  DCHECK(unresolved_list_.Contains(var));
+  var->mark_removed_from_unresolved();
+}
+
 Variable* Scope::NewTemporary(const AstRawString* name) {
   return NewTemporary(name, kMaybeAssigned);
 }
@@ -1403,8 +1394,7 @@ void Scope::CollectNonLocals(DeclarationScope* max_outer_scope,
           ? outer_scope()
           : this;
 
-  for (VariableProxy* proxy = unresolved_list_.first(); proxy != nullptr;
-       proxy = proxy->next_unresolved()) {
+  for (VariableProxy* proxy : unresolved_list_) {
     DCHECK(!proxy->is_resolved());
     Variable* var =
         Lookup<kParsedScope>(proxy, lookup, max_outer_scope->outer_scope());
@@ -1427,9 +1417,9 @@ void Scope::CollectNonLocals(DeclarationScope* max_outer_scope,
   }
 }
 
-void Scope::AnalyzePartially(
-    DeclarationScope* max_outer_scope, AstNodeFactory* ast_node_factory,
-    base::ThreadedList<VariableProxy>* new_unresolved_list) {
+void Scope::AnalyzePartially(DeclarationScope* max_outer_scope,
+                             AstNodeFactory* ast_node_factory,
+                             UnresolvedList* new_unresolved_list) {
   DCHECK_IMPLIES(is_declaration_scope(),
                  !AsDeclarationScope()->was_lazily_parsed());
 
@@ -1445,7 +1435,7 @@ void Scope::AnalyzePartially(
       if (!max_outer_scope->outer_scope()->is_script_scope() ||
           proxy->is_private_name()) {
         VariableProxy* copy = ast_node_factory->CopyVariableProxy(proxy);
-        new_unresolved_list->AddFront(copy);
+        new_unresolved_list->Add(copy);
       }
     } else if (var != Scope::kDummyPreParserVariable &&
                var != Scope::kDummyPreParserLexicalVariable) {
@@ -1530,7 +1520,7 @@ void DeclarationScope::SavePreParsedScopeDataForDeclarationScope() {
 
 void DeclarationScope::AnalyzePartially(AstNodeFactory* ast_node_factory) {
   DCHECK(!force_eager_compilation_);
-  base::ThreadedList<VariableProxy> new_unresolved_list;
+  UnresolvedList new_unresolved_list;
   if (!IsArrowFunction(function_kind_) &&
       (!outer_scope_->is_script_scope() ||
        (preparsed_scope_data_builder_ != nullptr &&

src/ast/scopes.h

@@ -112,6 +112,9 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
   }
 #endif
 
+  typedef base::ThreadedList<VariableProxy, VariableProxy::UnresolvedNext>
+      UnresolvedList;
+
   // TODO(verwaest): Is this needed on Scope?
   int num_parameters() const;
 
@@ -135,7 +138,7 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
    private:
     PointerWithPayload<Scope, bool, 1> outer_scope_and_calls_eval_;
     Scope* top_inner_scope_;
-    VariableProxy* top_unresolved_;
+    UnresolvedList::Iterator top_unresolved_;
     base::ThreadedList<Variable>::Iterator top_local_;
   };
 
@@ -226,14 +229,20 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
 
   void AddUnresolved(VariableProxy* proxy);
 
-  // Remove a unresolved variable. During parsing, an unresolved variable
-  // may have been added optimistically, but then only the variable name
-  // was used (typically for labels). If the variable was not declared, the
-  // addition introduced a new unresolved variable which may end up being
-  // allocated globally as a "ghost" variable. RemoveUnresolved removes
-  // such a variable again if it was added; otherwise this is a no-op.
+  // Removes an unresolved variable from the list so it can be readded to
+  // another list. This is used to reparent parameter initializers that contain
+  // sloppy eval.
   bool RemoveUnresolved(VariableProxy* var);
 
+  // Deletes an unresolved variable. The variable proxy cannot be reused for
+  // another list later. During parsing, an unresolved variable may have been
+  // added optimistically, but then only the variable name was used (typically
+  // for labels and arrow function parameters). If the variable was not
+  // declared, the addition introduced a new unresolved variable which may end
+  // up being allocated globally as a "ghost" variable. DeleteUnresolved removes
+  // such a variable again if it was added; otherwise this is a no-op.
+  void DeleteUnresolved(VariableProxy* var);
+
   // Creates a new temporary variable in this scope's TemporaryScope.  The
   // name is only used for printing and cannot be used to find the variable.
   // In particular, the only way to get hold of the temporary is by keeping the
@@ -546,7 +555,7 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
   base::ThreadedList<Variable> locals_;
   // Unresolved variables referred to from this scope. The proxies themselves
   // form a linked list of all unresolved proxies.
-  base::ThreadedList<VariableProxy> unresolved_list_;
+  UnresolvedList unresolved_list_;
   // Declarations.
   base::ThreadedList<Declaration> decls_;
 
@@ -632,7 +641,7 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
   // list along the way, so full resolution cannot be done afterwards.
   void AnalyzePartially(DeclarationScope* max_outer_scope,
                         AstNodeFactory* ast_node_factory,
-                        base::ThreadedList<VariableProxy>* new_unresolved_list);
+                        UnresolvedList* new_unresolved_list);
   void CollectNonLocals(DeclarationScope* max_outer_scope, Isolate* isolate,
                         ParseInfo* info, Handle<StringSet>* non_locals);
 
@@ -1070,7 +1079,7 @@ class V8_EXPORT_PRIVATE DeclarationScope : public Scope {
 Scope::Snapshot::Snapshot(Scope* scope)
     : outer_scope_and_calls_eval_(scope, scope->scope_calls_eval_),
       top_inner_scope_(scope->inner_scope_),
-      top_unresolved_(scope->unresolved_list_.first()),
+      top_unresolved_(scope->unresolved_list_.end()),
       top_local_(scope->GetClosureScope()->locals_.end()) {
   // Reset in order to record eval calls during this Snapshot's lifetime.
   outer_scope_and_calls_eval_.GetPointer()->scope_calls_eval_ = false;

src/base/threaded-list.h

@@ -16,6 +16,8 @@ namespace base {
 template <typename T>
 struct ThreadedListTraits {
   static T** next(T* t) { return t->next(); }
+  static T** start(T** t) { return t; }
+  static T* const* start(T* const* t) { return t; }
 };
 
 // Represents a linked list that threads through the nodes in the linked list.
@@ -44,21 +46,6 @@ class ThreadedListBase final : public BaseClass {
     head_ = v;
   }
 
-  // Reinitializing the head to a new node, this costs O(n).
-  void ReinitializeHead(T* v) {
-    head_ = v;
-    T* current = v;
-    if (current != nullptr) {  // Find tail
-      T* tmp;
-      while ((tmp = *TLTraits::next(current))) {
-        current = tmp;
-      }
-      tail_ = TLTraits::next(current);
-    } else {
-      tail_ = &head_;
-    }
-  }
-
   void DropHead() {
     DCHECK_NOT_NULL(head_);
 
@@ -68,6 +55,13 @@ class ThreadedListBase final : public BaseClass {
     *TLTraits::next(old_head) = nullptr;
   }
 
+  bool Contains(T* v) {
+    for (Iterator it = begin(); it != end(); ++it) {
+      if (*it == v) return true;
+    }
+    return false;
+  }
+
   void Append(ThreadedListBase&& list) {
     if (list.is_empty()) return;
 
@@ -152,7 +146,7 @@ class ThreadedListBase final : public BaseClass {
     bool operator!=(const Iterator& other) const {
       return entry_ != other.entry_;
     }
-    T* operator*() { return *entry_; }
+    T*& operator*() { return *entry_; }
     T* operator->() { return *entry_; }
     Iterator& operator=(T* entry) {
       T* next = *TLTraits::next(*entry_);
@@ -198,10 +192,10 @@ class ThreadedListBase final : public BaseClass {
     friend class ThreadedListBase;
   };
 
-  Iterator begin() { return Iterator(&head_); }
+  Iterator begin() { return Iterator(TLTraits::start(&head_)); }
   Iterator end() { return Iterator(tail_); }
 
-  ConstIterator begin() const { return ConstIterator(&head_); }
+  ConstIterator begin() const { return ConstIterator(TLTraits::start(&head_)); }
   ConstIterator end() const { return ConstIterator(tail_); }
 
   // Rewinds the list's tail to the reset point, i.e., cutting of the rest of

src/parsing/parser.cc

@@ -1484,7 +1484,7 @@ void Parser::DeclareLabel(ZonePtrList<const AstRawString>** labels,
   // Remove the "ghost" variable that turned out to be a label
   // from the top scope. This way, we don't try to resolve it
   // during the scope processing.
-  scope()->RemoveUnresolved(var);
+  scope()->DeleteUnresolved(var);
 }
 
 bool Parser::ContainsLabel(ZonePtrList<const AstRawString>* labels,

src/parsing/pattern-rewriter.cc

@@ -200,15 +200,12 @@ void PatternRewriter::VisitVariableProxy(VariableProxy* pattern) {
   DCHECK_NOT_NULL(descriptor_);
 
   Scope* outer_function_scope = nullptr;
-  bool success;
   if (declares_parameter_containing_sloppy_eval_) {
     outer_function_scope = scope()->outer_scope();
-    success = outer_function_scope->RemoveUnresolved(pattern);
+    outer_function_scope->DeleteUnresolved(pattern);
   } else {
-    success = scope()->RemoveUnresolved(pattern);
+    scope()->DeleteUnresolved(pattern);
   }
-  USE(success);
-  DCHECK(success);
 
   // Declare variable.
   // Note that we *always* must treat the initial value via a separate init

src/parsing/preparser.cc

@@ -415,7 +415,7 @@ void PreParser::DeclareAndInitializeVariables(
     ZonePtrList<const AstRawString>* names) {
   if (declaration->pattern.variables_ != nullptr) {
     for (auto variable : *(declaration->pattern.variables_)) {
-      declaration_descriptor->scope->RemoveUnresolved(variable);
+      declaration_descriptor->scope->DeleteUnresolved(variable);
       Variable* var = scope()->DeclareVariableName(
           variable->raw_name(), declaration_descriptor->mode);
       MarkLoopVariableAsAssigned(declaration_descriptor->scope, var,

test/unittests/base/threaded-list-unittest.cc

@@ -20,6 +20,10 @@ struct ThreadedListTestNode {
   ThreadedListTestNode* next_;
 
   struct OtherTraits {
+    static ThreadedListTestNode** start(ThreadedListTestNode** h) { return h; }
+    static ThreadedListTestNode* const* start(ThreadedListTestNode* const* h) {
+      return h;
+    }
     static ThreadedListTestNode** next(ThreadedListTestNode* t) {
       return t->other_next();
     }
@@ -134,16 +138,6 @@ TEST_F(ThreadedListTest, AddFront) {
   CHECK_EQ(list.first(), &new_node);
 }
 
-TEST_F(ThreadedListTest, ReinitializeHead) {
-  CHECK_EQ(list.LengthForTest(), 5);
-  CHECK_NE(extra_test_list.first(), list.first());
-  list.ReinitializeHead(&extra_test_node_0);
-  list.Verify();
-  CHECK_EQ(extra_test_list.first(), list.first());
-  CHECK_EQ(extra_test_list.end(), list.end());
-  CHECK_EQ(extra_test_list.LengthForTest(), 3);
-}
-
 TEST_F(ThreadedListTest, DropHead) {
   CHECK_EQ(extra_test_list.LengthForTest(), 3);
   CHECK_EQ(extra_test_list.first(), &extra_test_node_0);