Turn Scope::locals_ into a ThreadedList

This turns the ZoneList with minimum 6 words overhead into a linked list through variables, using 2 words for the empty list. Additionally the average number of pointers per entry goes down to the optimal 1 per variable that's in a list.

This does introduce 1 pointer unnecessary overhead for dynamic variables. If that becomes a problem we could distinguish between variables in lists and variables not in lists. We can distinguish them at construction-time.

BUG=v8:5209

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

src/ast/scopeinfo.cc

@@ -58,7 +58,6 @@ bool ScopeInfo::Equals(ScopeInfo* other) const {
 Handle<ScopeInfo> ScopeInfo::Create(Isolate* isolate, Zone* zone, Scope* scope,
                                     MaybeHandle<ScopeInfo> outer_scope) {
   // Collect variables.
-  ZoneList<Variable*>* locals = scope->locals();
   int stack_local_count = 0;
   int context_local_count = 0;
   int module_vars_count = 0;
@@ -67,8 +66,7 @@ Handle<ScopeInfo> ScopeInfo::Create(Isolate* isolate, Zone* zone, Scope* scope,
   // slot index indicates at which offset a particular scope starts in the
   // parent declaration scope.
   int first_slot_index = 0;
-  for (int i = 0; i < locals->length(); i++) {
-    Variable* var = locals->at(i);
+  for (Variable* var : *scope->locals()) {
     switch (var->location()) {
       case VariableLocation::LOCAL:
         if (stack_local_count == 0) first_slot_index = var->index();
@@ -198,8 +196,7 @@ Handle<ScopeInfo> ScopeInfo::Create(Isolate* isolate, Zone* zone, Scope* scope,
   int context_local_info_base = context_local_base + context_local_count;
   int module_var_entry = scope_info->ModuleVariablesIndex();
 
-  for (int i = 0; i < locals->length(); ++i) {
-    Variable* var = locals->at(i);
+  for (Variable* var : *scope->locals()) {
     switch (var->location()) {
       case VariableLocation::LOCAL: {
         int local_index = var->index() - first_slot_index;

src/ast/scopes.cc

@@ -96,7 +96,6 @@ Scope::Scope(Zone* zone)
     : zone_(zone),
       outer_scope_(nullptr),
       variables_(zone),
-      locals_(4, zone),
       scope_type_(SCRIPT_SCOPE) {
   SetDefaults();
 }
@@ -105,7 +104,6 @@ Scope::Scope(Zone* zone, Scope* outer_scope, ScopeType scope_type)
     : zone_(zone),
       outer_scope_(outer_scope),
       variables_(zone),
-      locals_(4, zone),
       scope_type_(scope_type) {
   DCHECK_NE(SCRIPT_SCOPE, scope_type);
   SetDefaults();
@@ -119,7 +117,7 @@ Scope::Snapshot::Snapshot(Scope* scope)
     : outer_scope_(scope),
       top_inner_scope_(scope->inner_scope_),
       top_unresolved_(scope->unresolved_),
-      top_local_(scope->GetClosureScope()->locals_.length()),
+      top_local_(scope->GetClosureScope()->locals_.end()),
       top_decl_(scope->GetClosureScope()->decls_.end()) {}
 
 DeclarationScope::DeclarationScope(Zone* zone,
@@ -209,7 +207,6 @@ Scope::Scope(Zone* zone, ScopeType scope_type, Handle<ScopeInfo> scope_info)
     : zone_(zone),
       outer_scope_(nullptr),
       variables_(zone),
-      locals_(0, zone),
       scope_info_(scope_info),
       scope_type_(scope_type) {
   DCHECK(!scope_info.is_null());
@@ -238,7 +235,6 @@ Scope::Scope(Zone* zone, const AstRawString* catch_variable_name,
     : zone_(zone),
       outer_scope_(nullptr),
       variables_(zone),
-      locals_(0, zone),
       scope_info_(scope_info),
       scope_type_(CATCH_SCOPE) {
   SetDefaults();
@@ -684,13 +680,32 @@ Scope* Scope::FinalizeBlockScope() {
   return NULL;
 }
 
+void DeclarationScope::AddLocal(Variable* var) {
+  DCHECK(!already_resolved_);
+  // Temporaries are only placed in ClosureScopes.
+  DCHECK_EQ(GetClosureScope(), this);
+  locals_.Add(var);
+}
+
+Variable* Scope::Declare(Zone* zone, Scope* scope, const AstRawString* name,
+                         VariableMode mode, VariableKind kind,
+                         InitializationFlag initialization_flag,
+                         MaybeAssignedFlag maybe_assigned_flag) {
+  bool added;
+  Variable* var =
+      variables_.Declare(zone, scope, name, mode, kind, initialization_flag,
+                         maybe_assigned_flag, &added);
+  if (added) locals_.Add(var);
+  return var;
+}
+
 void Scope::Snapshot::Reparent(DeclarationScope* new_parent) const {
   DCHECK_EQ(new_parent, outer_scope_->inner_scope_);
   DCHECK_EQ(new_parent->outer_scope_, outer_scope_);
   DCHECK_EQ(new_parent, new_parent->GetClosureScope());
   DCHECK_NULL(new_parent->inner_scope_);
   DCHECK_NULL(new_parent->unresolved_);
-  DCHECK_EQ(0, new_parent->locals_.length());
+  DCHECK(new_parent->locals_.is_empty());
   Scope* inner_scope = new_parent->sibling_;
   if (inner_scope != top_inner_scope_) {
     for (; inner_scope->sibling() != top_inner_scope_;
@@ -722,13 +737,13 @@ void Scope::Snapshot::Reparent(DeclarationScope* new_parent) const {
   // name in the closure-scope. See
   // test/mjsunit/harmony/default-parameter-do-expression.js.
   DeclarationScope* outer_closure = outer_scope_->GetClosureScope();
-  for (int i = top_local_; i < outer_closure->locals_.length(); i++) {
-    Variable* local = outer_closure->locals_.at(i);
+
+  new_parent->locals_.MoveTail(outer_closure->locals(), top_local_);
+  for (Variable* local : new_parent->locals_) {
     DCHECK(local->mode() == TEMPORARY || local->mode() == VAR);
     DCHECK_EQ(local->scope(), local->scope()->GetClosureScope());
     DCHECK_NE(local->scope(), new_parent);
     local->set_scope(new_parent);
-    new_parent->AddLocal(local);
     if (local->mode() == VAR) {
       outer_closure->variables_.Remove(local);
       new_parent->variables_.Add(new_parent->zone(), local);
@@ -1238,7 +1253,7 @@ void DeclarationScope::ResetAfterPreparsing(AstValueFactory* ast_value_factory,
 
   // Reset all non-trivial members.
   decls_.Clear();
-  locals_.Rewind(0);
+  locals_.Clear();
   sloppy_block_function_map_.Clear();
   variables_.Clear();
   // Make sure we won't walk the scope tree from here on.
@@ -1255,10 +1270,14 @@ void DeclarationScope::ResetAfterPreparsing(AstValueFactory* ast_value_factory,
     for (int i = 0; i < params_.length(); i++) {
       Variable* var = params_[i];
       if (var->mode() == TEMPORARY) {
-        locals_.Add(var, zone());
+        // TODO(verwaest): Remove and unfriend DeclarationScope from Variable.
+        *var->next() = nullptr;
+        locals_.Add(var);
       } else if (variables_.Lookup(var->raw_name()) == nullptr) {
+        // TODO(verwaest): Remove and unfriend DeclarationScope from Variable.
+        *var->next() = nullptr;
         variables_.Add(zone(), var);
-        locals_.Add(var, zone());
+        locals_.Add(var);
       }
     }
   } else {
@@ -1884,8 +1903,8 @@ void Scope::AllocateNonParameterLocal(Variable* var) {
 }
 
 void Scope::AllocateNonParameterLocalsAndDeclaredGlobals() {
-  for (int i = 0; i < locals_.length(); i++) {
-    AllocateNonParameterLocal(locals_[i]);
+  for (Variable* local : locals_) {
+    AllocateNonParameterLocal(local);
   }
 
   if (is_declaration_scope()) {

src/ast/scopes.h

@@ -96,7 +96,7 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
     Scope* outer_scope_;
     Scope* top_inner_scope_;
     VariableProxy* top_unresolved_;
-    int top_local_;
+    ThreadedList<Variable>::Iterator top_local_;
     ThreadedList<Declaration>::Iterator top_decl_;
   };
 
@@ -155,7 +155,7 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
   // Declarations list.
   ThreadedList<Declaration>* declarations() { return &decls_; }
 
-  ZoneList<Variable*>* locals() { return &locals_; }
+  ThreadedList<Variable>* locals() { return &locals_; }
 
   // Create a new unresolved variable.
   VariableProxy* NewUnresolved(AstNodeFactory* factory,
@@ -429,14 +429,7 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
   Variable* Declare(Zone* zone, Scope* scope, const AstRawString* name,
                     VariableMode mode, VariableKind kind,
                     InitializationFlag initialization_flag,
-                    MaybeAssignedFlag maybe_assigned_flag = kNotAssigned) {
-    bool added;
-    Variable* var =
-        variables_.Declare(zone, scope, name, mode, kind, initialization_flag,
-                           maybe_assigned_flag, &added);
-    if (added) locals_.Add(var, zone);
-    return var;
-  }
+                    MaybeAssignedFlag maybe_assigned_flag = kNotAssigned);
 
   // This method should only be invoked on scopes created during parsing (i.e.,
   // not deserialized from a context). Also, since NeedsContext() is only
@@ -459,8 +452,7 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
   VariableMap variables_;
   // In case of non-scopeinfo-backed scopes, this contains the variables of the
   // map above in order of addition.
-  // TODO(verwaest): Thread through Variable.
-  ZoneList<Variable*> locals_;
+  ThreadedList<Variable> locals_;
   // Unresolved variables referred to from this scope. The proxies themselves
   // form a linked list of all unresolved proxies.
   VariableProxy* unresolved_;
@@ -737,12 +729,7 @@ class DeclarationScope : public Scope {
   // Adds a local variable in this scope's locals list. This is for adjusting
   // the scope of temporaries and do-expression vars when desugaring parameter
   // initializers.
-  void AddLocal(Variable* var) {
-    DCHECK(!already_resolved_);
-    // Temporaries are only placed in ClosureScopes.
-    DCHECK_EQ(GetClosureScope(), this);
-    locals_.Add(var, zone());
-  }
+  void AddLocal(Variable* var);
 
   void DeclareSloppyBlockFunction(const AstRawString* name,
                                   SloppyBlockFunctionStatement* statement) {

src/ast/variables.cc

@@ -19,6 +19,7 @@ Variable::Variable(Scope* scope, const AstRawString* name, VariableMode mode,
     : scope_(scope),
       name_(name),
       local_if_not_shadowed_(nullptr),
+      next_(nullptr),
       index_(-1),
       initializer_position_(kNoSourcePosition),
       bit_field_(MaybeAssignedFlagField::encode(maybe_assigned_flag) |

src/ast/variables.h

@@ -123,6 +123,8 @@ class Variable final : public ZoneObject {
     return mode == VAR ? kCreatedInitialized : kNeedsInitialization;
   }
 
+  typedef ThreadedList<Variable> List;
+
  private:
   Scope* scope_;
   const AstRawString* name_;
@@ -132,6 +134,7 @@ class Variable final : public ZoneObject {
   // sloppy 'eval' calls between the reference scope (inclusive) and the
   // binding scope (exclusive).
   Variable* local_if_not_shadowed_;
+  Variable* next_;
   int index_;
   int initializer_position_;
   uint16_t bit_field_;
@@ -150,6 +153,11 @@ class Variable final : public ZoneObject {
   class MaybeAssignedFlagField
       : public BitField16<MaybeAssignedFlag, InitializationFlagField::kNext,
                           2> {};
+  Variable** next() { return &next_; }
+  friend List;
+  // To reset next to nullptr upon resetting after preparsing.
+  // TODO(verwaest): Remove once we properly preparse parameters.
+  friend class DeclarationScope;
 };
 }  // namespace internal
 }  // namespace v8

src/crankshaft/typing.cc

@@ -85,10 +85,8 @@ void AstTyper::ObserveTypesAtOsrEntry(IterationStatement* stmt) {
                     store_.LookupBounds(parameter_index(i)).lower);
     }
 
-    ZoneList<Variable*>* local_vars = scope_->locals();
     int local_index = 0;
-    for (int i = 0; i < local_vars->length(); i++) {
-      Variable* var = local_vars->at(i);
+    for (Variable* var : *scope_->locals()) {
       if (var->IsStackLocal()) {
         PrintObserved(
             var, frame->GetExpression(local_index),

src/debug/liveedit.cc

@@ -1900,9 +1900,7 @@ Handle<Object> LiveEditFunctionTracker::SerializeFunctionScope(Scope* scope) {
   Scope* current_scope = scope;
   while (current_scope != NULL) {
     HandleScope handle_scope(isolate_);
-    ZoneList<Variable*>* locals = current_scope->locals();
-    for (int i = 0; i < locals->length(); i++) {
-      Variable* var = locals->at(i);
+    for (Variable* var : *current_scope->locals()) {
       if (!var->IsContextSlot()) continue;
       int context_index = var->index() - Context::MIN_CONTEXT_SLOTS;
       int location = scope_info_length + context_index * 2;

src/utils.h

@@ -1648,6 +1648,15 @@ class ThreadedList final {
     *tail_ = nullptr;
   }
 
+  void MoveTail(ThreadedList<T>* parent, Iterator location) {
+    if (parent->end() != location) {
+      DCHECK_NULL(*tail_);
+      *tail_ = *location;
+      tail_ = parent->tail_;
+      parent->Rewind(location);
+    }
+  }
+
   bool is_empty() const { return head_ == nullptr; }
 
   // Slow. For testing purposes.

test/mjsunit/regress/regress-abort-preparsing-params.js

@@ -4,7 +4,7 @@
 
 var outer_a;
 
-function f(a) {
+function f(a, b, a) {
   outer_a = a;
   x = 1;
   x = 1;
@@ -942,5 +942,5 @@ function f(a) {
   x = 1;
   x = 1;
 }
-f(1);
+f(1, 2, 1);
 assertEquals(1, outer_a);