From 51cdb92b4f904bfbdb66cf103bf23a81528dd0d5 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 24 Nov 2023 22:38:22 +0100 Subject: [PATCH 1/3] deps: V8: cherry-pick 0fd478bcdabd MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Original commit message: [heap-profiler]: expose QueryObjects() to v8::HeapProfiler This allows embedders to use this API for testing memory leaks more reliably. See https://github.com/nodejs/node/pull/50572 for an example about how the API can be used. Change-Id: Ic3d1268e2b331c37e8ec92997b764b9b5486f8c2 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5006373 Reviewed-by: Camillo Bruni Reviewed-by: Simon Zünd Commit-Queue: Joyee Cheung Cr-Commit-Position: refs/heads/main@{#91123} Refs: https://github.com/v8/v8/commit/0fd478bcdabd3400d9d74c47c4883c085ef37d18 --- common.gypi | 2 +- deps/v8/include/v8-profiler.h | 10 ++++ deps/v8/src/api/api.cc | 10 ++++ deps/v8/src/debug/debug-interface.cc | 9 ---- deps/v8/src/debug/debug-interface.h | 10 ---- deps/v8/src/inspector/v8-debugger.cc | 5 +- deps/v8/src/profiler/heap-profiler.cc | 2 +- deps/v8/src/profiler/heap-profiler.h | 5 +- deps/v8/test/cctest/test-heap-profiler.cc | 62 +++++++++++++++++++++++ 9 files changed, 89 insertions(+), 26 deletions(-) diff --git a/common.gypi b/common.gypi index 28522c9fbf83b4..c6b9fa05b44c96 100644 --- a/common.gypi +++ b/common.gypi @@ -36,7 +36,7 @@ # Reset this number to 0 on major V8 upgrades. # Increment by one for each non-official patch applied to deps/v8. - 'v8_embedder_string': '-node.17', + 'v8_embedder_string': '-node.18', ##### V8 defaults for Node.js ##### diff --git a/deps/v8/include/v8-profiler.h b/deps/v8/include/v8-profiler.h index 75c9d6a4ae7863..3ec0cd4bb31318 100644 --- a/deps/v8/include/v8-profiler.h +++ b/deps/v8/include/v8-profiler.h @@ -921,12 +921,22 @@ class V8_EXPORT EmbedderGraph { virtual ~EmbedderGraph() = default; }; +class QueryObjectPredicate { + public: + virtual ~QueryObjectPredicate() = default; + virtual bool Filter(v8::Local object) = 0; +}; + /** * Interface for controlling heap profiling. Instance of the * profiler can be retrieved using v8::Isolate::GetHeapProfiler. */ class V8_EXPORT HeapProfiler { public: + void QueryObjects(v8::Local context, + QueryObjectPredicate* predicate, + std::vector>* objects); + enum SamplingFlags { kSamplingNoFlags = 0, kSamplingForceGC = 1 << 0, diff --git a/deps/v8/src/api/api.cc b/deps/v8/src/api/api.cc index 2708ad9b2e55c7..7807545da57f23 100644 --- a/deps/v8/src/api/api.cc +++ b/deps/v8/src/api/api.cc @@ -10917,6 +10917,16 @@ int HeapProfiler::GetSnapshotCount() { return reinterpret_cast(this)->GetSnapshotsCount(); } +void HeapProfiler::QueryObjects(Local v8_context, + QueryObjectPredicate* predicate, + std::vector>* objects) { + i::Isolate* isolate = reinterpret_cast(v8_context->GetIsolate()); + i::HeapProfiler* profiler = reinterpret_cast(this); + DCHECK_EQ(isolate, profiler->isolate()); + ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate); + profiler->QueryObjects(Utils::OpenHandle(*v8_context), predicate, objects); +} + const HeapSnapshot* HeapProfiler::GetHeapSnapshot(int index) { return reinterpret_cast( reinterpret_cast(this)->GetSnapshot(index)); diff --git a/deps/v8/src/debug/debug-interface.cc b/deps/v8/src/debug/debug-interface.cc index fb3b6b69e4be80..51e3633853ba56 100644 --- a/deps/v8/src/debug/debug-interface.cc +++ b/deps/v8/src/debug/debug-interface.cc @@ -1211,15 +1211,6 @@ v8::MaybeLocal EvaluateGlobalForTesting( RETURN_ESCAPED(result); } -void QueryObjects(v8::Local v8_context, - QueryObjectPredicate* predicate, - std::vector>* objects) { - i::Isolate* isolate = reinterpret_cast(v8_context->GetIsolate()); - ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate); - isolate->heap_profiler()->QueryObjects(Utils::OpenHandle(*v8_context), - predicate, objects); -} - void GlobalLexicalScopeNames(v8::Local v8_context, std::vector>* names) { i::Handle context = Utils::OpenHandle(*v8_context); diff --git a/deps/v8/src/debug/debug-interface.h b/deps/v8/src/debug/debug-interface.h index 5dfd505fed6f14..eb346b326392a7 100644 --- a/deps/v8/src/debug/debug-interface.h +++ b/deps/v8/src/debug/debug-interface.h @@ -530,16 +530,6 @@ class V8_EXPORT_PRIVATE StackTraceIterator { bool throw_on_side_effect) = 0; }; -class QueryObjectPredicate { - public: - virtual ~QueryObjectPredicate() = default; - virtual bool Filter(v8::Local object) = 0; -}; - -void QueryObjects(v8::Local context, - QueryObjectPredicate* predicate, - std::vector>* objects); - void GlobalLexicalScopeNames(v8::Local context, std::vector>* names); diff --git a/deps/v8/src/inspector/v8-debugger.cc b/deps/v8/src/inspector/v8-debugger.cc index 3d4733b5ba936e..d8a08b422505f7 100644 --- a/deps/v8/src/inspector/v8-debugger.cc +++ b/deps/v8/src/inspector/v8-debugger.cc @@ -8,6 +8,7 @@ #include "include/v8-context.h" #include "include/v8-function.h" #include "include/v8-microtask-queue.h" +#include "include/v8-profiler.h" #include "include/v8-util.h" #include "src/inspector/inspected-context.h" #include "src/inspector/protocol/Protocol.h" @@ -38,7 +39,7 @@ void cleanupExpiredWeakPointers(Map& map) { } } -class MatchPrototypePredicate : public v8::debug::QueryObjectPredicate { +class MatchPrototypePredicate : public v8::QueryObjectPredicate { public: MatchPrototypePredicate(V8InspectorImpl* inspector, v8::Local context, @@ -994,7 +995,7 @@ v8::Local V8Debugger::queryObjects(v8::Local context, v8::Isolate* isolate = context->GetIsolate(); std::vector> v8_objects; MatchPrototypePredicate predicate(m_inspector, context, prototype); - v8::debug::QueryObjects(context, &predicate, &v8_objects); + isolate->GetHeapProfiler()->QueryObjects(context, &predicate, &v8_objects); v8::MicrotasksScope microtasksScope(context, v8::MicrotasksScope::kDoNotRunMicrotasks); diff --git a/deps/v8/src/profiler/heap-profiler.cc b/deps/v8/src/profiler/heap-profiler.cc index a23e6d94d1717e..2a3ed138022d5a 100644 --- a/deps/v8/src/profiler/heap-profiler.cc +++ b/deps/v8/src/profiler/heap-profiler.cc @@ -247,7 +247,7 @@ Heap* HeapProfiler::heap() const { return ids_->heap(); } Isolate* HeapProfiler::isolate() const { return heap()->isolate(); } void HeapProfiler::QueryObjects(Handle context, - debug::QueryObjectPredicate* predicate, + v8::QueryObjectPredicate* predicate, std::vector>* objects) { // We need a stack marker here to allow deterministic passes over the stack. // The garbage collection and the two object heap iterators should scan the diff --git a/deps/v8/src/profiler/heap-profiler.h b/deps/v8/src/profiler/heap-profiler.h index 2756ade8141531..851972a729a806 100644 --- a/deps/v8/src/profiler/heap-profiler.h +++ b/deps/v8/src/profiler/heap-profiler.h @@ -30,7 +30,7 @@ class StringsStorage; // generate consistent IDs for moved objects. class HeapProfilerNativeMoveListener { public: - HeapProfilerNativeMoveListener(HeapProfiler* profiler) + explicit HeapProfilerNativeMoveListener(HeapProfiler* profiler) : profiler_(profiler) {} HeapProfilerNativeMoveListener(const HeapProfilerNativeMoveListener& other) = delete; @@ -116,8 +116,7 @@ class HeapProfiler : public HeapObjectAllocationTracker { Isolate* isolate() const; - void QueryObjects(Handle context, - debug::QueryObjectPredicate* predicate, + void QueryObjects(Handle context, QueryObjectPredicate* predicate, std::vector>* objects); void set_native_move_listener( std::unique_ptr listener) { diff --git a/deps/v8/test/cctest/test-heap-profiler.cc b/deps/v8/test/cctest/test-heap-profiler.cc index 1ca4a4808de2b3..4c685f90c26c54 100644 --- a/deps/v8/test/cctest/test-heap-profiler.cc +++ b/deps/v8/test/cctest/test-heap-profiler.cc @@ -30,6 +30,7 @@ #include #include +#include #include "include/v8-function.h" #include "include/v8-json.h" @@ -4064,6 +4065,67 @@ TEST(SamplingHeapProfilerSampleDuringDeopt) { heap_profiler->StopSamplingHeapProfiler(); } +namespace { +class TestQueryObjectPredicate : public v8::QueryObjectPredicate { + public: + TestQueryObjectPredicate(v8::Local context, + v8::Local symbol) + : context_(context), symbol_(symbol) {} + + bool Filter(v8::Local object) override { + return object->HasOwnProperty(context_, symbol_).FromMaybe(false); + } + + private: + v8::Local context_; + v8::Local symbol_; +}; + +class IncludeAllQueryObjectPredicate : public v8::QueryObjectPredicate { + public: + IncludeAllQueryObjectPredicate() {} + bool Filter(v8::Local object) override { return true; } +}; +} // anonymous namespace + +TEST(QueryObjects) { + LocalContext env; + v8::Isolate* isolate = env->GetIsolate(); + v8::HandleScope scope(isolate); + v8::Local context = env.local(); + + v8::Local sym = + v8::Symbol::New(isolate, v8_str("query_object_test")); + context->Global()->Set(context, v8_str("test_symbol"), sym).Check(); + v8::Local arr = CompileRun(R"( + const arr = []; + for (let i = 0; i < 10; ++i) { + arr.push({[test_symbol]: true}); + } + arr; + )"); + context->Global()->Set(context, v8_str("arr"), arr).Check(); + v8::HeapProfiler* heap_profiler = isolate->GetHeapProfiler(); + + { + TestQueryObjectPredicate predicate(context, sym); + std::vector> out; + heap_profiler->QueryObjects(context, &predicate, &out); + + CHECK_EQ(out.size(), 10); + for (size_t i = 0; i < out.size(); ++i) { + CHECK(out[i].Get(isolate)->HasOwnProperty(context, sym).FromMaybe(false)); + } + } + + { + IncludeAllQueryObjectPredicate predicate; + std::vector> out; + heap_profiler->QueryObjects(context, &predicate, &out); + CHECK_GE(out.size(), 10); + } +} + TEST(WeakReference) { v8::Isolate* isolate = CcTest::isolate(); i::Isolate* i_isolate = CcTest::i_isolate(); From 6ea5f6efa5e7dee7b0771fddb204ed6c75f2c9a0 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 3 Nov 2023 22:53:21 +0000 Subject: [PATCH 2/3] src: implement countObjectsWithPrototype This implements an internal utility for counting objects in the heap with a specified prototype. In addition this adds a checkIfCollectableByCounting() test helper. --- src/heap_utils.cc | 36 ++++++++++++++++++++++++++++++++++++ test/common/gc.js | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+) diff --git a/src/heap_utils.cc b/src/heap_utils.cc index e385955a5d5fce..75f7e7e1bdc1b4 100644 --- a/src/heap_utils.cc +++ b/src/heap_utils.cc @@ -474,6 +474,39 @@ void TriggerHeapSnapshot(const FunctionCallbackInfo& args) { return args.GetReturnValue().Set(filename_v); } +class PrototypeChainHas : public v8::QueryObjectPredicate { + public: + PrototypeChainHas(Local context, Local search) + : context_(context), search_(search) {} + + // What we can do in the filter can be quite limited, but looking up + // the prototype chain is something that the inspector console API + // queryObject() does so it is supported. + bool Filter(Local object) override { + for (Local proto = object->GetPrototype(); proto->IsObject(); + proto = proto.As()->GetPrototype()) { + if (search_ == proto) return true; + } + return false; + } + + private: + Local context_; + Local search_; +}; + +void CountObjectsWithPrototype(const FunctionCallbackInfo& args) { + CHECK_EQ(args.Length(), 1); + CHECK(args[0]->IsObject()); + Local proto = args[0].As(); + Isolate* isolate = args.GetIsolate(); + Local context = isolate->GetCurrentContext(); + PrototypeChainHas prototype_chain_has(context, proto); + std::vector> out; + isolate->GetHeapProfiler()->QueryObjects(context, &prototype_chain_has, &out); + args.GetReturnValue().Set(static_cast(out.size())); +} + void Initialize(Local target, Local unused, Local context, @@ -482,12 +515,15 @@ void Initialize(Local target, SetMethod(context, target, "triggerHeapSnapshot", TriggerHeapSnapshot); SetMethod( context, target, "createHeapSnapshotStream", CreateHeapSnapshotStream); + SetMethod( + context, target, "countObjectsWithPrototype", CountObjectsWithPrototype); } void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(BuildEmbedderGraph); registry->Register(TriggerHeapSnapshot); registry->Register(CreateHeapSnapshotStream); + registry->Register(CountObjectsWithPrototype); } } // namespace heap diff --git a/test/common/gc.js b/test/common/gc.js index 9860bb191ee072..4671a5422641ca 100644 --- a/test/common/gc.js +++ b/test/common/gc.js @@ -75,7 +75,54 @@ async function runAndBreathe(fn, repeat, waitTime = 20) { } } +/** + * This requires --expose-internals. + * This function can be used to check if an object factory leaks or not by + * iterating over the heap and count objects with the specified class + * (which is checked by looking up the prototype chain). + * @param {(i: number) => number} fn The factory receiving iteration count + * and returning number of objects created. The return value should be + * precise otherwise false negatives can be produced. + * @param {Function} klass The class whose object is used to count the objects + * @param {number} count Number of iterations that this check should be done + * @param {number} waitTime Optional breathing time for GC. + */ +async function checkIfCollectableByCounting(fn, klass, count, waitTime = 20) { + const { internalBinding } = require('internal/test/binding'); + const { countObjectsWithPrototype } = internalBinding('heap_utils'); + const { prototype, name } = klass; + const initialCount = countObjectsWithPrototype(prototype); + console.log(`Initial count of ${name}: ${initialCount}`); + let totalCreated = 0; + for (let i = 0; i < count; ++i) { + const created = await fn(i); + totalCreated += created; + console.log(`#${i}: created ${created} ${name}, total ${totalCreated}`); + await wait(waitTime); // give GC some breathing room. + const currentCount = countObjectsWithPrototype(prototype); + const collected = totalCreated - (currentCount - initialCount); + console.log(`#${i}: counted ${currentCount} ${name}, collected ${collected}`); + if (collected > 0) { + console.log(`Detected ${collected} collected ${name}, finish early`); + return; + } + } + + await wait(waitTime); // give GC some breathing room. + const currentCount = countObjectsWithPrototype(prototype); + const collected = totalCreated - (currentCount - initialCount); + console.log(`Last count: counted ${currentCount} ${name}, collected ${collected}`); + // Some objects with the prototype can be collected. + if (collected > 0) { + console.log(`Detected ${collected} collected ${name}`); + return; + } + + throw new Error(`${name} cannot be collected`); +} + module.exports = { checkIfCollectable, runAndBreathe, + checkIfCollectableByCounting, }; From fb6a0b69582944f902be67f771672ec5857d1c5b Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 3 Nov 2023 22:53:24 +0000 Subject: [PATCH 3/3] test: deflake test-diagnostics-channel-memory-leak --- test/parallel/parallel.status | 3 -- .../test-diagnostics-channel-memory-leak.js | 30 +++++++++---------- 2 files changed, 14 insertions(+), 19 deletions(-) diff --git a/test/parallel/parallel.status b/test/parallel/parallel.status index 838d465b596f9f..98288c5c1228c2 100644 --- a/test/parallel/parallel.status +++ b/test/parallel/parallel.status @@ -5,9 +5,6 @@ prefix parallel # sample-test : PASS,FLAKY [true] # This section applies to all platforms -# https://github.com/nodejs/node/pull/50327 -# Currently there's no reliable way to test it. -test-diagnostics-channel-memory-leak: SKIP [$system==win32] # https://github.com/nodejs/node/issues/41206 diff --git a/test/parallel/test-diagnostics-channel-memory-leak.js b/test/parallel/test-diagnostics-channel-memory-leak.js index 9e6364d168562f..fe50158070554d 100644 --- a/test/parallel/test-diagnostics-channel-memory-leak.js +++ b/test/parallel/test-diagnostics-channel-memory-leak.js @@ -1,24 +1,22 @@ -// Flags: --expose-gc +// Flags: --expose-internals --max-old-space-size=16 'use strict'; // This test ensures that diagnostic channel references aren't leaked. -require('../common'); -const { ok } = require('assert'); +const common = require('../common'); -const { subscribe, unsubscribe } = require('diagnostics_channel'); +const { subscribe, unsubscribe, Channel } = require('diagnostics_channel'); +const { checkIfCollectableByCounting } = require('../common/gc'); function noop() {} -const heapUsedBefore = process.memoryUsage().heapUsed; - -for (let i = 0; i < 1000; i++) { - subscribe(String(i), noop); - unsubscribe(String(i), noop); -} - -global.gc(); - -const heapUsedAfter = process.memoryUsage().heapUsed; - -ok(heapUsedBefore >= heapUsedAfter); +const outer = 64; +const inner = 256; +checkIfCollectableByCounting((i) => { + for (let j = 0; j < inner; j++) { + const key = String(i * inner + j); + subscribe(key, noop); + unsubscribe(key, noop); + } + return inner; +}, Channel, outer).then(common.mustCall());