Revert "[cpu-profiler] Implement weak phantom finalizers for CodeMap entries"

This reverts commit 3a405b01ba.

Reason for revert: thread-sanitizer failures on Linux64 TSAN bot:
https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Linux64%20TSAN/35141/overview

Original change's description:
> [cpu-profiler] Implement weak phantom finalizers for CodeMap entries
>
> Listen to code deletion events by registering finalizers on code
> objects, a first stab at non-leaky long-lived code entries.
>
> Bug: v8:11054
> Change-Id: Ieaaa5b63508263bd261e8385f5bf5dd3baedf9c5
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2628587
> Commit-Queue: Andrew Comminos <acomminos@fb.com>
> Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
> Reviewed-by: Peter Marshall <petermarshall@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#72342}

TBR=ulan@chromium.org,petermarshall@chromium.org,acomminos@fb.com

Change-Id: If22a893af469c9d4d3e00fb124c42cdc52b9a19b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:11054
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2649156
Reviewed-by: Adam Klein <adamk@chromium.org>
Commit-Queue: Adam Klein <adamk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72344}

src/profiler/cpu-profiler-inl.h

@@ -42,9 +42,6 @@ void CodeDeoptEventRecord::UpdateCodeMap(CodeMap* code_map) {
   delete[] deopt_frames;
 }
 
-void CodeSweepEventRecord::UpdateCodeMap(CodeMap* code_map) {
-  code_map->ClearUnused();
-}
 
 void ReportBuiltinEventRecord::UpdateCodeMap(CodeMap* code_map) {
   CodeEntry* entry = code_map->FindEntry(instruction_start);

src/profiler/cpu-profiler.cc

@@ -85,15 +85,6 @@ ProfilingScope::ProfilingScope(Isolate* isolate, ProfilerListener* listener)
   }
   logger->LogCompiledFunctions();
   logger->LogAccessorCallbacks();
-
-  // Trigger dead code to be cleared from the CodeMap on GC.
-  isolate_->heap()->AddGCEpilogueCallback(SweepCallback,
-                                          kGCTypeMarkSweepCompact, listener_);
-}
-
-void ProfilingScope::SweepCallback(v8::Isolate*, v8::GCType,
-                                   v8::GCCallbackFlags, void* listener) {
-  reinterpret_cast<ProfilerListener*>(listener)->CodeSweepEvent();
 }
 
 ProfilingScope::~ProfilingScope() {
@@ -104,7 +95,6 @@ ProfilingScope::~ProfilingScope() {
   profiler_count--;
   isolate_->set_num_cpu_profilers(profiler_count);
   if (profiler_count == 0) isolate_->set_is_profiling(false);
-  isolate_->heap()->RemoveGCEpilogueCallback(SweepCallback, listener_);
 }
 
 ProfilerEventsProcessor::ProfilerEventsProcessor(
@@ -206,7 +196,6 @@ void ProfilerEventsProcessor::CodeEventHandler(
     case CodeEventRecord::CODE_CREATION:
     case CodeEventRecord::CODE_MOVE:
     case CodeEventRecord::CODE_DISABLE_OPT:
-    case CodeEventRecord::CODE_SWEEP:
       Enqueue(evt_rec);
       break;
     case CodeEventRecord::CODE_DEOPT: {

src/profiler/cpu-profiler.h

@@ -34,7 +34,6 @@ class Symbolizer;
   V(CODE_MOVE, CodeMoveEventRecord)              \
   V(CODE_DISABLE_OPT, CodeDisableOptEventRecord) \
   V(CODE_DEOPT, CodeDeoptEventRecord)            \
-  V(CODE_SWEEP, CodeSweepEventRecord)            \
   V(REPORT_BUILTIN, ReportBuiltinEventRecord)
 
 class CodeEventRecord {
@@ -89,10 +88,6 @@ class CodeDeoptEventRecord : public CodeEventRecord {
   V8_INLINE void UpdateCodeMap(CodeMap* code_map);
 };
 
-class CodeSweepEventRecord : public CodeEventRecord {
- public:
-  V8_INLINE void UpdateCodeMap(CodeMap* code_map);
-};
 
 class ReportBuiltinEventRecord : public CodeEventRecord {
  public:
@@ -140,9 +135,6 @@ class V8_NODISCARD ProfilingScope {
   ~ProfilingScope();
 
  private:
-  static void SweepCallback(v8::Isolate*, v8::GCType, v8::GCCallbackFlags,
-                            void* listener);
-
   Isolate* const isolate_;
   ProfilerListener* const listener_;
 };

src/profiler/profile-generator.cc

@@ -719,29 +719,9 @@ CodeMap::CodeMap(StringsStorage& function_and_resource_names)
 
 CodeMap::~CodeMap() { Clear(); }
 
-size_t CodeMap::ClearUnused() {
-  size_t num_freed = 0;
-  for (auto it = code_map_.begin(); it != code_map_.end();) {
-    auto* entry = it->second.entry;
-    DCHECK(entry);
-
-    // If a CodeEntry is not used by a profile and its heap object
-    // representation has been deallocated, remove the entry.
-    if (!entry->used() && entry->IsHeapObjectFreed()) {
-      DeleteCodeEntry(entry);
-      it = code_map_.erase(it);
-      num_freed++;
-    } else {
-      it++;
-    }
-  }
-  return num_freed;
-}
-
 void CodeMap::Clear() {
   for (auto& slot : code_map_) {
     if (CodeEntry* entry = slot.second.entry) {
-      entry->UntrackHeapObject();
       entry->ReleaseStrings(function_and_resource_names_);
       delete entry;
     } else {

src/profiler/profile-generator.h

@@ -17,8 +17,6 @@
 #include "include/v8-profiler.h"
 #include "src/base/platform/time.h"
 #include "src/builtins/builtins.h"
-#include "src/execution/isolate.h"
-#include "src/handles/global-handles.h"
 #include "src/logging/code-events.h"
 #include "src/profiler/strings-storage.h"
 #include "src/utils/allocation.h"
@@ -104,8 +102,8 @@ class CodeEntry {
     rare_data_->deopt_reason_ = kNoDeoptReason;
     rare_data_->deopt_id_ = kNoDeoptimizationId;
   }
-  void mark_used() { used_ = true; }
+  void mark_used() { bit_field_ = UsedField::update(bit_field_, true); }
-  bool used() const { return used_; }
+  bool used() const { return UsedField::decode(bit_field_); }
 
   const char* code_type_string() const {
     switch (CodeTypeField::decode(bit_field_)) {
@@ -118,38 +116,8 @@ class CodeEntry {
     }
   }
 
-  bool is_tracking_heap_object() const {
-    return TrackingHeapObjectField::decode(bit_field_);
-  }
-
-  bool IsHeapObjectFreed() const {
-    return is_tracking_heap_object() && heap_object_handle_location_ == nullptr;
-  }
-
   void FillFunctionInfo(SharedFunctionInfo shared);
 
-  // Associates this CodeEntry with the given heap object, taking a weak
-  // reference to it. Must be called on the isolate thread.
-  template <typename T>
-  void TrackHeapObject(Isolate* isolate, Handle<T> object) {
-    DCHECK(!is_tracking_heap_object());
-    bit_field_ = TrackingHeapObjectField::update(bit_field_, true);
-    heap_object_handle_location_ =
-        isolate->global_handles()->Create(*object).location();
-    GlobalHandles::MakeWeak(&heap_object_handle_location_);
-  }
-
-  // Clears out any weak reference set by TrackHeapObject. Must be called on
-  // the isolate thread.
-  void UntrackHeapObject() {
-    if (is_tracking_heap_object()) {
-      bit_field_ = TrackingHeapObjectField::update(bit_field_, false);
-      if (heap_object_handle_location_) {
-        GlobalHandles::ClearWeakness(heap_object_handle_location_);
-      }
-    }
-  }
-
   void SetBuiltinId(Builtins::Name id);
   Builtins::Name builtin_id() const {
     return BuiltinIdField::decode(bit_field_);
@@ -232,23 +200,20 @@ class CodeEntry {
   static_assert(Builtins::builtin_count <= BuiltinIdField::kNumValues,
                 "builtin_count exceeds size of bitfield");
   using CodeTypeField = base::BitField<CodeType, 28, 2>;
-  using TrackingHeapObjectField = base::BitField<bool, 30, 1>;
+  using UsedField = base::BitField<bool, 30, 1>;
   using SharedCrossOriginField = base::BitField<bool, 31, 1>;
 
-  std::uint32_t bit_field_;
+  // Atomic because Used is written from the profiler thread while CodeType is
+  // read from the main thread.
+  std::atomic<std::uint32_t> bit_field_;
   const char* name_;
   const char* resource_name_;
   int line_number_;
   int column_number_;
   int script_id_;
   int position_;
-  // We use a full byte here instead of a bitfield to avoid raciness between
-  // the main thread and profiler thread.
-  // TODO(acomminos): Remove this after atomic reference counting is added.
-  std::uint8_t used_ = false;
   std::unique_ptr<SourcePositionTable> line_info_;
   std::unique_ptr<RareData> rare_data_;
-  Address* heap_object_handle_location_ = nullptr;
 };
 
 struct CodeEntryAndLineNumber {
@@ -444,12 +409,6 @@ class V8_EXPORT_PRIVATE CodeMap {
   CodeEntry* FindEntry(Address addr, Address* out_instruction_start = nullptr);
   void Print();
 
-  // Frees all CodeMap entries that no longer have a heap object, and are not
-  // being included in any profiles.
-  size_t ClearUnused();
-
-  size_t size() const { return code_map_.size(); }
-
   void Clear();
 
  private:

src/profiler/profiler-listener.cc

@@ -43,7 +43,6 @@ void ProfilerListener::CodeCreateEvent(LogEventsAndTags tag,
   rec->entry = new CodeEntry(tag, GetName(name), CodeEntry::kEmptyResourceName,
                              CpuProfileNode::kNoLineNumberInfo,
                              CpuProfileNode::kNoColumnNumberInfo, nullptr);
-  rec->entry->TrackHeapObject(isolate_, code);
   rec->instruction_size = code->InstructionSize();
   DispatchCodeEvent(evt_rec);
 }
@@ -57,7 +56,6 @@ void ProfilerListener::CodeCreateEvent(LogEventsAndTags tag,
   rec->entry = new CodeEntry(tag, GetName(*name), CodeEntry::kEmptyResourceName,
                              CpuProfileNode::kNoLineNumberInfo,
                              CpuProfileNode::kNoColumnNumberInfo, nullptr);
-  rec->entry->TrackHeapObject(isolate_, code);
   rec->instruction_size = code->InstructionSize();
   DispatchCodeEvent(evt_rec);
 }
@@ -75,7 +73,6 @@ void ProfilerListener::CodeCreateEvent(LogEventsAndTags tag,
                              CpuProfileNode::kNoColumnNumberInfo, nullptr);
   DCHECK(!code->IsCode());
   rec->entry->FillFunctionInfo(*shared);
-  rec->entry->TrackHeapObject(isolate_, code);
   rec->instruction_size = code->InstructionSize();
   DispatchCodeEvent(evt_rec);
 }
@@ -198,7 +195,6 @@ void ProfilerListener::CodeCreateEvent(LogEventsAndTags tag,
   }
 
   rec->entry->FillFunctionInfo(*shared);
-  rec->entry->TrackHeapObject(isolate_, abstract_code);
   rec->instruction_size = abstract_code->InstructionSize();
   DispatchCodeEvent(evt_rec);
 }
@@ -303,11 +299,6 @@ void ProfilerListener::CodeDeoptEvent(Handle<Code> code, DeoptimizeKind kind,
   DispatchCodeEvent(evt_rec);
 }
 
-void ProfilerListener::CodeSweepEvent() {
-  CodeEventsContainer evt_rec(CodeEventRecord::CODE_SWEEP);
-  DispatchCodeEvent(evt_rec);
-}
-
 const char* ProfilerListener::GetName(Vector<const char> name) {
   // TODO(all): Change {StringsStorage} to accept non-null-terminated strings.
   OwnedVector<char> null_terminated = OwnedVector<char>::New(name.size() + 1);

src/profiler/profiler-listener.h

@@ -62,7 +62,6 @@ class V8_EXPORT_PRIVATE ProfilerListener : public CodeEventListener {
   void CodeDependencyChangeEvent(Handle<Code> code,
                                  Handle<SharedFunctionInfo> sfi,
                                  const char* reason) override {}
-  void CodeSweepEvent();
 
   const char* GetName(Name name) {
     return function_and_resource_names_.GetName(name);

test/cctest/test-cpu-profiler.cc

@@ -4075,38 +4075,6 @@ TEST(FastApiCPUProfiler) {
 #endif
 }
 
-// Ensure that unused code entries are removed after GC with eager logging.
-TEST(ClearUnusedWithEagerLogging) {
-  ManualGCScope manual_gc;
-  TestSetup test_setup;
-  LocalContext env;
-  i::Isolate* isolate = CcTest::i_isolate();
-  i::HandleScope scope(isolate);
-
-  CpuProfiler profiler(isolate, kDebugNaming, kEagerLogging);
-
-  CodeMap* code_map = profiler.code_map_for_test();
-  size_t initial_size = code_map->size();
-
-  {
-    // Create and run a new script and function, generating 2 code objects.
-    i::HandleScope inner_scope(isolate);
-    CompileRun(
-        "function some_func() {}"
-        "some_func();");
-    CHECK_GT(code_map->size(), initial_size);
-  }
-
-  // Perform a few GCs, ensuring that the executed code's bytecode is flushed.
-  const int kAgingThreshold = 8;
-  for (int i = 0; i < kAgingThreshold; i++) {
-    CcTest::CollectAllGarbage();
-  }
-
-  // Verify that the CodeMap's size is unchanged post-GC.
-  CHECK_EQ(code_map->size(), initial_size);
-}
-
 }  // namespace test_cpu_profiler
 }  // namespace internal
 }  // namespace v8

test/cctest/test-profile-generator.cc

@@ -851,50 +851,6 @@ TEST(NodeSourceTypes) {
   CHECK_EQ(unresolved_node->source_type(), v8::CpuProfileNode::kUnresolved);
 }
 
-TEST(CodeMapClearUnused) {
-  Isolate* isolate = CcTest::i_isolate();
-
-  StringsStorage strings;
-  CodeMap code_map(strings);
-
-  auto* entry = new CodeEntry(i::CodeEventListener::FUNCTION_TAG, "aaa");
-  code_map.AddCode(0, entry, 1);
-
-  // Ensure that CodeEntry objects without a heap object aren't cleared.
-  CHECK_EQ(code_map.ClearUnused(), 0);
-
-  auto* gc_entry_unused =
-      new CodeEntry(i::CodeEventListener::FUNCTION_TAG, "bbb");
-  code_map.AddCode(0x10, gc_entry_unused, 1);
-
-  auto* gc_entry_used =
-      new CodeEntry(i::CodeEventListener::FUNCTION_TAG, "ccc");
-  gc_entry_used->mark_used();
-  code_map.AddCode(0x20, gc_entry_used, 1);
-
-  {
-    HandleScope scope(isolate);
-
-    // Temporarily associate a random heap object with the entries.
-    Handle<FixedArray> stub_object = isolate->factory()->NewFixedArray(1);
-    gc_entry_used->TrackHeapObject(isolate, stub_object);
-    gc_entry_unused->TrackHeapObject(isolate, stub_object);
-
-    // Ensure that the entries aren't dropped while the object is alive.
-    CcTest::CollectAllGarbage();
-    CHECK_EQ(code_map.ClearUnused(), 0);
-    CHECK_EQ(code_map.FindEntry(0x10), gc_entry_unused);
-    CHECK_EQ(code_map.FindEntry(0x20), gc_entry_used);
-  }
-
-  // Ensure that the unused CodeEntry associated with a gc'd address is dropped
-  // after the handle is lost, but the used one isn't.
-  CcTest::CollectAllGarbage();
-  CHECK_EQ(code_map.ClearUnused(), 1);
-  CHECK_EQ(code_map.FindEntry(0x10), nullptr);
-  CHECK_EQ(code_map.FindEntry(0x20), gc_entry_used);
-}
-
 }  // namespace test_profile_generator
 }  // namespace internal
 }  // namespace v8