[interpreter/runtime] Simplify how global declarations are processed

This makes the code a little more specific to what's happening: There is only 1
global scope, and if there is one, we know its declarations are
info->scope()->declarations(). That means we don't need multiple
GlobalDeclarationsBuilders, and we don't need to cache partially serialized
versions of the declarations. One builder is enough, and we can simply walk
those declarations if there are any.

Additionally this CL drops unnecessary information passed into DeclareGlobals:
- Global functions always have the name on the shared function info, so we can
  drop the name.
- Due to lazy feedback vectors there's no point in trying to preinitialize
  global loads. Also this was only preinitializing global loads at the script
  level, not sub functions; without even checking whether the global load was
  used. It may actually have caused us to do more work and allocate more global
  load feedback slots than neccessary.

Change-Id: Ibbdd029abe5a39ba27f7fc9be84670c5d444d98d
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1997123
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65725}

src/interpreter/bytecode-generator.cc

@@ -727,54 +727,44 @@ class BytecodeGenerator::TestResultScope final : public ExpressionResultScope {
 // Used to build a list of global declaration initial value pairs.
 class BytecodeGenerator::GlobalDeclarationsBuilder final : public ZoneObject {
  public:
-  explicit GlobalDeclarationsBuilder(Zone* zone)
-      : declarations_(0, zone),
-        constant_pool_entry_(0),
-        has_constant_pool_entry_(false) {}
-
-  void AddFunctionDeclaration(const AstRawString* name, FeedbackSlot slot,
-                              int feedback_cell_index, FunctionLiteral* func) {
-    DCHECK(!slot.IsInvalid());
-    declarations_.push_back(Declaration(name, slot, feedback_cell_index, func));
-  }
-
-  void AddUndefinedDeclaration(const AstRawString* name, FeedbackSlot slot) {
-    DCHECK(!slot.IsInvalid());
-    declarations_.push_back(Declaration(name, slot));
-  }
-
   Handle<FixedArray> AllocateDeclarations(UnoptimizedCompilationInfo* info,
+                                          BytecodeGenerator* generator,
                                           Handle<Script> script,
                                           Isolate* isolate) {
+    int size = 0;
+    for (Declaration* decl : *info->scope()->declarations()) {
+      Variable* var = decl->var();
+      if (!var->is_used()) continue;
+      if (var->location() != VariableLocation::UNALLOCATED) continue;
+      DCHECK_IMPLIES(decl->node_type() != AstNode::kVariableDeclaration,
+                     decl->node_type() == AstNode::kFunctionDeclaration);
+      size += decl->node_type() == AstNode::kVariableDeclaration ? 1 : 2;
+    }
+
     DCHECK(has_constant_pool_entry_);
-    int array_index = 0;
-    Handle<FixedArray> data = isolate->factory()->NewFixedArray(
-        static_cast<int>(declarations_.size() * 4), AllocationType::kOld);
-    for (const Declaration& declaration : declarations_) {
-      FunctionLiteral* func = declaration.func;
-      Handle<Object> initial_value;
-      if (func == nullptr) {
-        initial_value = isolate->factory()->undefined_value();
-      } else {
-        initial_value = Compiler::GetSharedFunctionInfo(func, script, isolate);
-      }
 
-      // Return a null handle if any initial values can't be created. Caller
-      // will set stack overflow.
-      if (initial_value.is_null()) return Handle<FixedArray>();
+    Handle<FixedArray> data =
+        isolate->factory()->NewFixedArray(size, AllocationType::kOld);
 
-      data->set(array_index++, *declaration.name->string());
-      data->set(array_index++, Smi::FromInt(declaration.slot.ToInt()));
-      Object undefined_or_literal_slot;
-      if (declaration.feedback_cell_index_for_function == -1) {
-        undefined_or_literal_slot = ReadOnlyRoots(isolate).undefined_value();
+    int array_index = 0;
+    for (Declaration* decl : *info->scope()->declarations()) {
+      Variable* var = decl->var();
+      if (!var->is_used()) continue;
+      if (var->location() != VariableLocation::UNALLOCATED) continue;
+      if (decl->node_type() == AstNode::kVariableDeclaration) {
+        data->set(array_index++, *var->raw_name()->string());
       } else {
-        undefined_or_literal_slot =
-            Smi::FromInt(declaration.feedback_cell_index_for_function);
+        FunctionLiteral* f = static_cast<FunctionDeclaration*>(decl)->fun();
+        Handle<Object> sfi(Compiler::GetSharedFunctionInfo(f, script, isolate));
+        // Return a null handle if any initial values can't be created. Caller
+        // will set stack overflow.
+        if (sfi.is_null()) return Handle<FixedArray>();
+        data->set(array_index++, *sfi);
+        int literal_index = generator->GetCachedCreateClosureSlot(f);
+        data->set(array_index++, Smi::FromInt(literal_index));
       }
-      data->set(array_index++, undefined_or_literal_slot);
-      data->set(array_index++, *initial_value);
     }
+
     return data;
   }
 
@@ -784,40 +774,22 @@ class BytecodeGenerator::GlobalDeclarationsBuilder final : public ZoneObject {
   }
 
   void set_constant_pool_entry(size_t constant_pool_entry) {
-    DCHECK(!empty());
+    DCHECK(has_global_declaration());
     DCHECK(!has_constant_pool_entry_);
     constant_pool_entry_ = constant_pool_entry;
     has_constant_pool_entry_ = true;
   }
 
-  bool empty() { return declarations_.empty(); }
+  void record_global_declaration() { has_seen_global_declaration_ = true; }
+  bool has_global_declaration() { return has_seen_global_declaration_; }
+  bool processed() { return processed_; }
+  void mark_processed() { processed_ = true; }
 
  private:
-  struct Declaration {
-    Declaration() : slot(FeedbackSlot::Invalid()), func(nullptr) {}
-    Declaration(const AstRawString* name, FeedbackSlot slot,
-                int feedback_cell_index, FunctionLiteral* func)
-        : name(name),
-          slot(slot),
-          feedback_cell_index_for_function(feedback_cell_index),
-          func(func) {}
-    Declaration(const AstRawString* name, FeedbackSlot slot)
-        : name(name),
-          slot(slot),
-          feedback_cell_index_for_function(-1),
-          func(nullptr) {}
-
-    const AstRawString* name;
-    FeedbackSlot slot;
-    // Only valid for function declarations. Specifies the index into the
-    // closure_feedback_cell array used when creating closures of this
-    // function.
-    int feedback_cell_index_for_function;
-    FunctionLiteral* func;
-  };
-  ZoneVector<Declaration> declarations_;
-  size_t constant_pool_entry_;
-  bool has_constant_pool_entry_;
+  size_t constant_pool_entry_ = 0;
+  bool has_constant_pool_entry_ = false;
+  bool has_seen_global_declaration_ = false;
+  bool processed_ = false;
 };
 
 class BytecodeGenerator::CurrentScope final {
@@ -1011,9 +983,8 @@ BytecodeGenerator::BytecodeGenerator(
       current_scope_(info->scope()),
       eager_inner_literals_(eager_inner_literals),
       feedback_slot_cache_(new (zone()) FeedbackSlotCache(zone())),
-      globals_builder_(new (zone()) GlobalDeclarationsBuilder(zone())),
+      globals_builder_(new (zone()) GlobalDeclarationsBuilder()),
       block_coverage_builder_(nullptr),
-      global_declarations_(0, zone()),
       function_literals_(0, zone()),
       native_function_literals_(0, zone()),
       object_literals_(0, zone()),
@@ -1095,13 +1066,13 @@ int BytecodeGenerator::CheckBytecodeMatches(Handle<BytecodeArray> bytecode) {
 
 void BytecodeGenerator::AllocateDeferredConstants(Isolate* isolate,
                                                   Handle<Script> script) {
-  // Build global declaration pair arrays.
-  for (GlobalDeclarationsBuilder* globals_builder : global_declarations_) {
+  if (globals_builder()->has_global_declaration()) {
+    // Build global declaration pair array.
     Handle<FixedArray> declarations =
-        globals_builder->AllocateDeclarations(info(), script, isolate);
+        globals_builder()->AllocateDeclarations(info(), this, script, isolate);
     if (declarations.is_null()) return SetStackOverflow();
     builder()->SetDeferredConstantPoolEntry(
-        globals_builder->constant_pool_entry(), declarations);
+        globals_builder()->constant_pool_entry(), declarations);
   }
 
   // Find or build shared function infos.
@@ -1250,7 +1221,11 @@ void BytecodeGenerator::GenerateBytecodeBody() {
   BuildIncrementBlockCoverageCounterIfEnabled(literal, SourceRangeKind::kBody);
 
   // Visit declarations within the function scope.
-  VisitDeclarations(closure_scope()->declarations());
+  if (closure_scope()->is_script_scope()) {
+    VisitGlobalDeclarations(closure_scope()->declarations());
+  } else {
+    VisitDeclarations(closure_scope()->declarations());
+  }
 
   // Emit initializing assignments for module namespace imports (if any).
   VisitModuleNamespaceImports();
@@ -1344,13 +1319,9 @@ void BytecodeGenerator::VisitVariableDeclaration(VariableDeclaration* decl) {
   if (!variable->is_used()) return;
 
   switch (variable->location()) {
-    case VariableLocation::UNALLOCATED: {
-      DCHECK(!variable->binding_needs_init());
-      FeedbackSlot slot =
-          GetCachedLoadGlobalICSlot(NOT_INSIDE_TYPEOF, variable);
-      globals_builder()->AddUndefinedDeclaration(variable->raw_name(), slot);
+    case VariableLocation::UNALLOCATED:
+      globals_builder()->record_global_declaration();
       break;
-    }
     case VariableLocation::LOCAL:
       if (variable->binding_needs_init()) {
         Register destination(builder()->Local(variable->index()));
@@ -1404,15 +1375,10 @@ void BytecodeGenerator::VisitFunctionDeclaration(FunctionDeclaration* decl) {
   if (!variable->is_used()) return;
 
   switch (variable->location()) {
-    case VariableLocation::UNALLOCATED: {
-      FeedbackSlot slot =
-          GetCachedLoadGlobalICSlot(NOT_INSIDE_TYPEOF, variable);
-      int literal_index = GetCachedCreateClosureSlot(decl->fun());
-      globals_builder()->AddFunctionDeclaration(variable->raw_name(), slot,
-                                                literal_index, decl->fun());
+    case VariableLocation::UNALLOCATED:
       AddToEagerLiteralsIfEager(decl->fun());
+      globals_builder()->record_global_declaration();
       break;
-    }
     case VariableLocation::PARAMETER:
     case VariableLocation::LOCAL: {
       VisitFunctionLiteral(decl->fun());
@@ -1467,14 +1433,12 @@ void BytecodeGenerator::VisitModuleNamespaceImports() {
   }
 }
 
-void BytecodeGenerator::VisitDeclarations(Declaration::List* declarations) {
+void BytecodeGenerator::VisitGlobalDeclarations(Declaration::List* decls) {
   RegisterAllocationScope register_scope(this);
-  DCHECK(globals_builder()->empty());
-  for (Declaration* decl : *declarations) {
-    RegisterAllocationScope register_scope(this);
-    Visit(decl);
-  }
-  if (globals_builder()->empty()) return;
+  VisitDeclarations(decls);
+
+  if (!globals_builder()->has_global_declaration()) return;
+  DCHECK(!globals_builder()->processed());
 
   globals_builder()->set_constant_pool_entry(
       builder()->AllocateDeferredConstantPoolEntry());
@@ -1490,9 +1454,14 @@ void BytecodeGenerator::VisitDeclarations(Declaration::List* declarations) {
       .MoveRegister(Register::function_closure(), args[2])
       .CallRuntime(Runtime::kDeclareGlobals, args);
 
-  // Push and reset globals builder.
-  global_declarations_.push_back(globals_builder());
-  globals_builder_ = new (zone()) GlobalDeclarationsBuilder(zone());
+  globals_builder()->mark_processed();
+}
+
+void BytecodeGenerator::VisitDeclarations(Declaration::List* declarations) {
+  for (Declaration* decl : *declarations) {
+    RegisterAllocationScope register_scope(this);
+    Visit(decl);
+  }
 }
 
 void BytecodeGenerator::VisitStatements(

src/interpreter/bytecode-generator.h

@@ -50,6 +50,7 @@ class BytecodeGenerator final : public AstVisitor<BytecodeGenerator> {
 #undef DECLARE_VISIT
 
   // Visiting function for declarations list and statements are overridden.
+  void VisitGlobalDeclarations(Declaration::List* declarations);
   void VisitDeclarations(Declaration::List* declarations);
   void VisitStatements(const ZonePtrList<Statement>* statments);
 
@@ -495,7 +496,6 @@ class BytecodeGenerator final : public AstVisitor<BytecodeGenerator> {
 
   GlobalDeclarationsBuilder* globals_builder_;
   BlockCoverageBuilder* block_coverage_builder_;
-  ZoneVector<GlobalDeclarationsBuilder*> global_declarations_;
   ZoneVector<std::pair<FunctionLiteral*, size_t>> function_literals_;
   ZoneVector<std::pair<NativeFunctionLiteral*, size_t>>
       native_function_literals_;

src/runtime/runtime-scopes.cc

@@ -48,9 +48,8 @@ Object ThrowRedeclarationError(Isolate* isolate, Handle<String> name,
 Object DeclareGlobal(
     Isolate* isolate, Handle<JSGlobalObject> global, Handle<String> name,
     Handle<Object> value, PropertyAttributes attr, bool is_var,
-    bool is_function_declaration, RedeclarationType redeclaration_type,
-    Handle<FeedbackVector> feedback_vector = Handle<FeedbackVector>(),
-    FeedbackSlot slot = FeedbackSlot::Invalid()) {
+    RedeclarationType redeclaration_type,
+    Handle<FeedbackVector> feedback_vector = Handle<FeedbackVector>()) {
   Handle<ScriptContextTable> script_contexts(
       global->native_context().script_context_table(), isolate);
   ScriptContextTable::LookupResult lookup;
@@ -66,7 +65,7 @@ Object DeclareGlobal(
   // Do the lookup own properties only, see ES5 erratum.
   LookupIterator::Configuration lookup_config(
       LookupIterator::Configuration::OWN_SKIP_INTERCEPTOR);
-  if (is_function_declaration) {
+  if (!is_var) {
     // For function declarations, use the interceptor on the declaration. For
     // non-functions, use it only on initialization.
     lookup_config = LookupIterator::Configuration::OWN;
@@ -82,7 +81,6 @@ Object DeclareGlobal(
     // Skip var re-declarations.
     if (is_var) return ReadOnlyRoots(isolate).undefined_value();
 
-    DCHECK(is_function_declaration);
     if ((old_attributes & DONT_DELETE) != 0) {
       // Only allow reconfiguring globals to functions in user code (no
       // natives, which are marked as read-only).
@@ -111,25 +109,12 @@ Object DeclareGlobal(
     if (it.state() == LookupIterator::ACCESSOR) it.Delete();
   }
 
-  if (is_function_declaration) {
-    it.Restart();
-  }
+  if (!is_var) it.Restart();
 
   // Define or redefine own property.
   RETURN_FAILURE_ON_EXCEPTION(
       isolate, JSObject::DefineOwnPropertyIgnoreAttributes(&it, value, attr));
 
-  if (!feedback_vector.is_null() &&
-      it.state() != LookupIterator::State::INTERCEPTOR) {
-    DCHECK_EQ(*global, *it.GetHolder<Object>());
-    // Preinitialize the feedback slot if the global object does not have
-    // named interceptor or the interceptor is not masking.
-    if (!global->HasNamedInterceptor() ||
-        global->GetNamedInterceptor().non_masking()) {
-      FeedbackNexus nexus(feedback_vector, slot);
-      nexus.ConfigurePropertyCellMode(it.GetPropertyCell());
-    }
-  }
   return ReadOnlyRoots(isolate).undefined_value();
 }
 
@@ -154,32 +139,23 @@ Object DeclareGlobals(Isolate* isolate, Handle<FixedArray> declarations,
 
   // Traverse the name/value pairs and set the properties.
   int length = declarations->length();
-  FOR_WITH_HANDLE_SCOPE(isolate, int, i = 0, i, i < length, i += 4, {
-    Handle<String> name(String::cast(declarations->get(i)), isolate);
-    FeedbackSlot slot(Smi::ToInt(declarations->get(i + 1)));
-    Handle<Object> possibly_feedback_cell_slot(declarations->get(i + 2),
-                                               isolate);
-    Handle<Object> initial_value(declarations->get(i + 3), isolate);
-
-    bool is_var = initial_value->IsUndefined(isolate);
-    bool is_function = initial_value->IsSharedFunctionInfo();
-    DCHECK_NE(is_var, is_function);
-
+  FOR_WITH_HANDLE_SCOPE(isolate, int, i = 0, i, i < length, i++, {
+    Handle<Object> decl(declarations->get(i), isolate);
+    Handle<String> name;
     Handle<Object> value;
-    if (is_function) {
-      DCHECK(possibly_feedback_cell_slot->IsSmi());
-      Handle<FeedbackCell> feedback_cell =
-          closure_feedback_cell_array->GetFeedbackCell(
-              Smi::ToInt(*possibly_feedback_cell_slot));
-      // Copy the function and update its context. Use it as value.
-      Handle<SharedFunctionInfo> shared =
-          Handle<SharedFunctionInfo>::cast(initial_value);
-      Handle<JSFunction> function =
-          isolate->factory()->NewFunctionFromSharedFunctionInfo(
-              shared, context, feedback_cell, AllocationType::kOld);
-      value = function;
-    } else {
+    bool is_var = decl->IsString();
+
+    if (is_var) {
+      name = Handle<String>::cast(decl);
       value = isolate->factory()->undefined_value();
+    } else {
+      Handle<SharedFunctionInfo> sfi = Handle<SharedFunctionInfo>::cast(decl);
+      name = handle(sfi->Name(), isolate);
+      int index = Smi::ToInt(declarations->get(++i));
+      Handle<FeedbackCell> feedback_cell =
+          closure_feedback_cell_array->GetFeedbackCell(index);
+      value = isolate->factory()->NewFunctionFromSharedFunctionInfo(
+          sfi, context, feedback_cell, AllocationType::kOld);
     }
 
     // Compute the property attributes. According to ECMA-262,
@@ -190,10 +166,9 @@ Object DeclareGlobals(Isolate* isolate, Handle<FixedArray> declarations,
 
     // ES#sec-globaldeclarationinstantiation 5.d:
     // If hasRestrictedGlobal is true, throw a SyntaxError exception.
-    Object result = DeclareGlobal(isolate, global, name, value,
-                                  static_cast<PropertyAttributes>(attr), is_var,
-                                  is_function, RedeclarationType::kSyntaxError,
-                                  feedback_vector, slot);
+    Object result = DeclareGlobal(
+        isolate, global, name, value, static_cast<PropertyAttributes>(attr),
+        is_var, RedeclarationType::kSyntaxError, feedback_vector);
     if (isolate->has_pending_exception()) return result;
   });
 
@@ -228,9 +203,8 @@ Object DeclareEvalHelper(Isolate* isolate, Handle<String> name,
          (context->IsBlockContext() &&
           context->scope_info().is_declaration_scope()));
 
-  bool is_function = value->IsJSFunction();
-  bool is_var = !is_function;
-  DCHECK(!is_var || value->IsUndefined(isolate));
+  bool is_var = value->IsUndefined(isolate);
+  DCHECK_IMPLIES(!is_var, value->IsJSFunction());
 
   int index;
   PropertyAttributes attributes;
@@ -249,20 +223,19 @@ Object DeclareEvalHelper(Isolate* isolate, Handle<String> name,
     // ES#sec-evaldeclarationinstantiation 8.a.iv.1.b:
     // If fnDefinable is false, throw a TypeError exception.
     return DeclareGlobal(isolate, Handle<JSGlobalObject>::cast(holder), name,
-                         value, NONE, is_var, is_function,
-                         RedeclarationType::kTypeError);
+                         value, NONE, is_var, RedeclarationType::kTypeError);
   }
   if (context->has_extension() && context->extension().IsJSGlobalObject()) {
     Handle<JSGlobalObject> global(JSGlobalObject::cast(context->extension()),
                                   isolate);
     return DeclareGlobal(isolate, global, name, value, NONE, is_var,
-                         is_function, RedeclarationType::kTypeError);
+                         RedeclarationType::kTypeError);
   } else if (context->IsScriptContext()) {
     DCHECK(context->global_object().IsJSGlobalObject());
     Handle<JSGlobalObject> global(
         JSGlobalObject::cast(context->global_object()), isolate);
     return DeclareGlobal(isolate, global, name, value, NONE, is_var,
-                         is_function, RedeclarationType::kTypeError);
+                         RedeclarationType::kTypeError);
   }
 
   if (attributes != ABSENT) {
@@ -271,7 +244,6 @@ Object DeclareEvalHelper(Isolate* isolate, Handle<String> name,
     // Skip var re-declarations.
     if (is_var) return ReadOnlyRoots(isolate).undefined_value();
 
-    DCHECK(is_function);
     if (index != Context::kNotFound) {
       DCHECK(holder.is_identical_to(context));
       context->set(index, *value);

test/cctest/interpreter/bytecode_expectations/DeclareGlobals.golden

@@ -22,7 +22,7 @@ bytecodes: [
                 B(Mov), R(closure), R(3),
                 B(CallRuntime), U16(Runtime::kDeclareGlobals), R(1), U8(3),
   /*    8 S> */ B(LdaSmi), I8(1),
-  /*    8 E> */ B(StaGlobal), U8(1), U8(2),
+  /*    8 E> */ B(StaGlobal), U8(1), U8(0),
                 B(LdaUndefined),
   /*   11 S> */ B(Return),
 ]
@@ -74,9 +74,9 @@ bytecodes: [
                 B(Mov), R(closure), R(3),
                 B(CallRuntime), U16(Runtime::kDeclareGlobals), R(1), U8(3),
   /*    8 S> */ B(LdaSmi), I8(1),
-  /*    8 E> */ B(StaGlobal), U8(1), U8(2),
+  /*    8 E> */ B(StaGlobal), U8(1), U8(0),
   /*   11 S> */ B(LdaSmi), I8(2),
-  /*   12 E> */ B(StaGlobal), U8(1), U8(2),
+  /*   12 E> */ B(StaGlobal), U8(1), U8(0),
                 B(Star), R(0),
   /*   16 S> */ B(Return),
 ]

test/cctest/interpreter/bytecode_expectations/TopLevelObjectLiterals.golden

@@ -21,12 +21,12 @@ bytecodes: [
                 B(Star), R(2),
                 B(Mov), R(closure), R(3),
                 B(CallRuntime), U16(Runtime::kDeclareGlobals), R(1), U8(3),
-  /*    8 S> */ B(CreateObjectLiteral), U8(1), U8(2), U8(41),
+  /*    8 S> */ B(CreateObjectLiteral), U8(1), U8(0), U8(41),
                 B(Star), R(1),
   /*   16 E> */ B(CreateClosure), U8(2), U8(0), U8(0),
-                B(StaNamedOwnProperty), R(1), U8(3), U8(3),
+                B(StaNamedOwnProperty), R(1), U8(3), U8(1),
                 B(Ldar), R(1),
-  /*    8 E> */ B(StaGlobal), U8(4), U8(5),
+  /*    8 E> */ B(StaGlobal), U8(4), U8(3),
                 B(LdaUndefined),
   /*   34 S> */ B(Return),
 ]