[parser] Skipping inner funcs: collect data needed for allocation, not the allocation result.

This pretty much rewrites the preparsed scope data collection. We used to store
the allocation result, but it's faster to just store the raw data which is
needed for deciding it later. (This way we don't need to run the allocation
algorithm for just getting this data.)

For each variable: is_used, maybe_assigned,
has_forced_context_allocation, and for each scope:
inner_scope_calls_eval_.

In addition, this CL moves data handling out of Scope and into
PreParsedScopeData where it belongs and simplifies the API for
PreParsedScopeData.

BUG=v8:5516
R=vogelheim@chromium.org

Change-Id: Ia5a4fa52f585cd4f483ce9a92f2dd7d9754f34ed
Reviewed-on: https://chromium-review.googlesource.com/451273
Commit-Queue: Marja Hölttä <marja@chromium.org>
Reviewed-by: Daniel Vogelheim <vogelheim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#43641}

src/ast/scopes.cc

@@ -1523,10 +1523,9 @@ void DeclarationScope::AnalyzePartially(
     }
 
     if (FLAG_preparser_scope_analysis) {
-      // Decide context allocation for the locals and parameters and store the
-      // info away.
-      AllocateVariablesRecursively();
-      CollectVariableData(preparsed_scope_data);
+      // Store the information needed for allocating the locals of this scope
+      // and its inner scopes.
+      preparsed_scope_data->SaveData(this);
     }
   }
 #ifdef DEBUG
@@ -2304,17 +2303,6 @@ void Scope::AllocateDebuggerScopeInfos(Isolate* isolate,
   }
 }
 
-void Scope::CollectVariableData(PreParsedScopeData* data) {
-  PreParsedScopeData::ScopeScope scope_scope(data, scope_type(),
-                                             start_position(), end_position());
-  for (Variable* local : locals_) {
-    scope_scope.MaybeAddVariable(local);
-  }
-  for (Scope* inner = inner_scope_; inner != nullptr; inner = inner->sibling_) {
-    inner->CollectVariableData(data);
-  }
-}
-
 int Scope::StackLocalCount() const {
   Variable* function =
       is_function_scope() ? AsDeclarationScope()->function_var() : nullptr;

src/ast/scopes.h

@@ -594,8 +594,6 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
   void AllocateDebuggerScopeInfos(Isolate* isolate,
                                   MaybeHandle<ScopeInfo> outer_scope);
 
-  void CollectVariableData(PreParsedScopeData* data);
-
   // Construct a scope based on the scope info.
   Scope(Zone* zone, ScopeType type, Handle<ScopeInfo> scope_info);
 

src/parsing/preparsed-scope-data.cc

@@ -11,74 +11,107 @@
 namespace v8 {
 namespace internal {
 
-bool PreParsedScopeData::HasVariablesWhichNeedAllocationData(Scope* scope) {
+namespace {
+
+class VariableIsUsedField : public BitField16<bool, 0, 1> {};
+class VariableMaybeAssignedField
+    : public BitField16<bool, VariableIsUsedField::kNext, 1> {};
+class VariableContextAllocatedField
+    : public BitField16<bool, VariableMaybeAssignedField::kNext, 1> {};
+
+}  // namespace
+
+void PreParsedScopeData::SaveData(Scope* scope) {
+  size_t old_size = backing_store_.size();
+
   if (!scope->is_hidden()) {
     for (Variable* var : *scope->locals()) {
       if (var->mode() == VAR || var->mode() == LET || var->mode() == CONST) {
-        return true;
+        SaveDataForVariable(var);
       }
     }
   }
   for (Scope* inner = scope->inner_scope(); inner != nullptr;
        inner = inner->sibling()) {
-    if (HasVariablesWhichNeedAllocationData(inner)) {
-      return true;
-    }
+    SaveData(inner);
   }
-  return false;
-}
 
-PreParsedScopeData::ScopeScope::ScopeScope(PreParsedScopeData* data,
-                                           ScopeType scope_type,
-                                           int start_position, int end_position)
-    : data_(data), previous_scope_(data->current_scope_) {
-  data->current_scope_ = this;
-  data->backing_store_.push_back(scope_type);
-  data->backing_store_.push_back(start_position);
-  data->backing_store_.push_back(end_position);
-  // Reserve space for variable and inner scope count (we don't know yet how
-  // many will be added).
-  index_in_data_ = data->backing_store_.size();
-  data->backing_store_.push_back(-1);
-  data->backing_store_.push_back(-1);
+  if (old_size != backing_store_.size()) {
+#ifdef DEBUG
+    backing_store_.push_back(scope->scope_type());
+#endif
+
+    backing_store_.push_back(scope->inner_scope_calls_eval());
+  }
 }
 
-PreParsedScopeData::ScopeScope::~ScopeScope() {
-  data_->current_scope_ = previous_scope_;
-  if (got_data_) {
-    DCHECK_GT(variable_count_ + inner_scope_count_, 0);
-    if (previous_scope_ != nullptr) {
-      previous_scope_->got_data_ = true;
-      ++previous_scope_->inner_scope_count_;
+void PreParsedScopeData::RestoreData(Scope* scope, int* index_ptr) const {
+  int& index = *index_ptr;
+  int old_index = index;
+
+  if (!scope->is_hidden()) {
+    for (Variable* var : *scope->locals()) {
+      if (var->mode() == VAR || var->mode() == LET || var->mode() == CONST) {
+        RestoreDataForVariable(var, index_ptr);
+      }
     }
-    data_->backing_store_[index_in_data_] = inner_scope_count_;
-    data_->backing_store_[index_in_data_ + 1] = variable_count_;
-  } else {
-    // No interesting data for this scope (or its children); remove from the
-    // data.
-    DCHECK_EQ(data_->backing_store_.size(), index_in_data_ + 2);
-    DCHECK_GE(index_in_data_, 3);
-    DCHECK_EQ(variable_count_, 0);
-    data_->backing_store_.erase(
-        data_->backing_store_.begin() + index_in_data_ - 3,
-        data_->backing_store_.end());
   }
-}
+  for (Scope* inner = scope->inner_scope(); inner != nullptr;
+       inner = inner->sibling()) {
+    RestoreData(inner, index_ptr);
+  }
+
+  if (index != old_index) {
+// Some data was read, i.e., there's data for the Scope.
 
-void PreParsedScopeData::ScopeScope::MaybeAddVariable(Variable* var) {
-  if (var->mode() == VAR || var->mode() == LET || var->mode() == CONST) {
 #ifdef DEBUG
-    // For tests (which check that the data is about the same variables).
-    const AstRawString* name = var->raw_name();
-    data_->backing_store_.push_back(name->length());
-    for (int i = 0; i < name->length(); ++i) {
-      data_->backing_store_.push_back(name->raw_data()[i]);
+    DCHECK_EQ(backing_store_[index++], scope->scope_type());
+#endif
+
+    if (backing_store_[index++]) {
+      scope->RecordEvalCall();
     }
+  }
+}
+
+void PreParsedScopeData::SaveDataForVariable(Variable* var) {
+#ifdef DEBUG
+  // Store the variable name in debug mode; this way we can check that we
+  // restore data to the correct variable.
+  const AstRawString* name = var->raw_name();
+  backing_store_.push_back(name->length());
+  for (int i = 0; i < name->length(); ++i) {
+    backing_store_.push_back(name->raw_data()[i]);
+  }
 #endif
-    data_->backing_store_.push_back(var->location());
-    data_->backing_store_.push_back(var->maybe_assigned());
-    ++variable_count_;
-    got_data_ = true;
+  int variable_data = VariableIsUsedField::encode(var->is_used()) |
+                      VariableMaybeAssignedField::encode(
+                          var->maybe_assigned() == kMaybeAssigned) |
+                      VariableContextAllocatedField::encode(
+                          var->has_forced_context_allocation());
+
+  backing_store_.push_back(variable_data);
+}
+
+void PreParsedScopeData::RestoreDataForVariable(Variable* var,
+                                                int* index_ptr) const {
+  int& index = *index_ptr;
+#ifdef DEBUG
+  const AstRawString* name = var->raw_name();
+  DCHECK_EQ(backing_store_[index++], name->length());
+  for (int i = 0; i < name->length(); ++i) {
+    DCHECK_EQ(backing_store_[index++], name->raw_data()[i]);
+  }
+#endif
+  int variable_data = backing_store_[index++];
+  if (VariableIsUsedField::decode(variable_data)) {
+    var->set_is_used();
+  }
+  if (VariableMaybeAssignedField::decode(variable_data)) {
+    var->set_maybe_assigned();
+  }
+  if (VariableContextAllocatedField::decode(variable_data)) {
+    var->ForceContextAllocation();
   }
 }
 

src/parsing/preparsed-scope-data.h

@@ -17,36 +17,23 @@ class PreParsedScopeData {
   PreParsedScopeData() {}
   ~PreParsedScopeData() {}
 
-  // Whether the scope has variables whose context allocation or
-  // maybeassignedness we need to decide based on preparsed scope data.
-  static bool HasVariablesWhichNeedAllocationData(Scope* scope);
+  // Saves the information needed for allocating the Scope's (and its
+  // subscopes') variables.
+  void SaveData(Scope* scope);
 
-  class ScopeScope {
-   public:
-    ScopeScope(PreParsedScopeData* data, ScopeType scope_type,
-               int start_position, int end_position);
-    ~ScopeScope();
-
-    void MaybeAddVariable(Variable* var);
-
-   private:
-    PreParsedScopeData* data_;
-    size_t index_in_data_;
-    ScopeScope* previous_scope_;
-
-    int inner_scope_count_ = 0;
-    int variable_count_ = 0;
-    bool got_data_ = false;
-    DISALLOW_COPY_AND_ASSIGN(ScopeScope);
-  };
+  // Restores the information needed for allocating the Scopes's (and its
+  // subscopes') variables.
+  void RestoreData(Scope* scope, int* index_ptr) const;
 
  private:
   friend class ScopeTestHelper;
 
+  void SaveDataForVariable(Variable* var);
+  void RestoreDataForVariable(Variable* var, int* index_ptr) const;
+
   // TODO(marja): Make the backing store more efficient once we know exactly
   // what data is needed.
-  std::vector<int> backing_store_;
-  ScopeScope* current_scope_ = nullptr;
+  std::vector<byte> backing_store_;
 
   DISALLOW_COPY_AND_ASSIGN(PreParsedScopeData);
 };

src/utils.h

@@ -1640,6 +1640,7 @@ class ThreadedList final {
     }
     bool operator!=(const Iterator& other) { return entry_ != other.entry_; }
     T* operator*() { return *entry_; }
+    T* operator->() { return *entry_; }
     Iterator& operator=(T* entry) {
       T* next = *(*entry_)->next();
       *entry->next() = next;

test/cctest/parsing/test-preparser.cc

@@ -616,21 +616,43 @@ TEST(PreParserScopeAnalysis) {
       printf("\n");
 
       script = factory->NewScript(source);
-      i::ParseInfo eager_info(script);
-      eager_info.set_allow_lazy_parsing(false);
 
-      CHECK(i::parsing::ParseProgram(&eager_info));
-      CHECK(i::Compiler::Analyze(&eager_info));
+      // Compare the allocation of the variables in two cases: 1) normal scope
+      // allocation 2) allocation based on the preparse data.
 
-      i::Scope* scope =
-          eager_info.literal()->scope()->inner_scope()->inner_scope();
-      DCHECK_NOT_NULL(scope);
-      DCHECK_NULL(scope->sibling());
-      DCHECK(scope->is_function_scope());
+      i::ParseInfo eager_normal(script);
+      eager_normal.set_allow_lazy_parsing(false);
 
-      size_t index = 0;
-      i::ScopeTestHelper::CompareScopeToData(
-          scope, lazy_info.preparsed_scope_data(), index,
+      CHECK(i::parsing::ParseProgram(&eager_normal));
+      CHECK(i::Compiler::Analyze(&eager_normal));
+
+      i::Scope* normal_scope =
+          eager_normal.literal()->scope()->inner_scope()->inner_scope();
+      CHECK_NOT_NULL(normal_scope);
+      CHECK_NULL(normal_scope->sibling());
+      CHECK(normal_scope->is_function_scope());
+
+      i::ParseInfo eager_using_scope_data(script);
+      eager_using_scope_data.set_allow_lazy_parsing(false);
+
+      CHECK(i::parsing::ParseProgram(&eager_using_scope_data));
+      // Don't run scope analysis (that would obviously decide the correct
+      // allocation for the variables).
+
+      i::Scope* unallocated_scope = eager_using_scope_data.literal()
+                                        ->scope()
+                                        ->inner_scope()
+                                        ->inner_scope();
+      CHECK_NOT_NULL(unallocated_scope);
+      CHECK_NULL(unallocated_scope->sibling());
+      CHECK(unallocated_scope->is_function_scope());
+
+      int index = 0;
+      lazy_info.preparsed_scope_data()->RestoreData(unallocated_scope, &index);
+      i::ScopeTestHelper::AllocateWithoutVariableResolution(unallocated_scope);
+
+      i::ScopeTestHelper::CompareScopes(
+          normal_scope, unallocated_scope,
           inners[inner_ix].precise_maybe_assigned);
     }
   }

test/cctest/scope-test-helper.h

@@ -17,75 +17,46 @@ class ScopeTestHelper {
     return var->scope()->MustAllocateInContext(var);
   }
 
-  static void CompareScopeToData(Scope* scope, const PreParsedScopeData* data,
-                                 size_t& index, bool precise_maybe_assigned) {
-    CHECK(PreParsedScopeData::HasVariablesWhichNeedAllocationData(scope));
-    CHECK_GT(data->backing_store_.size(), index + 4);
-    CHECK_EQ(data->backing_store_[index++], scope->scope_type());
-    CHECK_EQ(data->backing_store_[index++], scope->start_position());
-    CHECK_EQ(data->backing_store_[index++], scope->end_position());
-
-    int inner_scope_count = 0;
-    for (Scope* inner = scope->inner_scope(); inner != nullptr;
-         inner = inner->sibling()) {
-      if (PreParsedScopeData::HasVariablesWhichNeedAllocationData(inner)) {
-        ++inner_scope_count;
-      }
-    }
-    CHECK_EQ(data->backing_store_[index++], inner_scope_count);
-
-    int variable_count = 0;
-    for (Variable* local : scope->locals_) {
-      if (local->mode() == VAR || local->mode() == LET ||
-          local->mode() == CONST) {
-        ++variable_count;
-      }
-    }
+  static void AllocateWithoutVariableResolution(Scope* scope) {
+    scope->AllocateVariablesRecursively();
+  }
 
-    CHECK_EQ(data->backing_store_[index++], variable_count);
+  static void CompareScopes(Scope* baseline, Scope* scope,
+                            bool precise_maybe_assigned) {
+    if (!scope->is_hidden()) {
+      for (auto baseline_local = baseline->locals()->begin(),
+                scope_local = scope->locals()->begin();
+           baseline_local != baseline->locals()->end();
+           ++baseline_local, ++scope_local) {
+        if (scope_local->mode() == VAR || scope_local->mode() == LET ||
+            scope_local->mode() == CONST) {
+          // Sanity check the variable name. If this fails, the variable order
+          // is not deterministic.
+          CHECK_EQ(scope_local->raw_name()->length(),
+                   baseline_local->raw_name()->length());
+          for (int i = 0; i < scope_local->raw_name()->length(); ++i) {
+            CHECK_EQ(scope_local->raw_name()->raw_data()[i],
+                     baseline_local->raw_name()->raw_data()[i]);
+          }
 
-    for (Variable* local : scope->locals_) {
-      if (local->mode() == VAR || local->mode() == LET ||
-          local->mode() == CONST) {
-#ifdef DEBUG
-        const AstRawString* local_name = local->raw_name();
-        int name_length = data->backing_store_[index++];
-        CHECK_EQ(name_length, local_name->length());
-        for (int i = 0; i < name_length; ++i) {
-          CHECK_EQ(data->backing_store_[index++], local_name->raw_data()[i]);
-        }
-#endif
-        // Allow PreParser to not distinguish between parameter / local; that
-        // information is not relevant for deciding the allocation (potentially
-        // skipped inner functions don't affect it).
-        int location = data->backing_store_[index++];
-        switch (local->location()) {
-          case PARAMETER:
-          case LOCAL:
-            CHECK(location == PARAMETER || location == LOCAL);
-            break;
-          case CONTEXT:
-          case UNALLOCATED:
-            CHECK_EQ(location, local->location());
-            break;
-          default:
-            CHECK(false);
-        }
-
-        if (precise_maybe_assigned) {
-          CHECK_EQ(data->backing_store_[index++], local->maybe_assigned());
-        } else {
-          STATIC_ASSERT(kMaybeAssigned > kNotAssigned);
-          CHECK_GE(data->backing_store_[index++], local->maybe_assigned());
+          CHECK_EQ(scope_local->location(), baseline_local->location());
+          if (precise_maybe_assigned) {
+            CHECK_EQ(scope_local->maybe_assigned(),
+                     baseline_local->maybe_assigned());
+          } else {
+            STATIC_ASSERT(kMaybeAssigned > kNotAssigned);
+            CHECK_GE(scope_local->maybe_assigned(),
+                     baseline_local->maybe_assigned());
+          }
         }
       }
     }
 
-    for (Scope* inner = scope->inner_scope(); inner != nullptr;
-         inner = inner->sibling()) {
-      if (PreParsedScopeData::HasVariablesWhichNeedAllocationData(inner)) {
-        CompareScopeToData(inner, data, index, precise_maybe_assigned);
-      }
+    for (Scope *baseline_inner = baseline->inner_scope(),
+               *scope_inner = scope->inner_scope();
+         scope_inner != nullptr; scope_inner = scope_inner->sibling(),
+               baseline_inner = baseline_inner->sibling()) {
+      CompareScopes(baseline_inner, scope_inner, precise_maybe_assigned);
     }
   }
 };