[cpu-profiler] Clear the CodeMap after the last profile to stop leak

We kept the CodeMap filled with entries between profiles, even in
kLazyLogging mode which will re-fill the CodeMap when profiling starts
again. See the bug for more details.

This fix manually clears the CodeMap after the last profile is deleted.
We already call DisableLogging() when the last profile is stopped. At
this point we still need the CodeMap alive because the profile object
we expose via the API is backed by the CodeEntry objects in the CodeMap.
Once the last profile is deleted though, we can empty the CodeMap.

There is still another bug, which is that we never delete CodeEntry
objects for deleted code, as there are no CodeDeleteEvents from the GC.
We will work on that separately, but this fix should stop those leaks
accumulating between profiles as we wipe the CodeMap entirely between
profiles (at least for kLazyLogging mode). kEagerLogging mode still has
this problem and will only be fixed by introducing CodeDelete events or
similar.

Bug: v8:11051
Change-Id: Iab9570747d17c657e6e318d434f935af8047d05f
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2491033
Commit-Queue: Peter Marshall <petermarshall@chromium.org>
Reviewed-by: Simon Zünd <szuend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#70792}

src/profiler/cpu-profiler.cc

@@ -326,6 +326,8 @@ ProfilerCodeObserver::ProfilerCodeObserver(Isolate* isolate)
   LogBuiltins();
 }
 
+void ProfilerCodeObserver::ClearCodeMap() { code_map_.Clear(); }
+
 void ProfilerCodeObserver::CodeEventHandler(
     const CodeEventsContainer& evt_rec) {
   if (processor_) {
@@ -487,7 +489,10 @@ void CpuProfiler::ResetProfiles() {
   profiles_.reset(new CpuProfilesCollection(isolate_));
   profiles_->set_cpu_profiler(this);
   symbolizer_.reset();
-  if (!profiling_scope_) profiler_listener_.reset();
+  if (!profiling_scope_) {
+    profiler_listener_.reset();
+    code_observer_.ClearCodeMap();
+  }
 }
 
 void CpuProfiler::EnableLogging() {

src/profiler/cpu-profiler.h

@@ -246,6 +246,7 @@ class V8_EXPORT_PRIVATE ProfilerCodeObserver : public CodeEventObserver {
   void CodeEventHandler(const CodeEventsContainer& evt_rec) override;
 
   CodeMap* code_map() { return &code_map_; }
+  void ClearCodeMap();
 
  private:
   friend class ProfilerEventsProcessor;
@@ -331,6 +332,7 @@ class V8_EXPORT_PRIVATE CpuProfiler {
   ProfilerListener* profiler_listener_for_test() const {
     return profiler_listener_.get();
   }
+  CodeMap* code_map_for_test() { return code_observer_.code_map(); }
 
  private:
   void StartProcessorIfNotStarted();

src/profiler/profile-generator.cc

@@ -644,7 +644,9 @@ void CpuProfile::Print() const {
 
 CodeMap::CodeMap() = default;
 
-CodeMap::~CodeMap() {
+CodeMap::~CodeMap() { Clear(); }
+
+void CodeMap::Clear() {
   // First clean the free list as it's otherwise impossible to tell
   // the slot type.
   unsigned free_slot = free_list_head_;
@@ -654,6 +656,10 @@ CodeMap::~CodeMap() {
     free_slot = next_slot;
   }
   for (auto slot : code_entries_) delete slot.entry;
+
+  code_entries_.clear();
+  code_map_.clear();
+  free_list_head_ = kNoFreeSlot;
 }
 
 void CodeMap::AddCode(Address addr, CodeEntry* entry, unsigned size) {

src/profiler/profile-generator.h

@@ -412,6 +412,8 @@ class V8_EXPORT_PRIVATE CodeMap {
   CodeEntry* FindEntry(Address addr, Address* out_instruction_start = nullptr);
   void Print();
 
+  void Clear();
+
  private:
   struct CodeEntryMapInfo {
     unsigned index;
@@ -431,6 +433,7 @@ class V8_EXPORT_PRIVATE CodeMap {
 
   CodeEntry* entry(unsigned index) { return code_entries_[index].entry; }
 
+  // Added state here needs to be dealt with in Clear() as well.
   std::deque<CodeEntrySlotInfo> code_entries_;
   std::map<Address, CodeEntryMapInfo> code_map_;
   unsigned free_list_head_ = kNoFreeSlot;

test/cctest/test-cpu-profiler.cc

@@ -279,6 +279,94 @@ TEST(TickEvents) {
   CHECK(top_down_ddd_children->empty());
 }
 
+TEST(CodeMapClearedBetweenProfilesWithLazyLogging) {
+  TestSetup test_setup;
+  LocalContext env;
+  i::Isolate* isolate = CcTest::i_isolate();
+  i::HandleScope scope(isolate);
+
+  // This gets logged when the profiler starts up and scans the heap.
+  i::Handle<i::AbstractCode> code1(CreateCode(&env), isolate);
+
+  CpuProfiler profiler(isolate, kDebugNaming, kLazyLogging);
+  profiler.StartProfiling("");
+
+  CpuProfile* profile = profiler.StopProfiling("");
+  CHECK(profile);
+
+  // Check that our code is still in the code map.
+  CodeMap* code_map = profiler.code_map_for_test();
+  CodeEntry* code1_entry = code_map->FindEntry(code1->InstructionStart());
+  CHECK(code1_entry);
+  CHECK_EQ(0, strcmp("function_1", code1_entry->name()));
+
+  profiler.DeleteProfile(profile);
+
+  // Check that the code map is emptied once the last profile is deleted.
+  CHECK(!code_map->FindEntry(code1->InstructionStart()));
+
+  // Create code between profiles. This should not be logged yet.
+  i::Handle<i::AbstractCode> code2(CreateCode(&env), isolate);
+
+  CHECK(!code_map->FindEntry(code2->InstructionStart()));
+}
+
+TEST(CodeMapNotClearedBetweenProfilesWithEagerLogging) {
+  TestSetup test_setup;
+  LocalContext env;
+  i::Isolate* isolate = CcTest::i_isolate();
+  i::HandleScope scope(isolate);
+
+  // This gets logged when the profiler starts up and scans the heap.
+  i::Handle<i::AbstractCode> code1(CreateCode(&env), isolate);
+
+  CpuProfiler profiler(isolate, kDebugNaming, kEagerLogging);
+  profiler.StartProfiling("");
+
+  CpuProfile* profile = profiler.StopProfiling("");
+  CHECK(profile);
+
+  // Check that our code is still in the code map.
+  CodeMap* code_map = profiler.code_map_for_test();
+  CodeEntry* code1_entry = code_map->FindEntry(code1->InstructionStart());
+  CHECK(code1_entry);
+  CHECK_EQ(0, strcmp("function_1", code1_entry->name()));
+
+  profiler.DeleteProfile(profile);
+
+  // We should still have an entry in kEagerLogging mode.
+  code1_entry = code_map->FindEntry(code1->InstructionStart());
+  CHECK(code1_entry);
+  CHECK_EQ(0, strcmp("function_1", code1_entry->name()));
+
+  // Create code between profiles. This should be logged too.
+  i::Handle<i::AbstractCode> code2(CreateCode(&env), isolate);
+  CHECK(code_map->FindEntry(code2->InstructionStart()));
+
+  profiler.StartProfiling("");
+  CpuProfile* profile2 = profiler.StopProfiling("");
+  CHECK(profile2);
+
+  // Check that we still have code map entries for both code objects.
+  code1_entry = code_map->FindEntry(code1->InstructionStart());
+  CHECK(code1_entry);
+  CHECK_EQ(0, strcmp("function_1", code1_entry->name()));
+  CodeEntry* code2_entry = code_map->FindEntry(code2->InstructionStart());
+  CHECK(code2_entry);
+  CHECK_EQ(0, strcmp("function_2", code2_entry->name()));
+
+  profiler.DeleteProfile(profile2);
+
+  // Check that we still have code map entries for both code objects, even after
+  // the last profile is deleted.
+  code1_entry = code_map->FindEntry(code1->InstructionStart());
+  CHECK(code1_entry);
+  CHECK_EQ(0, strcmp("function_1", code1_entry->name()));
+  code2_entry = code_map->FindEntry(code2->InstructionStart());
+  CHECK(code2_entry);
+  CHECK_EQ(0, strcmp("function_2", code2_entry->name()));
+}
+
 // http://crbug/51594
 // This test must not crash.
 TEST(CrashIfStoppingLastNonExistentProfile) {

test/cctest/test-profile-generator.cc

@@ -356,6 +356,21 @@ TEST(CodeMapMoveAndDeleteCode) {
   CHECK_EQ(entry3, code_map.FindEntry(ToAddress(0x1750)));
 }
 
+TEST(CodeMapClear) {
+  CodeMap code_map;
+  CodeEntry* entry1 = new CodeEntry(i::CodeEventListener::FUNCTION_TAG, "aaa");
+  CodeEntry* entry2 = new CodeEntry(i::CodeEventListener::FUNCTION_TAG, "bbb");
+  code_map.AddCode(ToAddress(0x1500), entry1, 0x200);
+  code_map.AddCode(ToAddress(0x1700), entry2, 0x100);
+
+  code_map.Clear();
+  CHECK(!code_map.FindEntry(ToAddress(0x1500)));
+  CHECK(!code_map.FindEntry(ToAddress(0x1700)));
+
+  // Check that Clear() doesn't cause issues if called twice.
+  code_map.Clear();
+}
+
 namespace {
 
 class TestSetup {