From b6b82cba27da9ef83bfef67cac9d3593bd72c0e2 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 17 May 2020 07:13:16 +0200 Subject: [PATCH] src: use enum for refed flag on native immediates Refs: https://github.com/nodejs/node/pull/33320#discussion_r423141443 PR-URL: https://github.com/nodejs/node/pull/33444 Reviewed-By: James M Snell Reviewed-By: Zeyu Yang Reviewed-By: Colin Ihrig Reviewed-By: David Carlier Reviewed-By: Ben Noordhuis --- src/async_wrap.cc | 2 +- src/callback_queue-inl.h | 16 ++++++++-------- src/callback_queue.h | 18 +++++++++++++----- src/env-inl.h | 32 ++++++++++++-------------------- src/env.cc | 7 ++++--- src/env.h | 11 ++++------- src/node_dir.cc | 4 ++-- src/node_file.cc | 4 ++-- src/node_perf.cc | 4 ++-- src/node_worker.cc | 2 +- test/cctest/test_environment.cc | 6 +++--- 11 files changed, 52 insertions(+), 54 deletions(-) diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 7e7f4ed57236a1..46a6ea6eebe85f 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -753,7 +753,7 @@ void AsyncWrap::EmitDestroy(Environment* env, double async_id) { } if (env->destroy_async_id_list()->empty()) { - env->SetUnrefImmediate(&DestroyAsyncIdsCallback); + env->SetImmediate(&DestroyAsyncIdsCallback, CallbackFlags::kUnrefed); } env->destroy_async_id_list()->push_back(async_id); diff --git a/src/callback_queue-inl.h b/src/callback_queue-inl.h index e83c81cd0dd802..13561864027316 100644 --- a/src/callback_queue-inl.h +++ b/src/callback_queue-inl.h @@ -10,8 +10,8 @@ namespace node { template template std::unique_ptr::Callback> -CallbackQueue::CreateCallback(Fn&& fn, bool refed) { - return std::make_unique>(std::move(fn), refed); +CallbackQueue::CreateCallback(Fn&& fn, CallbackFlags::Flags flags) { + return std::make_unique>(std::move(fn), flags); } template @@ -57,12 +57,12 @@ size_t CallbackQueue::size() const { } template -CallbackQueue::Callback::Callback(bool refed) - : refed_(refed) {} +CallbackQueue::Callback::Callback(CallbackFlags::Flags flags) + : flags_(flags) {} template -bool CallbackQueue::Callback::is_refed() const { - return refed_; +CallbackFlags::Flags CallbackQueue::Callback::flags() const { + return flags_; } template @@ -80,8 +80,8 @@ void CallbackQueue::Callback::set_next( template template CallbackQueue::CallbackImpl::CallbackImpl( - Fn&& callback, bool refed) - : Callback(refed), + Fn&& callback, CallbackFlags::Flags flags) + : Callback(flags), callback_(std::move(callback)) {} template diff --git a/src/callback_queue.h b/src/callback_queue.h index ebf975e6391d13..e5694d5e1fe56a 100644 --- a/src/callback_queue.h +++ b/src/callback_queue.h @@ -7,6 +7,13 @@ namespace node { +namespace CallbackFlags { +enum Flags { + kUnrefed = 0, + kRefed = 1, +}; +} + // A queue of C++ functions that take Args... as arguments and return R // (this is similar to the signature of std::function). // New entries are added using `CreateCallback()`/`Push()`, and removed using @@ -18,25 +25,26 @@ class CallbackQueue { public: class Callback { public: - explicit inline Callback(bool refed); + explicit inline Callback(CallbackFlags::Flags flags); virtual ~Callback() = default; virtual R Call(Args... args) = 0; - inline bool is_refed() const; + inline CallbackFlags::Flags flags() const; private: inline std::unique_ptr get_next(); inline void set_next(std::unique_ptr next); - bool refed_; + CallbackFlags::Flags flags_; std::unique_ptr next_; friend class CallbackQueue; }; template - inline std::unique_ptr CreateCallback(Fn&& fn, bool refed); + inline std::unique_ptr CreateCallback( + Fn&& fn, CallbackFlags::Flags); inline std::unique_ptr Shift(); inline void Push(std::unique_ptr cb); @@ -51,7 +59,7 @@ class CallbackQueue { template class CallbackImpl final : public Callback { public: - CallbackImpl(Fn&& callback, bool refed); + CallbackImpl(Fn&& callback, CallbackFlags::Flags flags); R Call(Args... args) override; private: diff --git a/src/env-inl.h b/src/env-inl.h index 74e36af4fe3b72..3bc59acfc8beb5 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -707,29 +707,21 @@ inline void IsolateData::set_options( } template -void Environment::CreateImmediate(Fn&& cb, bool ref) { - auto callback = native_immediates_.CreateCallback(std::move(cb), ref); +void Environment::SetImmediate(Fn&& cb, CallbackFlags::Flags flags) { + auto callback = native_immediates_.CreateCallback(std::move(cb), flags); native_immediates_.Push(std::move(callback)); -} - -template -void Environment::SetImmediate(Fn&& cb) { - CreateImmediate(std::move(cb), true); - if (immediate_info()->ref_count() == 0) - ToggleImmediateRef(true); - immediate_info()->ref_count_inc(1); -} - -template -void Environment::SetUnrefImmediate(Fn&& cb) { - CreateImmediate(std::move(cb), false); + if (flags & CallbackFlags::kRefed) { + if (immediate_info()->ref_count() == 0) + ToggleImmediateRef(true); + immediate_info()->ref_count_inc(1); + } } template -void Environment::SetImmediateThreadsafe(Fn&& cb, bool refed) { - auto callback = - native_immediates_threadsafe_.CreateCallback(std::move(cb), refed); +void Environment::SetImmediateThreadsafe(Fn&& cb, CallbackFlags::Flags flags) { + auto callback = native_immediates_threadsafe_.CreateCallback( + std::move(cb), flags); { Mutex::ScopedLock lock(native_immediates_threadsafe_mutex_); native_immediates_threadsafe_.Push(std::move(callback)); @@ -740,8 +732,8 @@ void Environment::SetImmediateThreadsafe(Fn&& cb, bool refed) { template void Environment::RequestInterrupt(Fn&& cb) { - auto callback = - native_immediates_interrupts_.CreateCallback(std::move(cb), false); + auto callback = native_immediates_interrupts_.CreateCallback( + std::move(cb), CallbackFlags::kRefed); { Mutex::ScopedLock lock(native_immediates_threadsafe_mutex_); native_immediates_interrupts_.Push(std::move(callback)); diff --git a/src/env.cc b/src/env.cc index 39e0d7309ef92f..8214936d794101 100644 --- a/src/env.cc +++ b/src/env.cc @@ -587,7 +587,7 @@ void Environment::CleanupHandles() { Isolate::DisallowJavascriptExecutionScope disallow_js(isolate(), Isolate::DisallowJavascriptExecutionScope::THROW_ON_FAILURE); - RunAndClearNativeImmediates(true /* skip SetUnrefImmediate()s */); + RunAndClearNativeImmediates(true /* skip unrefed SetImmediate()s */); for (ReqWrapBase* request : req_wrap_queue_) request->Cancel(); @@ -730,10 +730,11 @@ void Environment::RunAndClearNativeImmediates(bool only_refed) { TryCatchScope try_catch(this); DebugSealHandleScope seal_handle_scope(isolate()); while (auto head = queue->Shift()) { - if (head->is_refed()) + bool is_refed = head->flags() & CallbackFlags::kRefed; + if (is_refed) ref_count++; - if (head->is_refed() || !only_refed) + if (is_refed || !only_refed) head->Call(this); head.reset(); // Destroy now so that this is also observed by try_catch. diff --git a/src/env.h b/src/env.h index 9371ef296aefe8..a0a33d71b6c8eb 100644 --- a/src/env.h +++ b/src/env.h @@ -1165,12 +1165,12 @@ class Environment : public MemoryRetainer { // Unlike the JS setImmediate() function, nested SetImmediate() calls will // be run without returning control to the event loop, similar to nextTick(). template - inline void SetImmediate(Fn&& cb); - template - inline void SetUnrefImmediate(Fn&& cb); + inline void SetImmediate( + Fn&& cb, CallbackFlags::Flags flags = CallbackFlags::kRefed); template // This behaves like SetImmediate() but can be called from any thread. - inline void SetImmediateThreadsafe(Fn&& cb, bool refed = true); + inline void SetImmediateThreadsafe( + Fn&& cb, CallbackFlags::Flags flags = CallbackFlags::kRefed); // This behaves like V8's Isolate::RequestInterrupt(), but also accounts for // the event loop (i.e. combines the V8 function with SetImmediate()). // The passed callback may not throw exceptions. @@ -1253,9 +1253,6 @@ class Environment : public MemoryRetainer { void RunAndClearInterrupts(); private: - template - inline void CreateImmediate(Fn&& cb, bool ref); - inline void ThrowError(v8::Local (*fun)(v8::Local), const char* errmsg); diff --git a/src/node_dir.cc b/src/node_dir.cc index b0904c3d39ddd0..cb32fa0a5c6629 100644 --- a/src/node_dir.cc +++ b/src/node_dir.cc @@ -126,10 +126,10 @@ inline void DirHandle::GCClose() { // to notify that the file descriptor was gc'd. We want to be noisy about // this because not explicitly closing the DirHandle is a bug. - env()->SetUnrefImmediate([](Environment* env) { + env()->SetImmediate([](Environment* env) { ProcessEmitWarning(env, "Closing directory handle on garbage collection"); - }); + }, CallbackFlags::kUnrefed); } void AfterClose(uv_fs_t* req) { diff --git a/src/node_file.cc b/src/node_file.cc index 96adbc9e756688..d10a695ff9e679 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -226,7 +226,7 @@ inline void FileHandle::Close() { // to notify that the file descriptor was gc'd. We want to be noisy about // this because not explicitly closing the FileHandle is a bug. - env()->SetUnrefImmediate([detail](Environment* env) { + env()->SetImmediate([detail](Environment* env) { ProcessEmitWarning(env, "Closing file descriptor %d on garbage collection", detail.fd); @@ -240,7 +240,7 @@ inline void FileHandle::Close() { "thrown if a file descriptor is closed during garbage collection.", "DEP0137").IsNothing(); } - }); + }, CallbackFlags::kUnrefed); } void FileHandle::CloseReq::Resolve() { diff --git a/src/node_perf.cc b/src/node_perf.cc index 4b8bf2a8a7c913..fb3b2c43acdd9f 100644 --- a/src/node_perf.cc +++ b/src/node_perf.cc @@ -282,9 +282,9 @@ void MarkGarbageCollectionEnd(Isolate* isolate, static_cast(flags), state->performance_last_gc_start_mark, PERFORMANCE_NOW()); - env->SetUnrefImmediate([entry = std::move(entry)](Environment* env) mutable { + env->SetImmediate([entry = std::move(entry)](Environment* env) mutable { PerformanceGCCallback(env, std::move(entry)); - }); + }, CallbackFlags::kUnrefed); } void GarbageCollectionCleanupHook(void* data) { diff --git a/src/node_worker.cc b/src/node_worker.cc index 8ba250577354d9..f7d5434efd9e7c 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -759,7 +759,7 @@ void Worker::TakeHeapSnapshot(const FunctionCallbackInfo& args) { env, std::move(snapshot)); Local args[] = { stream->object() }; taker->MakeCallback(env->ondone_string(), arraysize(args), args); - }, /* refed */ false); + }, CallbackFlags::kUnrefed); }); args.GetReturnValue().Set(scheduled ? taker->object() : Local()); } diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index adde282570a8cc..d0b241528ba31e 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -266,11 +266,11 @@ TEST_F(EnvironmentTest, SetImmediateCleanup) { (*env)->SetImmediate([&](node::Environment* env_arg) { EXPECT_EQ(env_arg, *env); called++; - }); - (*env)->SetUnrefImmediate([&](node::Environment* env_arg) { + }, node::CallbackFlags::kRefed); + (*env)->SetImmediate([&](node::Environment* env_arg) { EXPECT_EQ(env_arg, *env); called_unref++; - }); + }, node::CallbackFlags::kUnrefed); } EXPECT_EQ(called, 1);