[inspector] Make collectGarbage of HeapProfiler precise

Instead of forcing GC right away, the function now post a task and performance GC from the task with an empty stack to avoid false positive pointers in conservative stack scanning. Bug: chromium:1098187 Change-Id: I88864845a1e395056c5d5f6e867ad774b87dbb6a Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2307217 Commit-Queue: Ulan Degenbaev <[email protected]> Reviewed-by: Simon Zünd <[email protected]> Reviewed-by: Peter Marshall <[email protected]> Cr-Commit-Position: refs/heads/master@{#69444}
Mikefotiaoqing · Aug 18, 2020 · d1070e4 · d1070e4
1 parent 12b88d8
commit d1070e4
Show file tree

Hide file tree

Showing 15 changed files with 124 additions and 11 deletions.
diff --git a/src/api/api.cc b/src/api/api.cc
@@ -9788,6 +9788,14 @@ v8::Platform* debug::GetCurrentPlatform() {
   return i::V8::GetCurrentPlatform();
 }
 
+void debug::ForceGarbageCollection(
+    v8::Isolate* isolate,
+    v8::EmbedderHeapTracer::EmbedderStackState embedder_stack_state) {
+  i::Heap* heap = reinterpret_cast<i::Isolate*>(isolate)->heap();
+  heap->SetEmbedderStackStateForNextFinalizaton(embedder_stack_state);
+  isolate->LowMemoryNotification();
+}
+
 debug::WasmScript* debug::WasmScript::Cast(debug::Script* script) {
   CHECK(script->IsWasm());
   return static_cast<WasmScript*>(script);

diff --git a/src/debug/debug-interface.h b/src/debug/debug-interface.h
@@ -521,6 +521,10 @@ bool SetFunctionBreakpoint(v8::Local<v8::Function> function,
 
 v8::Platform* GetCurrentPlatform();
 
+void ForceGarbageCollection(
+    v8::Isolate* isolate,
+    v8::EmbedderHeapTracer::EmbedderStackState embedder_stack_state);
+
 class PostponeInterruptsScope {
  public:
   explicit PostponeInterruptsScope(v8::Isolate* isolate);

diff --git a/src/inspector/inspector_protocol_config.json b/src/inspector/inspector_protocol_config.json
@@ -25,7 +25,8 @@
                 "domain": "Profiler"
             },
             {
-                "domain": "HeapProfiler"
+                "domain": "HeapProfiler",
+                "async": ["collectGarbage"]
             }
         ]
     },

diff --git a/src/inspector/v8-heap-profiler-agent-impl.cc b/src/inspector/v8-heap-profiler-agent-impl.cc
@@ -4,6 +4,10 @@
 
 #include "src/inspector/v8-heap-profiler-agent-impl.h"
 
+#include "include/v8-inspector.h"
+#include "include/v8-profiler.h"
+#include "include/v8-version.h"
+#include "src/base/platform/mutex.h"
 #include "src/inspector/injected-script.h"
 #include "src/inspector/inspected-context.h"
 #include "src/inspector/protocol/Protocol.h"
@@ -12,10 +16,6 @@
 #include "src/inspector/v8-inspector-impl.h"
 #include "src/inspector/v8-inspector-session-impl.h"
 
-#include "include/v8-inspector.h"
-#include "include/v8-profiler.h"
-#include "include/v8-version.h"
-
 namespace v8_inspector {
 
 namespace {
@@ -143,16 +143,51 @@ class HeapStatsStream final : public v8::OutputStream {
 
 }  // namespace
 
+struct V8HeapProfilerAgentImpl::AsyncGC {
+  v8::base::Mutex m_mutex;
+  bool m_canceled = false;
+  bool m_pending = false;
+  std::vector<std::unique_ptr<CollectGarbageCallback>> m_pending_callbacks;
+};
+
+class V8HeapProfilerAgentImpl::GCTask : public v8::Task {
+ public:
+  GCTask(v8::Isolate* isolate, std::shared_ptr<AsyncGC> async_gc)
+      : m_isolate(isolate), m_async_gc(async_gc) {}
+
+  void Run() override {
+    std::shared_ptr<AsyncGC> async_gc = m_async_gc.lock();
+    if (!async_gc) return;
+    v8::base::MutexGuard lock(&async_gc->m_mutex);
+    if (async_gc->m_canceled) return;
+    v8::debug::ForceGarbageCollection(
+        m_isolate, v8::EmbedderHeapTracer::EmbedderStackState::kNoHeapPointers);
+    for (auto& callback : async_gc->m_pending_callbacks) {
+      callback->sendSuccess();
+    }
+    async_gc->m_pending_callbacks.clear();
+  }
+
+ private:
+  v8::Isolate* m_isolate;
+  std::weak_ptr<AsyncGC> m_async_gc;
+};
+
 V8HeapProfilerAgentImpl::V8HeapProfilerAgentImpl(
     V8InspectorSessionImpl* session, protocol::FrontendChannel* frontendChannel,
     protocol::DictionaryValue* state)
     : m_session(session),
       m_isolate(session->inspector()->isolate()),
       m_frontend(frontendChannel),
       m_state(state),
-      m_hasTimer(false) {}
+      m_hasTimer(false),
+      m_async_gc(std::make_shared<AsyncGC>()) {}
 
-V8HeapProfilerAgentImpl::~V8HeapProfilerAgentImpl() = default;
+V8HeapProfilerAgentImpl::~V8HeapProfilerAgentImpl() {
+  v8::base::MutexGuard lock(&m_async_gc->m_mutex);
+  m_async_gc->m_canceled = true;
+  m_async_gc->m_pending_callbacks.clear();
+}
 
 void V8HeapProfilerAgentImpl::restore() {
   if (m_state->booleanProperty(HeapProfilerAgentState::heapProfilerEnabled,
@@ -171,9 +206,15 @@ void V8HeapProfilerAgentImpl::restore() {
   }
 }
 
-Response V8HeapProfilerAgentImpl::collectGarbage() {
-  m_isolate->LowMemoryNotification();
-  return Response::Success();
+void V8HeapProfilerAgentImpl::collectGarbage(
+    std::unique_ptr<CollectGarbageCallback> callback) {
+  v8::base::MutexGuard lock(&m_async_gc->m_mutex);
+  m_async_gc->m_pending_callbacks.push_back(std::move(callback));
+  if (!m_async_gc->m_pending) {
+    v8::debug::GetCurrentPlatform()
+        ->GetForegroundTaskRunner(m_isolate)
+        ->PostNonNestableTask(std::make_unique<GCTask>(m_isolate, m_async_gc));
+  }
 }
 
 Response V8HeapProfilerAgentImpl::startTrackingHeapObjects(

diff --git a/src/inspector/v8-heap-profiler-agent-impl.h b/src/inspector/v8-heap-profiler-agent-impl.h
@@ -27,7 +27,8 @@ class V8HeapProfilerAgentImpl : public protocol::HeapProfiler::Backend {
   ~V8HeapProfilerAgentImpl() override;
   void restore();
 
-  Response collectGarbage() override;
+  void collectGarbage(
+      std::unique_ptr<CollectGarbageCallback> callback) override;
 
   Response enable() override;
   Response startTrackingHeapObjects(Maybe<bool> trackAllocations) override;
@@ -55,6 +56,9 @@ class V8HeapProfilerAgentImpl : public protocol::HeapProfiler::Backend {
       std::unique_ptr<protocol::HeapProfiler::SamplingHeapProfile>*) override;
 
  private:
+  struct AsyncGC;
+  class GCTask;
+
   void startTrackingHeapObjectsInternal(bool trackAllocations);
   void stopTrackingHeapObjectsInternal();
   void requestHeapStatsUpdate();
@@ -65,6 +69,7 @@ class V8HeapProfilerAgentImpl : public protocol::HeapProfiler::Backend {
   protocol::HeapProfiler::Frontend m_frontend;
   protocol::DictionaryValue* m_state;
   bool m_hasTimer;
+  std::shared_ptr<AsyncGC> m_async_gc;
 
   DISALLOW_COPY_AND_ASSIGN(V8HeapProfilerAgentImpl);
 };

diff --git a/test/inspector/cpu-profiler/coverage-block.js b/test/inspector/cpu-profiler/coverage-block.js
@@ -4,6 +4,7 @@
 
 // Flags: --allow-natives-syntax --no-always-opt --opt
 // Flags: --no-stress-flush-bytecode
+// Flags: --no-stress-incremental-marking
 
 var source =
 `

diff --git a/test/inspector/cpu-profiler/coverage.js b/test/inspector/cpu-profiler/coverage.js
@@ -4,6 +4,7 @@
 
 // Flags: --allow-natives-syntax --no-always-opt --opt
 // Flags: --no-stress-flush-bytecode
+// Flags: --no-stress-incremental-marking
 
 var source =
 `

diff --git a/test/inspector/debugger/limit-size-of-collected-scripts.js b/test/inspector/debugger/limit-size-of-collected-scripts.js
@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// Flags: --no-stress-incremental-marking
+
 let {session, contextGroup, Protocol} = InspectorTest.start(
     'Checks that inspector does not retain old collected scripts.\n');
 

diff --git a/test/inspector/debugger/not-hold-promises.js b/test/inspector/debugger/not-hold-promises.js
@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// Flags: --no-stress-incremental-marking
+
 let {session, contextGroup, Protocol} =
   InspectorTest.start('Tests that we don\'t hold promises.');
 

diff --git a/test/inspector/debugger/script-on-after-compile.js b/test/inspector/debugger/script-on-after-compile.js
@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// Flags: --no-stress-incremental-marking
+
 let {session, contextGroup, Protocol} = InspectorTest.start("Checks that inspector correctly process compiled scripts");
 
 function addScripts() {

diff --git a/test/inspector/heap-profiler/collect-garbage-expected.txt b/test/inspector/heap-profiler/collect-garbage-expected.txt
@@ -0,0 +1,4 @@
+Tests collectGarbage.
+
+Running test: testCollectGarbage
+WeakRef state: WeakRef is cleared after GC.
diff --git a/test/inspector/heap-profiler/collect-garbage.js b/test/inspector/heap-profiler/collect-garbage.js
@@ -0,0 +1,30 @@
+// 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.
+
+// Flags: --no-stress-incremental-marking
+
+let {session, contextGroup, Protocol} = InspectorTest.start(
+    'Tests collectGarbage.');
+
+contextGroup.addScript(`
+function createWeakRef() {
+  globalThis.weak_ref = new WeakRef(new Array(1000).fill(0));
+}
+function getWeakRef() {
+  if (!globalThis.weak_ref.deref()) return 'WeakRef is cleared after GC.';
+  return 'WeakRef is not cleared. GC did not happen?'
+}
+//# sourceURL=test.js`);
+
+Protocol.Debugger.enable();
+Protocol.HeapProfiler.enable();
+
+InspectorTest.runAsyncTestSuite([
+  async function testCollectGarbage() {
+    await Protocol.Runtime.evaluate({ expression: 'createWeakRef()' });
+    await Protocol.HeapProfiler.collectGarbage();
+    let weak_ref = await Protocol.Runtime.evaluate({ expression: 'getWeakRef()' });
+    InspectorTest.log(`WeakRef state: ${weak_ref.result.result.value}`);
+  }
+]);
diff --git a/test/inspector/inspector.status b/test/inspector/inspector.status
@@ -100,14 +100,22 @@
 ##############################################################################
 ['gc_stress or gc_fuzzer or variant == stress_incremental_marking', {
   # Skip tests that fail with GC stress: https://crbug.com/v8/10748
+  'cpu-profiler/coverage': [SKIP],
+  'cpu-profiler/coverage-block': [SKIP],
   'debugger/get-possible-breakpoints': [SKIP],
   'debugger/get-possible-breakpoints-array-literal': [SKIP],
   'debugger/get-possible-breakpoints-master': [SKIP],
+  'debugger/limit-size-of-collected-scripts': [SKIP],
+  'debugger/not-hold-promises': [SKIP],
   'debugger/regression-424142': [SKIP],
   'debugger/return-break-locations': [SKIP],
+  'debugger/script-on-after-compile': [SKIP],
   'debugger/set-breakpoint-at-last-line': [SKIP],
   'debugger/set-breakpoint-breaks-on-first-breakable-location': [SKIP],
+  'heap-profiler/collect-garbage' : [SKIP],
   'runtime-call-stats/collection': [SKIP],
+  'runtime/context-destroyed-on-context-collected': [SKIP],
+  'runtime/evaluate-async': [SKIP],
   'runtime/internal-properties-entries': [SKIP],
   'type-profiler/type-profile-start-stop': [SKIP],
 }], # gc_stress

diff --git a/test/inspector/runtime/context-destroyed-on-context-collected.js b/test/inspector/runtime/context-destroyed-on-context-collected.js
@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// Flags: --no-stress-incremental-marking
+
 let {session, contextGroup, Protocol} =
   InspectorTest.start('Tests that contextDesrtoyed nofitication is fired when context is collected.');
 

diff --git a/test/inspector/runtime/evaluate-async.js b/test/inspector/runtime/evaluate-async.js
@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// Flags: --no-stress-incremental-marking
+
 let {session, contextGroup, Protocol} = InspectorTest.start("Tests that Runtime.evaluate works with awaitPromise flag.");
 
 contextGroup.addScript(`