[compiler] Don't serialize BytecodeArrayData's source_positions_

This CL adds functionality to read the source positions directly
from the JS heap rather than from serialized data.

In order to do this, we create a PersistentHandles container in the
OptimizedCompilationInfo which gets passed onto the JSHeapBroker. This
allows us to create the handles in the main thread and pass them safely
to the background thread.

In order to read safely from the background thread, we need a LocalHeap
which blocks the GC from running and potentially moving the handles.
This LocalHeap is created only when the JSHeapBroker has finalized
serializing and destroyed when retiring it.

Bug: v8:7790
Change-Id: I19f8b08d12e5be0a3df34d6af2043310c0c7b6fe
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2277802Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Commit-Queue: Santiago Aboy Solanes <solanes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#68836}

BUILD.gn

@@ -2458,7 +2458,6 @@ v8_source_set("v8_base_without_compiler") {
     "src/handles/local-handles.h",
     "src/handles/maybe-handles-inl.h",
     "src/handles/maybe-handles.h",
-    "src/handles/persistent-handles-inl.h",
     "src/handles/persistent-handles.cc",
     "src/handles/persistent-handles.h",
     "src/heap/allocation-stats.h",

src/codegen/optimized-compilation-info.cc

@@ -28,6 +28,7 @@ OptimizedCompilationInfo::OptimizedCompilationInfo(
   bytecode_array_ = handle(shared->GetBytecodeArray(), isolate);
   shared_info_ = shared;
   closure_ = closure;
+  ph_ = isolate->NewPersistentHandles();
 
   // Collect source positions for optimized code when profiling or if debugger
   // is active, to be able to get more precise source positions at the price of

src/codegen/optimized-compilation-info.h

@@ -14,6 +14,7 @@
 #include "src/diagnostics/basic-block-profiler.h"
 #include "src/execution/frames.h"
 #include "src/handles/handles.h"
+#include "src/handles/persistent-handles.h"
 #include "src/objects/objects.h"
 #include "src/utils/utils.h"
 #include "src/utils/vector.h"
@@ -220,6 +221,10 @@ class V8_EXPORT_PRIVATE OptimizedCompilationInfo final {
     profiler_data_ = profiler_data;
   }
 
+  std::unique_ptr<PersistentHandles> DetachPersistentHandles() {
+    return std::move(ph_);
+  }
+
  private:
   void ConfigureFlags();
 
@@ -276,6 +281,15 @@ class V8_EXPORT_PRIVATE OptimizedCompilationInfo final {
 
   TickCounter tick_counter_;
 
+  // This PersistentHandles container is owned first by
+  // OptimizedCompilationInfo, then by JSHeapBroker, then by LocalHeap (when we
+  // go to the background thread), then again by JSHeapBroker (right before
+  // returning to the main thread), which gets destroyed when PipelineData gets
+  // destroyed when e.g. PipelineCompilationJob gets destroyed. Since it is a
+  // member of OptimizedCompilationInfo, we make sure that we have one and only
+  // one per compilation job.
+  std::unique_ptr<PersistentHandles> ph_;
+
   DISALLOW_COPY_AND_ASSIGN(OptimizedCompilationInfo);
 };
 

src/compiler/js-heap-broker.cc

@@ -1613,12 +1613,8 @@ class BytecodeArrayData : public FixedArrayBaseData {
       constant_pool_.push_back(broker->GetOrCreateData(constant_pool->get(i)));
     }
 
-    Handle<ByteArray> source_position_table(
-        bytecode_array->SourcePositionTableIfCollected(), broker->isolate());
-    source_positions_.reserve(source_position_table->length());
-    for (int i = 0; i < source_position_table->length(); i++) {
-      source_positions_.push_back(source_position_table->get(i));
-    }
+    source_positions_ = broker->NewPersistentHandle(
+        bytecode_array->SourcePositionTableIfCollected());
 
     Handle<ByteArray> handlers(bytecode_array->handler_table(),
                                broker->isolate());
@@ -1631,10 +1627,10 @@ class BytecodeArrayData : public FixedArrayBaseData {
   }
 
   const byte* source_positions_address() const {
-    return source_positions_.data();
+    return source_positions_->GetDataStartAddress();
   }
 
-  size_t source_positions_size() const { return source_positions_.size(); }
+  int source_positions_size() const { return source_positions_->length(); }
 
   Address handler_table_address() const {
     CHECK(is_serialized_for_compilation_);
@@ -1654,7 +1650,6 @@ class BytecodeArrayData : public FixedArrayBaseData {
         incoming_new_target_or_generator_register_(
             object->incoming_new_target_or_generator_register()),
         bytecodes_(broker->zone()),
-        source_positions_(broker->zone()),
         handler_table_(broker->zone()),
         constant_pool_(broker->zone()) {}
 
@@ -1665,7 +1660,7 @@ class BytecodeArrayData : public FixedArrayBaseData {
 
   bool is_serialized_for_compilation_ = false;
   ZoneVector<uint8_t> bytecodes_;
-  ZoneVector<uint8_t> source_positions_;
+  Handle<ByteArray> source_positions_;
   ZoneVector<uint8_t> handler_table_;
   ZoneVector<ObjectData*> constant_pool_;
 };
@@ -2389,9 +2384,10 @@ base::Optional<ObjectRef> ContextRef::get(int index,
   return base::nullopt;
 }
 
-JSHeapBroker::JSHeapBroker(Isolate* isolate, Zone* broker_zone,
-                           bool tracing_enabled, bool is_concurrent_inlining,
-                           bool is_native_context_independent)
+JSHeapBroker::JSHeapBroker(
+    Isolate* isolate, Zone* broker_zone, bool tracing_enabled,
+    bool is_concurrent_inlining, bool is_native_context_independent,
+    std::unique_ptr<PersistentHandles> persistent_handles)
     : isolate_(isolate),
       zone_(broker_zone),
       refs_(zone()->New<RefsMap>(kMinimalRefsBucketCount, AddressMatcher(),
@@ -2401,6 +2397,8 @@ JSHeapBroker::JSHeapBroker(Isolate* isolate, Zone* broker_zone,
       tracing_enabled_(tracing_enabled),
       is_concurrent_inlining_(is_concurrent_inlining),
       is_native_context_independent_(is_native_context_independent),
+      ph_(std::move(persistent_handles)),
+      local_heap_(base::nullopt),
       feedback_(zone()),
       bytecode_analyses_(zone()),
       property_access_infos_(zone()),
@@ -2414,6 +2412,8 @@ JSHeapBroker::JSHeapBroker(Isolate* isolate, Zone* broker_zone,
   TRACE(this, "Constructing heap broker");
 }
 
+JSHeapBroker::~JSHeapBroker() { DCHECK(!local_heap_); }
+
 std::string JSHeapBroker::Trace() const {
   std::ostringstream oss;
   oss << "[" << this << "] ";
@@ -2421,6 +2421,19 @@ std::string JSHeapBroker::Trace() const {
   return oss.str();
 }
 
+void JSHeapBroker::InitializeLocalHeap() {
+  DCHECK(ph_);
+  DCHECK(!local_heap_);
+  local_heap_.emplace(isolate_->heap(), std::move(ph_));
+}
+
+void JSHeapBroker::TearDownLocalHeap() {
+  DCHECK(!ph_);
+  DCHECK(local_heap_);
+  ph_ = local_heap_->DetachPersistentHandles();
+  local_heap_.reset();
+}
+
 void JSHeapBroker::StopSerializing() {
   CHECK_EQ(mode_, kSerializing);
   TRACE(this, "Stopping serialization");
@@ -3259,7 +3272,7 @@ int BytecodeArrayRef::source_positions_size() const {
                                                       broker()->mode());
     return object()->SourcePositionTableIfCollected().length();
   }
-  return static_cast<int>(data()->AsBytecodeArray()->source_positions_size());
+  return data()->AsBytecodeArray()->source_positions_size();
 }
 
 Address BytecodeArrayRef::handler_table_address() const {

src/compiler/js-heap-broker.h

@@ -14,6 +14,8 @@
 #include "src/compiler/refs-map.h"
 #include "src/compiler/serializer-hints.h"
 #include "src/handles/handles.h"
+#include "src/handles/persistent-handles.h"
+#include "src/heap/local-heap.h"
 #include "src/interpreter/bytecode-array-accessor.h"
 #include "src/objects/feedback-vector.h"
 #include "src/objects/function-kind.h"
@@ -74,13 +76,17 @@ struct PropertyAccessTarget {
 class V8_EXPORT_PRIVATE JSHeapBroker {
  public:
   JSHeapBroker(Isolate* isolate, Zone* broker_zone, bool tracing_enabled,
-               bool is_concurrent_inlining, bool is_native_context_independent);
+               bool is_concurrent_inlining, bool is_native_context_independent,
+               std::unique_ptr<PersistentHandles> persistent_handles);
 
   // For use only in tests, sets default values for some arguments. Avoids
   // churn when new flags are added.
-  JSHeapBroker(Isolate* isolate, Zone* broker_zone)
-      : JSHeapBroker(isolate, broker_zone, FLAG_trace_heap_broker, false,
-                     false) {}
+  JSHeapBroker(Isolate* isolate, Zone* broker_zone,
+               std::unique_ptr<PersistentHandles> persistent_handles)
+      : JSHeapBroker(isolate, broker_zone, FLAG_trace_heap_broker, false, false,
+                     std::move(persistent_handles)) {}
+
+  ~JSHeapBroker();
 
   // The compilation target's native context. We need the setter because at
   // broker construction time we don't yet have the canonical handle.
@@ -101,6 +107,8 @@ class V8_EXPORT_PRIVATE JSHeapBroker {
 
   enum BrokerMode { kDisabled, kSerializing, kSerialized, kRetired };
   BrokerMode mode() const { return mode_; }
+  void InitializeLocalHeap();
+  void TearDownLocalHeap();
   void StopSerializing();
   void Retire();
   bool SerializingAllowed() const;
@@ -202,6 +210,16 @@ class V8_EXPORT_PRIVATE JSHeapBroker {
   bool IsSerializedForCompilation(const SharedFunctionInfoRef& shared,
                                   const FeedbackVectorRef& feedback) const;
 
+  template <typename T>
+  Handle<T> NewPersistentHandle(T obj) {
+    return ph_->NewHandle(obj);
+  }
+
+  template <typename T>
+  Handle<T> NewPersistentHandle(Handle<T> obj) {
+    return ph_->NewHandle(*obj);
+  }
+
   std::string Trace() const;
   void IncrementTracingIndentation();
   void DecrementTracingIndentation();
@@ -255,6 +273,8 @@ class V8_EXPORT_PRIVATE JSHeapBroker {
   bool const tracing_enabled_;
   bool const is_concurrent_inlining_;
   bool const is_native_context_independent_;
+  std::unique_ptr<PersistentHandles> ph_;
+  base::Optional<LocalHeap> local_heap_;
   unsigned trace_indentation_ = 0;
   PerIsolateCompilerCache* compiler_cache_ = nullptr;
   ZoneUnorderedMap<FeedbackSource, ProcessedFeedback const*,

src/compiler/pipeline.cc

@@ -152,7 +152,8 @@ class PipelineData {
         codegen_zone_(codegen_zone_scope_.zone()),
         broker_(new JSHeapBroker(
             isolate_, info_->zone(), info_->trace_heap_broker(),
-            is_concurrent_inlining, info->native_context_independent())),
+            is_concurrent_inlining, info->native_context_independent(),
+            info->DetachPersistentHandles())),
         register_allocation_zone_scope_(zone_stats_,
                                         kRegisterAllocationZoneName),
         register_allocation_zone_(register_allocation_zone_scope_.zone()),
@@ -898,6 +899,19 @@ class PipelineRunScope {
   RuntimeCallTimerScope runtime_call_timer_scope;
 };
 
+// LocalHeapScope encapsulates the liveness of the brokers's LocalHeap.
+class LocalHeapScope {
+ public:
+  explicit LocalHeapScope(JSHeapBroker* broker) : broker_(broker) {
+    broker_->InitializeLocalHeap();
+  }
+
+  ~LocalHeapScope() { broker_->TearDownLocalHeap(); }
+
+ private:
+  JSHeapBroker* broker_;
+};
+
 PipelineStatistics* CreatePipelineStatistics(Handle<Script> script,
                                              OptimizedCompilationInfo* info,
                                              Isolate* isolate,
@@ -1128,6 +1142,7 @@ PipelineCompilationJob::Status PipelineCompilationJob::ExecuteJobImpl(
   // Ensure that the RuntimeCallStats table is only available during execution
   // and not during finalization as that might be on a different thread.
   PipelineJobScope scope(&data_, stats);
+  LocalHeapScope local_heap_scope(data_.broker());
   if (data_.broker()->is_concurrent_inlining()) {
     if (!pipeline_.CreateGraph()) {
       return AbortOptimization(BailoutReason::kGraphBuildingFailed);
@@ -2834,6 +2849,7 @@ MaybeHandle<Code> Pipeline::GenerateCodeForTesting(
   Deoptimizer::EnsureCodeForDeoptimizationEntries(isolate);
 
   pipeline.Serialize();
+  LocalHeapScope local_heap_scope(data.broker());
   if (!pipeline.CreateGraph()) return MaybeHandle<Code>();
   if (!pipeline.OptimizeGraph(&linkage)) return MaybeHandle<Code>();
   pipeline.AssembleCode(&linkage);

src/handles/persistent-handles-inl.h

-// Copyright 2020 the V8 project authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef V8_HANDLES_PERSISTENT_HANDLES_INL_H_
-#define V8_HANDLES_PERSISTENT_HANDLES_INL_H_
-
-#include "src/handles/persistent-handles.h"
-
-namespace v8 {
-namespace internal {
-
-template <typename T>
-Handle<T> PersistentHandles::NewHandle(T obj) {
-#ifdef DEBUG
-  CheckOwnerIsParked();
-#endif
-  return Handle<T>(GetHandle(obj.ptr()));
-}
-
-template <typename T>
-Handle<T> PersistentHandles::NewHandle(Handle<T> obj) {
-  return NewHandle(*obj);
-}
-
-}  // namespace internal
-}  // namespace v8
-
-#endif  // V8_HANDLES_PERSISTENT_HANDLES_INL_H_

src/handles/persistent-handles.cc

@@ -41,7 +41,7 @@ void PersistentHandles::Detach() {
   owner_ = nullptr;
 }
 
-void PersistentHandles::CheckOwnerIsParked() {
+void PersistentHandles::CheckOwnerIsNotParked() {
   if (owner_) DCHECK(!owner_->IsParked());
 }
 

src/handles/persistent-handles.h

@@ -32,10 +32,17 @@ class PersistentHandles {
   void Iterate(RootVisitor* visitor);
 
   template <typename T>
-  inline Handle<T> NewHandle(T object);
+  Handle<T> NewHandle(T obj) {
+#ifdef DEBUG
+    CheckOwnerIsNotParked();
+#endif
+    return Handle<T>(GetHandle(obj.ptr()));
+  }
 
   template <typename T>
-  inline Handle<T> NewHandle(Handle<T> object);
+  Handle<T> NewHandle(Handle<T> obj) {
+    return NewHandle(*obj);
+  }
 
 #ifdef DEBUG
   bool Contains(Address* location);
@@ -48,7 +55,7 @@ class PersistentHandles {
 #ifdef DEBUG
   void Attach(LocalHeap* local_heap);
   void Detach();
-  V8_EXPORT_PRIVATE void CheckOwnerIsParked();
+  V8_EXPORT_PRIVATE void CheckOwnerIsNotParked();
 
   LocalHeap* owner_ = nullptr;
 

src/heap/local-heap-inl.h

@@ -5,7 +5,6 @@
 #ifndef V8_HEAP_LOCAL_HEAP_INL_H_
 #define V8_HEAP_LOCAL_HEAP_INL_H_
 
-#include "src/handles/persistent-handles-inl.h"
 #include "src/handles/persistent-handles.h"
 #include "src/heap/local-heap.h"
 

test/cctest/compiler/test-js-constant-cache.cc

@@ -38,7 +38,8 @@ class JSConstantCacheTester : public HandleAndZoneScope,
         JSGraph(main_isolate(), &main_graph_, &main_common_, &main_javascript_,
                 nullptr, &main_machine_),
         canonical_(main_isolate()),
-        broker_(main_isolate(), main_zone()) {
+        broker_(main_isolate(), main_zone(),
+                main_isolate()->NewPersistentHandles()) {
     main_graph_.SetStart(main_graph_.NewNode(common()->Start(0)));
     main_graph_.SetEnd(
         main_graph_.NewNode(common()->End(1), main_graph_.start()));

test/cctest/compiler/test-js-context-specialization.cc

@@ -33,7 +33,8 @@ class ContextSpecializationTester : public HandleAndZoneScope {
         jsgraph_(main_isolate(), graph(), common(), &javascript_, &simplified_,
                  &machine_),
         reducer_(main_zone(), graph(), &tick_counter_),
-        js_heap_broker_(main_isolate(), main_zone()),
+        js_heap_broker_(main_isolate(), main_zone(),
+                        main_isolate()->NewPersistentHandles()),
         spec_(&reducer_, jsgraph(), &js_heap_broker_, context,
               MaybeHandle<JSFunction>()) {}
 

test/cctest/compiler/test-js-typed-lowering.cc

@@ -27,7 +27,7 @@ class JSTypedLoweringTester : public HandleAndZoneScope {
   explicit JSTypedLoweringTester(int num_parameters = 0)
       : isolate(main_isolate()),
         canonical(isolate),
-        js_heap_broker(isolate, main_zone()),
+        js_heap_broker(isolate, main_zone(), isolate->NewPersistentHandles()),
         binop(nullptr),
         unop(nullptr),
         javascript(main_zone()),

test/cctest/compiler/test-representation-change.cc

@@ -27,7 +27,8 @@ class RepresentationChangerTester : public HandleAndZoneScope,
         javascript_(main_zone()),
         jsgraph_(main_isolate(), main_graph_, &main_common_, &javascript_,
                  &main_simplified_, &main_machine_),
-        broker_{main_isolate(), main_zone()},
+        broker_(main_isolate(), main_zone(),
+                main_isolate()->NewPersistentHandles()),
         changer_(&jsgraph_, &broker_) {
     Node* s = graph()->NewNode(common()->Start(num_parameters));
     graph()->SetStart(s);

test/cctest/test-concurrent-descriptor-array.cc

@@ -6,7 +6,6 @@
 #include "src/base/platform/semaphore.h"
 #include "src/handles/handles-inl.h"
 #include "src/handles/local-handles-inl.h"
-#include "src/handles/persistent-handles-inl.h"
 #include "src/handles/persistent-handles.h"
 #include "src/heap/heap.h"
 #include "src/heap/local-heap-inl.h"

test/cctest/test-concurrent-prototype.cc

@@ -6,7 +6,6 @@
 #include "src/base/platform/semaphore.h"
 #include "src/handles/handles-inl.h"
 #include "src/handles/local-handles-inl.h"
-#include "src/handles/persistent-handles-inl.h"
 #include "src/handles/persistent-handles.h"
 #include "src/heap/heap.h"
 #include "src/heap/local-heap-inl.h"

test/cctest/test-persistent-handles.cc

@@ -10,7 +10,6 @@
 #include "src/base/platform/semaphore.h"
 #include "src/handles/handles-inl.h"
 #include "src/handles/local-handles-inl.h"
-#include "src/handles/persistent-handles-inl.h"
 #include "src/handles/persistent-handles.h"
 #include "src/heap/heap.h"
 #include "src/heap/local-heap-inl.h"

test/common/types-fuzz.h

@@ -41,7 +41,9 @@ namespace compiler {
 class Types {
  public:
   Types(Zone* zone, Isolate* isolate, v8::base::RandomNumberGenerator* rng)
-      : zone_(zone), js_heap_broker_(isolate, zone), rng_(rng) {
+      : zone_(zone),
+        js_heap_broker_(isolate, zone, isolate->NewPersistentHandles()),
+        rng_(rng) {
 #define DECLARE_TYPE(name, value) \
   name = Type::name();            \
   types.push_back(name);

test/unittests/compiler/common-operator-reducer-unittest.cc

@@ -29,7 +29,7 @@ class CommonOperatorReducerTest : public GraphTest {
   Reduction Reduce(
       AdvancedReducer::Editor* editor, Node* node,
       MachineOperatorBuilder::Flags flags = MachineOperatorBuilder::kNoFlags) {
-    JSHeapBroker broker(isolate(), zone());
+    JSHeapBroker broker(isolate(), zone(), isolate()->NewPersistentHandles());
     MachineOperatorBuilder machine(zone(), MachineType::PointerRepresentation(),
                                    flags);
     CommonOperatorReducer reducer(editor, graph(), &broker, common(), &machine,

test/unittests/compiler/constant-folding-reducer-unittest.cc

@@ -63,7 +63,7 @@ class ConstantFoldingReducerTest : public TypedGraphTest {
  public:
   ConstantFoldingReducerTest()
       : TypedGraphTest(3),
-        broker_(isolate(), zone()),
+        broker_(isolate(), zone(), isolate()->NewPersistentHandles()),
         simplified_(zone()),
         deps_(&broker_, zone()) {}
   ~ConstantFoldingReducerTest() override = default;

test/unittests/compiler/graph-unittest.cc

@@ -18,7 +18,7 @@ GraphTest::GraphTest(int num_parameters)
     : canonical_(isolate()),
       common_(zone()),
       graph_(zone()),
-      broker_(isolate(), zone()),
+      broker_(isolate(), zone(), isolate()->NewPersistentHandles()),
       source_positions_(&graph_),
       node_origins_(&graph_) {
   graph()->SetStart(graph()->NewNode(common()->Start(num_parameters)));

test/unittests/compiler/simplified-operator-reducer-unittest.cc

@@ -30,7 +30,7 @@ class SimplifiedOperatorReducerTest : public GraphTest {
 
  protected:
   Reduction Reduce(Node* node) {
-    JSHeapBroker broker(isolate(), zone());
+    JSHeapBroker broker(isolate(), zone(), isolate()->NewPersistentHandles());
     MachineOperatorBuilder machine(zone());
     JSOperatorBuilder javascript(zone());
     JSGraph jsgraph(isolate(), graph(), common(), &javascript, simplified(),

test/unittests/compiler/typer-unittest.cc

@@ -22,7 +22,7 @@ class TyperTest : public TypedGraphTest {
  public:
   TyperTest()
       : TypedGraphTest(3),
-        broker_(isolate(), zone()),
+        broker_(isolate(), zone(), isolate()->NewPersistentHandles()),
         operation_typer_(&broker_, zone()),
         types_(zone(), isolate(), random_number_generator()),
         javascript_(zone()),