[turbofan] Clean up heap broker initialization

E.g. make the setup of the ObjectRef hash table more explicit.

Tbr: jgruber@chromium.org
Bug: v8:7790
Change-Id: I58c03848e7da5c418ff2d6ae1e71b644278f406b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1776089
Commit-Queue: Georg Neis <neis@chromium.org>
Reviewed-by: Maya Lekova <mslekova@chromium.org>
Cr-Commit-Position: refs/heads/master@{#63475}

src/compiler/heap-refs.h

@@ -165,6 +165,7 @@ class V8_EXPORT_PRIVATE ObjectRef {
   friend class JSArrayData;
   friend class JSGlobalProxyRef;
   friend class JSGlobalProxyData;
+  friend class JSHeapBroker;
   friend class JSObjectData;
   friend class StringData;
 

src/compiler/js-heap-broker.cc

@@ -2258,11 +2258,11 @@ JSHeapBroker::JSHeapBroker(Isolate* isolate, Zone* broker_zone,
       bytecode_analyses_(zone()),
       property_access_infos_(zone()),
       typed_array_string_tags_(zone()) {
-  // Note that this initialization of the refs_ pointer with the minimal
-  // initial capacity is redundant in the normal use case (concurrent
-  // compilation enabled, standard objects to be serialized), as the map
-  // is going to be replaced immediatelly with a larger capacity one.
-  // It doesn't seem to affect the performance in a noticeable way though.
+  // Note that this initialization of {refs_} with the minimal initial capacity
+  // is redundant in the normal use case (concurrent compilation enabled,
+  // standard objects to be serialized), as the map is going to be replaced
+  // immediately with a larger-capacity one.  It doesn't seem to affect the
+  // performance in a noticeable way though.
   TRACE(this, "Constructing heap broker");
 }
 
@@ -2271,13 +2271,6 @@ std::ostream& JSHeapBroker::Trace() {
                     << std::string(trace_indentation_ * 2, ' ');
 }
 
-void JSHeapBroker::StartSerializing() {
-  CHECK_EQ(mode_, kDisabled);
-  TRACE(this, "Starting serialization");
-  mode_ = kSerializing;
-  refs_->Clear();
-}
-
 void JSHeapBroker::StopSerializing() {
   CHECK_EQ(mode_, kSerializing);
   TRACE(this, "Stopping serialization");
@@ -2292,39 +2285,51 @@ void JSHeapBroker::Retire() {
 
 bool JSHeapBroker::SerializingAllowed() const { return mode() == kSerializing; }
 
-void JSHeapBroker::SetNativeContextRef(Handle<NativeContext> context) {
-  // At the broker construction time, we don't yet have the canonical handle
-  // for the native context, that's why we need the explicit setter
-  target_native_context_ = NativeContextRef(this, context);
+void JSHeapBroker::SetTargetNativeContextRef(
+    Handle<NativeContext> native_context) {
+  // The MapData constructor uses {target_native_context_}. This creates a
+  // benign cycle that we break by setting {target_native_context_} right before
+  // starting to serialize (thus creating dummy data), and then again properly
+  // right after.
+  DCHECK((mode() == kDisabled && !target_native_context_.has_value()) ||
+         (mode() == kSerializing &&
+          target_native_context_->object().equals(native_context) &&
+          target_native_context_->data_->kind() == kUnserializedHeapObject));
+  target_native_context_ = NativeContextRef(this, native_context);
 }
 
 bool IsShareable(Handle<Object> object, Isolate* isolate) {
-  Builtins* const b = isolate->builtins();
-
   int index;
   RootIndex root_index;
-  return (object->IsHeapObject() &&
-          b->IsBuiltinHandle(Handle<HeapObject>::cast(object), &index)) ||
+  bool is_builtin_handle =
+      object->IsHeapObject() && isolate->builtins()->IsBuiltinHandle(
+                                    Handle<HeapObject>::cast(object), &index);
+  return is_builtin_handle ||
          isolate->roots_table().IsRootHandle(object, &root_index);
 }
 
-void JSHeapBroker::SerializeShareableObjects() {
+void JSHeapBroker::InitializeRefsMap() {
+  TraceScope tracer(this, "JSHeapBroker::InitializeRefsMap");
+
+  DCHECK_NULL(compiler_cache_);
   PerIsolateCompilerCache::Setup(isolate());
   compiler_cache_ = isolate()->compiler_cache();
 
   if (compiler_cache_->HasSnapshot()) {
-    RefsMap* snapshot = compiler_cache_->GetSnapshot();
-
-    refs_ = new (zone()) RefsMap(snapshot, zone());
+    TRACE(this, "Importing existing RefsMap snapshot");
+    DCHECK_NULL(refs_);
+    refs_ = new (zone()) RefsMap(compiler_cache_->GetSnapshot(), zone());
     return;
   }
 
-  TraceScope tracer(
-      this, "JSHeapBroker::SerializeShareableObjects (building snapshot)");
-
+  TRACE(this, "Building RefsMap snapshot");
+  DCHECK_NULL(refs_);
   refs_ =
       new (zone()) RefsMap(kInitialRefsBucketCount, AddressMatcher(), zone());
 
+  // Temporarily use the "compiler zone" for serialization, such that the
+  // serialized data survives this compilation.
+  DCHECK_EQ(current_zone_, broker_zone_);
   current_zone_ = compiler_cache_->zone();
 
   Builtins* const b = isolate()->builtins();
@@ -2360,12 +2365,13 @@ void JSHeapBroker::SerializeShareableObjects() {
     }
   }
 
+  // TODO(mslekova): Serialize root objects (from factory).
+
+  // Verify.
   for (RefsMap::Entry* p = refs_->Start(); p != nullptr; p = refs_->Next(p)) {
     CHECK(IsShareable(p->value->object(), isolate()));
   }
 
-  // TODO(mslekova):
-  // Serialize root objects (from factory).
   compiler_cache()->SetSnapshot(refs_);
   current_zone_ = broker_zone_;
 }
@@ -2420,22 +2426,25 @@ bool JSHeapBroker::IsArrayOrObjectPrototype(const JSObjectRef& object) const {
          array_and_object_prototypes_.end();
 }
 
-void JSHeapBroker::SerializeStandardObjects(Handle<NativeContext> context) {
-  if (mode() == kDisabled) return;
-  CHECK_EQ(mode(), kSerializing);
+void JSHeapBroker::InitializeAndStartSerializing(
+    Handle<NativeContext> native_context) {
+  TraceScope tracer(this, "JSHeapBroker::InitializeAndStartSerializing");
 
-  SerializeShareableObjects();
+  CHECK_EQ(mode_, kDisabled);
+  mode_ = kSerializing;
 
-  TraceScope tracer(this, "JSHeapBroker::SerializeStandardObjects");
+  // Throw away the dummy data that we created while disabled.
+  refs_->Clear();
+  refs_ = nullptr;
 
-  CollectArrayAndObjectPrototypes();
-  SerializeTypedArrayStringTags();
+  InitializeRefsMap();
 
-  // It's important that this serialization happens after the
-  // CollectArrayAndObjectPrototypes is done.
-  SetNativeContextRef(context);
+  SetTargetNativeContextRef(native_context);
   target_native_context().Serialize();
 
+  CollectArrayAndObjectPrototypes();
+  SerializeTypedArrayStringTags();
+
   Factory* const f = isolate()->factory();
 
   // Maps, strings, oddballs

src/compiler/js-heap-broker.h

@@ -67,20 +67,21 @@ class V8_EXPORT_PRIVATE JSHeapBroker {
  public:
   JSHeapBroker(Isolate* isolate, Zone* broker_zone, bool tracing_enabled);
 
-  void SetNativeContextRef(Handle<NativeContext> context);
-  void SerializeStandardObjects(Handle<NativeContext> context);
+  // The compilation target's native context. We need the setter because at
+  // broker construction time we don't yet have the canonical handle.
+  NativeContextRef target_native_context() const {
+    return target_native_context_.value();
+  }
+  void SetTargetNativeContextRef(Handle<NativeContext> native_context);
+
+  void InitializeAndStartSerializing(Handle<NativeContext> native_context);
 
   Isolate* isolate() const { return isolate_; }
   Zone* zone() const { return current_zone_; }
   bool tracing_enabled() const { return tracing_enabled_; }
-  NativeContextRef target_native_context() const {
-    return target_native_context_.value();
-  }
-  PerIsolateCompilerCache* compiler_cache() const { return compiler_cache_; }
 
   enum BrokerMode { kDisabled, kSerializing, kSerialized, kRetired };
   BrokerMode mode() const { return mode_; }
-  void StartSerializing();
   void StopSerializing();
   void Retire();
   bool SerializingAllowed() const;
@@ -182,13 +183,15 @@ class V8_EXPORT_PRIVATE JSHeapBroker {
       FeedbackSource const& source, AccessMode mode,
       base::Optional<NameRef> static_name);
 
-  void SerializeShareableObjects();
+  void InitializeRefsMap();
   void CollectArrayAndObjectPrototypes();
   void SerializeTypedArrayStringTags();
 
+  PerIsolateCompilerCache* compiler_cache() const { return compiler_cache_; }
+
   Isolate* const isolate_;
   Zone* const broker_zone_;
-  Zone* current_zone_;
+  Zone* current_zone_ = nullptr;
   base::Optional<NativeContextRef> target_native_context_;
   RefsMap* refs_;
   ZoneUnorderedSet<Handle<JSObject>, Handle<JSObject>::hash,
@@ -198,7 +201,7 @@ class V8_EXPORT_PRIVATE JSHeapBroker {
   bool const tracing_enabled_;
   StdoutStream trace_out_;
   unsigned trace_indentation_ = 0;
-  PerIsolateCompilerCache* compiler_cache_;
+  PerIsolateCompilerCache* compiler_cache_ = nullptr;
   ZoneUnorderedMap<FeedbackSource, ProcessedFeedback const*,
                    FeedbackSource::Hash, FeedbackSource::Equal>
       feedback_;

src/compiler/per-isolate-compiler-cache.h

@@ -19,41 +19,41 @@ namespace compiler {
 
 class ObjectData;
 
-// This class serves as a per-isolate container of data that should be
-// persisted between compiler runs. For now it stores the code builtins
-// so they are not serialized on each compiler run.
+// This class serves as a container of data that should persist across all
+// (optimizing) compiler runs in an isolate. For now it stores serialized data
+// for various common objects such as builtins, so that these objects don't have
+// to be serialized in each compilation job. See JSHeapBroker::InitializeRefsMap
+// for details.
 class PerIsolateCompilerCache : public ZoneObject {
  public:
   explicit PerIsolateCompilerCache(Zone* zone)
       : zone_(zone), refs_snapshot_(nullptr) {}
 
-  RefsMap* GetSnapshot() { return refs_snapshot_; }
+  bool HasSnapshot() const { return refs_snapshot_ != nullptr; }
+  RefsMap* GetSnapshot() {
+    DCHECK(HasSnapshot());
+    return refs_snapshot_;
+  }
   void SetSnapshot(RefsMap* refs) {
-    DCHECK_NULL(refs_snapshot_);
+    DCHECK(!HasSnapshot());
     DCHECK(!refs->IsEmpty());
     refs_snapshot_ = new (zone_) RefsMap(refs, zone_);
+    DCHECK(HasSnapshot());
   }
 
-  bool HasSnapshot() const { return refs_snapshot_; }
-
   Zone* zone() const { return zone_; }
 
   static void Setup(Isolate* isolate) {
-    if (isolate->compiler_cache()) return;
-
-    // The following zone is supposed to contain compiler-related objects
-    // that should live through all compilations, as opposed to the
-    // broker_zone which holds per-compilation data. It's not meant for
-    // per-compilation or heap broker data.
-    Zone* compiler_zone = new Zone(isolate->allocator(), "Compiler zone");
-    PerIsolateCompilerCache* compiler_cache =
-        new (compiler_zone) PerIsolateCompilerCache(compiler_zone);
-    isolate->set_compiler_utils(compiler_cache, compiler_zone);
+    if (isolate->compiler_cache() == nullptr) {
+      Zone* zone = new Zone(isolate->allocator(), "Compiler zone");
+      PerIsolateCompilerCache* cache = new (zone) PerIsolateCompilerCache(zone);
+      isolate->set_compiler_utils(cache, zone);
+    }
+    DCHECK_NOT_NULL(isolate->compiler_cache());
   }
 
  private:
   Zone* const zone_;
-
   RefsMap* refs_snapshot_;
 };
 

src/compiler/pipeline.cc

@@ -1358,11 +1358,11 @@ struct UntyperPhase {
   }
 };
 
-struct SerializeStandardObjectsPhase {
-  static const char* phase_name() { return "V8.TFSerializeStandardObjects"; }
+struct HeapBrokerInitializationPhase {
+  static const char* phase_name() { return "V8.TFHeapBrokerInitialization"; }
 
   void Run(PipelineData* data, Zone* temp_zone) {
-    data->broker()->SerializeStandardObjects(data->native_context());
+    data->broker()->InitializeAndStartSerializing(data->native_context());
   }
 };
 
@@ -1384,11 +1384,8 @@ struct CopyMetadataForConcurrentCompilePhase {
   }
 };
 
-// TODO(turbofan): Move all calls from CopyMetaDataForConcurrentCompilePhase
-// here. Also all the calls to Serialize* methods that are currently sprinkled
-// over inlining will move here as well.
 struct SerializationPhase {
-  static const char* phase_name() { return "V8.TFSerializeBytecode"; }
+  static const char* phase_name() { return "V8.TFSerialization"; }
 
   void Run(PipelineData* data, Zone* temp_zone) {
     SerializerForBackgroundCompilationFlags flags;
@@ -2212,17 +2209,10 @@ bool PipelineImpl::CreateGraph() {
     data->node_origins()->AddDecorator();
   }
 
+  data->broker()->SetTargetNativeContextRef(data->native_context());
   if (FLAG_concurrent_inlining) {
-    // The following is a workaround for the NativeContexRef storage being
-    // unpopulated when we start the SerializeStandardObjectsPhase, therefore
-    // stopping us from the ability to use the target_native_context getter
-    // in the MapData constructor.
-    data->broker()->SetNativeContextRef(data->native_context());
-    data->broker()->StartSerializing();
-    Run<SerializeStandardObjectsPhase>();
+    Run<HeapBrokerInitializationPhase>();
     Run<SerializationPhase>();
-  } else {
-    data->broker()->SetNativeContextRef(data->native_context());
   }
 
   Run<GraphBuilderPhase>();
@@ -2261,8 +2251,7 @@ bool PipelineImpl::CreateGraph() {
       Run<CopyMetadataForConcurrentCompilePhase>();
       data->broker()->StopSerializing();
     } else {
-      data->broker()->StartSerializing();
-      Run<SerializeStandardObjectsPhase>();
+      Run<HeapBrokerInitializationPhase>();
       Run<CopyMetadataForConcurrentCompilePhase>();
       data->broker()->StopSerializing();
     }

src/execution/isolate.h

@@ -1779,6 +1779,8 @@ class Isolate final : private HiddenFactory {
   interpreter::Interpreter* interpreter_ = nullptr;
 
   compiler::PerIsolateCompilerCache* compiler_cache_ = nullptr;
+  // The following zone is for compiler-related objects that should live
+  // through all compilations (and thus all JSHeapBroker instances).
   Zone* compiler_zone_ = nullptr;
 
   CompilerDispatcher* compiler_dispatcher_ = nullptr;

test/unittests/compiler/graph-unittest.cc

@@ -23,7 +23,7 @@ GraphTest::GraphTest(int num_parameters)
       node_origins_(&graph_) {
   graph()->SetStart(graph()->NewNode(common()->Start(num_parameters)));
   graph()->SetEnd(graph()->NewNode(common()->End(1), graph()->start()));
-  broker()->SetNativeContextRef(isolate()->native_context());
+  broker()->SetTargetNativeContextRef(isolate()->native_context());
 }
 
 GraphTest::~GraphTest() = default;

test/unittests/compiler/js-call-reducer-unittest.cc

@@ -24,7 +24,6 @@ class JSCallReducerTest : public TypedGraphTest {
  public:
   JSCallReducerTest()
       : TypedGraphTest(3), javascript_(zone()), deps_(broker(), zone()) {
-    broker()->SerializeStandardObjects(isolate()->native_context());
   }
   ~JSCallReducerTest() override = default;