From ff89670902042cc6a6d99596c7e2d34fd11ad6a2 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Thu, 4 Apr 2019 10:24:21 -0400 Subject: [PATCH] n-api: reduce gc finalization stress https://github.com/nodejs/node/pull/24494 fixed a crash but resulted in increased stress on gc finalization. A leak was reported in https://github.com/nodejs/node/issues/26667 which we are still investigating. As part of this investigation I realized we can optimize to reduce amount of deferred finalization. Regardless of the root cause of the leak this should be a good optimization. It also resolves the leak for the case being reported in #26667. The OP in 26667 has confirmed that he can still recreate the original problem that 24494 fixed and that the fix still works with this optimization PR-URL: https://github.com/nodejs/node/pull/27085 Reviewed-By: Ben Noordhuis Reviewed-By: Colin Ihrig Reviewed-By: James M Snell --- src/js_native_api_v8.cc | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index f6914e72ddafd2..134ae992ec037b 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -227,10 +227,11 @@ class Reference : private Finalizer { // from one of Unwrap or napi_delete_reference. // // When it is called from Unwrap or napi_delete_reference we only - // want to do the delete if the finalizer has already run, + // want to do the delete if the finalizer has already run or + // cannot have been queued to run (ie the reference count is > 0), // otherwise we may crash when the finalizer does run. - // If the finalizer has not already run delay the delete until - // the finalizer runs by not doing the delete + // If the finalizer may have been queued and has not already run + // delay the delete until the finalizer runs by not doing the delete // and setting _delete_self to true so that the finalizer will // delete it when it runs. // @@ -238,13 +239,14 @@ class Reference : private Finalizer { // the finalizer and _delete_self is set. In this case we // know we need to do the deletion so just do it. static void Delete(Reference* reference) { - if ((reference->_delete_self) || (reference->_finalize_ran)) { + if ((reference->RefCount() != 0) || + (reference->_delete_self) || + (reference->_finalize_ran)) { delete reference; } else { - // reduce the reference count to 0 and defer until - // finalizer runs + // defer until finalizer runs as + // it may alread be queued reference->_delete_self = true; - while (reference->Unref() != 0) {} } }