From cb9100ce00ecd3760d4243fd59dc392905ab7b76 Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Thu, 29 Oct 2026 13:11:46 -0700 Subject: [PATCH] Drain deferred V8 N-API finalizers after each dispatcher turn On V8, a garbage collection does not run N-API finalizers directly. The weak callback enqueues each finalizer on napi_env::pending_finalizers for a second pass that has to run from a safe host boundary, and the host owns draining that queue. JsRuntimeHost never drained it, so every collected wrapper (Napi::External, ObjectWrap, external array buffers) kept its native resources alive until the environment was torn down. Plugins that hand out many short-lived wrappers per frame, such as WebGPU buffers, textures and command encoders, leaked without bound. AppRuntime now calls an engine-specific DrainPostDispatchWork hook after each dispatcher turn that ran work. On V8 the hook is Napi::DrainFinalizers, which pops finalizers from the pending queue one at a time (a finalizer may enqueue or dequeue others) and stops once an 8 ms budget is spent so a large batch cannot monopolize the JavaScript thread; the remainder runs on the following turns. Running once per turn rather than after each callback keeps the drain off the per-callback path. The hook is a no-op on Chakra, JavaScriptCore, QuickJS, Hermes and JSI, which finalize through their own mechanisms. Two V8-only unit tests cover the behaviour: the finalizers of collected externals have all run by the dispatcher turn after the collection, and a batch whose finalizers exceed the budget is spread across turns instead of completing in one. This supersedes plugin-local workarounds such as the finalizer pump in the NativeDawn plugin (#1781), which reaches into napi_env internals from outside JsRuntimeHost. Port of BabylonJS/JsRuntimeHost#217. --- .../AppRuntime/Include/Babylon/AppRuntime.h | 4 + .../Core/AppRuntime/Source/AppRuntime.cpp | 5 +- .../AppRuntime/Source/AppRuntime_Chakra.cpp | 4 + .../AppRuntime/Source/AppRuntime_Hermes.cpp | 4 + .../Core/AppRuntime/Source/AppRuntime_JSI.cpp | 4 + .../Source/AppRuntime_JavaScriptCore.cpp | 4 + .../AppRuntime/Source/AppRuntime_QuickJS.cpp | 4 + .../Core/AppRuntime/Source/AppRuntime_V8.cpp | 7 ++ .../Node-API/Include/Engine/V8/napi/env.h | 2 + .../Core/Node-API/Source/env_v8.cc | 27 ++++++ .../Tests/UnitTests/CMakeLists.txt | 3 + .../Android/app/src/main/cpp/CMakeLists.txt | 3 + .../UnitTests/Source/Tests.AppRuntime.cpp | 92 +++++++++++++++++++ 13 files changed, 162 insertions(+), 1 deletion(-) diff --git a/Dependencies/JsRuntimeHost/Core/AppRuntime/Include/Babylon/AppRuntime.h b/Dependencies/JsRuntimeHost/Core/AppRuntime/Include/Babylon/AppRuntime.h index d6d024e571..48bc3552b4 100644 --- a/Dependencies/JsRuntimeHost/Core/AppRuntime/Include/Babylon/AppRuntime.h +++ b/Dependencies/JsRuntimeHost/Core/AppRuntime/Include/Babylon/AppRuntime.h @@ -85,6 +85,10 @@ namespace Babylon // queue explicitly (Napi::DrainJobs / JS_ExecutePendingJob). void DrainMicrotasks(Napi::Env env); + // Engine-specific maintenance that should run once after a complete + // dispatcher turn, rather than after each callback in that turn. + void DrainPostDispatchWork(Napi::Env env); + Internal::DelayedTaskScheduler& GetDelayedTaskScheduler(); Options m_options; diff --git a/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime.cpp b/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime.cpp index 78ebe2d924..ea141d6511 100644 --- a/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime.cpp +++ b/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime.cpp @@ -90,7 +90,10 @@ namespace Babylon while (!m_impl->m_cancelSource.cancelled()) { - m_impl->m_dispatcher.blocking_tick(m_impl->m_cancelSource); + if (m_impl->m_dispatcher.blocking_tick(m_impl->m_cancelSource)) + { + DrainPostDispatchWork(env); + } } Napi::HandleScope scope{env}; diff --git a/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime_Chakra.cpp b/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime_Chakra.cpp index 6c41d67fd4..3f241ee06f 100644 --- a/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime_Chakra.cpp +++ b/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime_Chakra.cpp @@ -82,4 +82,8 @@ namespace Babylon // JsSetPromiseContinuationCallback hook (see RunEnvironmentTier). // No explicit pump needed here. } + + void AppRuntime::DrainPostDispatchWork(Napi::Env) + { + } } diff --git a/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime_Hermes.cpp b/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime_Hermes.cpp index f9dc3d5afc..c78755f45f 100644 --- a/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime_Hermes.cpp +++ b/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime_Hermes.cpp @@ -29,4 +29,8 @@ namespace Babylon // observes the same "between turns" semantics it gets on V8/Chakra. Napi::DrainJobs(env); } + + void AppRuntime::DrainPostDispatchWork(Napi::Env) + { + } } diff --git a/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime_JSI.cpp b/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime_JSI.cpp index a81a39000d..5c83053b2d 100644 --- a/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime_JSI.cpp +++ b/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime_JSI.cpp @@ -54,4 +54,8 @@ namespace Babylon { // JSI/V8 backed JSI auto-drains microtasks per scope. } + + void AppRuntime::DrainPostDispatchWork(Napi::Env) + { + } } diff --git a/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp b/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp index f2483f4178..70672a6de8 100644 --- a/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp +++ b/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp @@ -32,4 +32,8 @@ namespace Babylon { // JavaScriptCore drains microtasks automatically at script boundaries. } + + void AppRuntime::DrainPostDispatchWork(Napi::Env) + { + } } diff --git a/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime_QuickJS.cpp b/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime_QuickJS.cpp index 10505fea60..36538b687f 100644 --- a/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime_QuickJS.cpp +++ b/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime_QuickJS.cpp @@ -68,4 +68,8 @@ namespace Babylon { } } + + void AppRuntime::DrainPostDispatchWork(Napi::Env) + { + } } diff --git a/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime_V8.cpp b/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime_V8.cpp index 83da9550fc..efe3f23427 100644 --- a/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime_V8.cpp +++ b/Dependencies/JsRuntimeHost/Core/AppRuntime/Source/AppRuntime_V8.cpp @@ -126,4 +126,11 @@ namespace Babylon { // V8 auto-drains microtasks. Foreground tasks run on AppRuntime's dispatcher. } + + void AppRuntime::DrainPostDispatchWork(Napi::Env env) + { + // N-API finalizers deferred by a V8 garbage collection require a safe + // host boundary, but should not monopolize a dispatcher turn. + Napi::DrainFinalizers(env); + } } diff --git a/Dependencies/JsRuntimeHost/Core/Node-API/Include/Engine/V8/napi/env.h b/Dependencies/JsRuntimeHost/Core/Node-API/Include/Engine/V8/napi/env.h index 22334ec5e6..65406498d9 100644 --- a/Dependencies/JsRuntimeHost/Core/Node-API/Include/Engine/V8/napi/env.h +++ b/Dependencies/JsRuntimeHost/Core/Node-API/Include/Engine/V8/napi/env.h @@ -52,6 +52,8 @@ namespace Napi void Detach(Napi::Env); + void DrainFinalizers(Napi::Env); + Napi::Value Eval(Napi::Env env, const char* source, const char* sourceUrl); v8::Local GetContext(Napi::Env); diff --git a/Dependencies/JsRuntimeHost/Core/Node-API/Source/env_v8.cc b/Dependencies/JsRuntimeHost/Core/Node-API/Source/env_v8.cc index 1fbd7d3085..fd2575d967 100644 --- a/Dependencies/JsRuntimeHost/Core/Node-API/Source/env_v8.cc +++ b/Dependencies/JsRuntimeHost/Core/Node-API/Source/env_v8.cc @@ -2,8 +2,15 @@ #include #include "js_native_api_v8.h" +#include + namespace Napi { + namespace + { + constexpr auto FinalizerDrainBudget{std::chrono::milliseconds{8}}; + } + Env Attach(v8::Local isolate) { // second argument is module version @@ -16,6 +23,26 @@ namespace Napi env_ptr->DeleteMe(); } + void DrainFinalizers(Env env) + { + napi_env env_ptr{env}; + const auto start = std::chrono::steady_clock::now(); + + // Finalizers can mutate this queue. Bound each dispatcher turn so a large + // batch cannot monopolize the runtime thread. + while (!env_ptr->pending_finalizers.empty()) + { + auto* finalizer = *env_ptr->pending_finalizers.begin(); + env_ptr->pending_finalizers.erase(finalizer); + finalizer->Finalize(); + + if (std::chrono::steady_clock::now() - start >= FinalizerDrainBudget) + { + break; + } + } + } + v8::Local GetContext(Env env) { napi_env env_ptr{env}; diff --git a/Dependencies/JsRuntimeHost/Tests/UnitTests/CMakeLists.txt b/Dependencies/JsRuntimeHost/Tests/UnitTests/CMakeLists.txt index 802734b58c..f713feee93 100644 --- a/Dependencies/JsRuntimeHost/Tests/UnitTests/CMakeLists.txt +++ b/Dependencies/JsRuntimeHost/Tests/UnitTests/CMakeLists.txt @@ -94,6 +94,9 @@ target_compile_definitions(${_jsruntimehost_unit_tests_target} PRIVATE NAPI_JAVA if(NAPI_JAVASCRIPT_ENGINE STREQUAL "JSI") target_compile_definitions(${_jsruntimehost_unit_tests_target} PRIVATE JSRUNTIMEHOST_NAPI_ENGINE_JSI) endif() +if(NAPI_JAVASCRIPT_ENGINE STREQUAL "V8") + target_compile_definitions(${_jsruntimehost_unit_tests_target} PRIVATE JSRUNTIMEHOST_NAPI_ENGINE_V8) +endif() target_link_libraries(${_jsruntimehost_unit_tests_target} PRIVATE AppRuntime diff --git a/Dependencies/JsRuntimeHost/Tests/UnitTests/Source/Android/app/src/main/cpp/CMakeLists.txt b/Dependencies/JsRuntimeHost/Tests/UnitTests/Source/Android/app/src/main/cpp/CMakeLists.txt index bcc869e548..8a0bf50f88 100644 --- a/Dependencies/JsRuntimeHost/Tests/UnitTests/Source/Android/app/src/main/cpp/CMakeLists.txt +++ b/Dependencies/JsRuntimeHost/Tests/UnitTests/Source/Android/app/src/main/cpp/CMakeLists.txt @@ -45,6 +45,9 @@ target_compile_definitions(UnitTestsJNI PRIVATE ARCANA_TEST_HOOKS) if(NAPI_JAVASCRIPT_ENGINE STREQUAL "JSI") target_compile_definitions(UnitTestsJNI PRIVATE JSRUNTIMEHOST_NAPI_ENGINE_JSI) endif() +if(NAPI_JAVASCRIPT_ENGINE STREQUAL "V8") + target_compile_definitions(UnitTestsJNI PRIVATE JSRUNTIMEHOST_NAPI_ENGINE_V8) +endif() target_include_directories(UnitTestsJNI PRIVATE ${UNIT_TESTS_DIR}) diff --git a/Dependencies/JsRuntimeHost/Tests/UnitTests/Source/Tests.AppRuntime.cpp b/Dependencies/JsRuntimeHost/Tests/UnitTests/Source/Tests.AppRuntime.cpp index 79e306f4a1..076a0cf5a6 100644 --- a/Dependencies/JsRuntimeHost/Tests/UnitTests/Source/Tests.AppRuntime.cpp +++ b/Dependencies/JsRuntimeHost/Tests/UnitTests/Source/Tests.AppRuntime.cpp @@ -1,11 +1,16 @@ #include #include #include +#include #include #include #include #include +#if defined(JSRUNTIMEHOST_NAPI_ENGINE_V8) +#include +#endif + TEST(AppRuntime, DestroyDoesNotDeadlock) { // Regression test verifying AppRuntime destruction doesn't deadlock. @@ -102,3 +107,90 @@ TEST(AppRuntime, DestroyDoesNotDeadlock) testThread.join(); } + +#if defined(JSRUNTIMEHOST_NAPI_ENGINE_V8) +TEST(AppRuntime, V8FinalizersDrainAfterDispatch) +{ + constexpr size_t ExternalCount{32}; + std::atomic finalized{}; + std::promise created; + std::promise collectionRequested; + std::promise observed; + + Babylon::AppRuntime runtime{}; + runtime.Dispatch([&finalized, &created](Napi::Env env) { + for (size_t index{}; index < ExternalCount; ++index) + { + Napi::External>::New( + env, + &finalized, + [](Napi::Env, std::atomic* count) { + count->fetch_add(1, std::memory_order_relaxed); + }); + } + created.set_value(); + }); + created.get_future().wait(); + + runtime.Dispatch([&collectionRequested](Napi::Env env) { + Napi::GetContext(env)->GetIsolate()->LowMemoryNotification(); + collectionRequested.set_value(); + }); + collectionRequested.get_future().wait(); + + runtime.Dispatch([&finalized, &observed](Napi::Env) { + observed.set_value(finalized.load(std::memory_order_relaxed)); + }); + + EXPECT_EQ(observed.get_future().get(), ExternalCount); +} + +TEST(AppRuntime, V8FinalizerDrainYieldsBetweenDispatcherTurns) +{ + constexpr size_t ExternalCount{16}; + std::atomic finalized{}; + std::promise created; + std::promise collectionRequested; + + Babylon::AppRuntime runtime{}; + runtime.Dispatch([&](Napi::Env env) { + for (size_t index{}; index < ExternalCount; ++index) + { + Napi::External>::New( + env, + &finalized, + [](Napi::Env, std::atomic* count) { + std::this_thread::sleep_for(std::chrono::milliseconds{2}); + count->fetch_add(1, std::memory_order_relaxed); + }); + } + created.set_value(); + }); + created.get_future().wait(); + + runtime.Dispatch([&](Napi::Env env) { + Napi::GetContext(env)->GetIsolate()->LowMemoryNotification(); + collectionRequested.set_value(); + }); + collectionRequested.get_future().wait(); + + const auto observeFinalized = [&]() { + std::promise observed; + auto future = observed.get_future(); + runtime.Dispatch([&](Napi::Env) { + observed.set_value(finalized.load(std::memory_order_relaxed)); + }); + return future.get(); + }; + + auto observed = observeFinalized(); + EXPECT_GT(observed, 0u); + EXPECT_LT(observed, ExternalCount); + + for (size_t turn{}; turn < ExternalCount && observed < ExternalCount; ++turn) + { + observed = observeFinalized(); + } + EXPECT_EQ(observed, ExternalCount); +} +#endif