From 058a8d53631df7aba5fbbff8859ebc8e88fade49 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 26 Oct 2019 18:29:46 +0200 Subject: [PATCH] src: do not use `std::function` for `OnScopeLeave` Using `std::function` adds an extra layer of indirection, and in particular, heap allocations that are not necessary in our use case here. PR-URL: https://github.com/nodejs/node/pull/30134 Reviewed-By: Gus Caplan Reviewed-By: Colin Ihrig Reviewed-By: David Carlier Reviewed-By: Franziska Hinkelmann Reviewed-By: James M Snell --- src/api/callback.cc | 2 +- src/large_pages/node_large_page.cc | 2 +- src/node_binding.cc | 2 +- src/node_env_var.cc | 2 +- src/node_http_parser.cc | 2 +- src/node_i18n.cc | 2 +- src/node_options.cc | 2 +- src/node_os.cc | 2 +- src/node_process_methods.cc | 2 +- src/node_worker.cc | 2 +- src/node_zlib.cc | 2 +- src/util.h | 45 ++++++++++++++++++++++-------- 12 files changed, 45 insertions(+), 22 deletions(-) diff --git a/src/api/callback.cc b/src/api/callback.cc index 238bf49a54f76d..355986b981a381 100644 --- a/src/api/callback.cc +++ b/src/api/callback.cc @@ -105,7 +105,7 @@ void InternalCallbackScope::Close() { if (!env_->can_call_into_js()) return; - OnScopeLeave weakref_cleanup([&]() { env_->RunWeakRefCleanup(); }); + auto weakref_cleanup = OnScopeLeave([&]() { env_->RunWeakRefCleanup(); }); if (!tick_info->has_tick_scheduled()) { MicrotasksScope::PerformCheckpoint(env_->isolate()); diff --git a/src/large_pages/node_large_page.cc b/src/large_pages/node_large_page.cc index 4e2f8fc4410316..68fa178b40b6cd 100644 --- a/src/large_pages/node_large_page.cc +++ b/src/large_pages/node_large_page.cc @@ -333,7 +333,7 @@ MoveTextRegionToLargePages(const text_region& r) { PrintSystemError(errno); return -1; } - OnScopeLeave munmap_on_return([nmem, size]() { + auto munmap_on_return = OnScopeLeave([nmem, size]() { if (-1 == munmap(nmem, size)) PrintSystemError(errno); }); diff --git a/src/node_binding.cc b/src/node_binding.cc index f0a148a495605c..3ae361634d1263 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -152,7 +152,7 @@ void* wrapped_dlopen(const char* filename, int flags) { Mutex::ScopedLock lock(dlhandles_mutex); uv_fs_t req; - OnScopeLeave cleanup([&]() { uv_fs_req_cleanup(&req); }); + auto cleanup = OnScopeLeave([&]() { uv_fs_req_cleanup(&req); }); int rc = uv_fs_stat(nullptr, &req, filename, nullptr); if (rc != 0) { diff --git a/src/node_env_var.cc b/src/node_env_var.cc index c63cb2c37fb3e0..9d229ccf4e5f8b 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -149,7 +149,7 @@ Local RealEnvStore::Enumerate(Isolate* isolate) const { uv_env_item_t* items; int count; - OnScopeLeave cleanup([&]() { uv_os_free_environ(items, count); }); + auto cleanup = OnScopeLeave([&]() { uv_os_free_environ(items, count); }); CHECK_EQ(uv_os_environ(&items, &count), 0); MaybeStackBuffer, 256> env_v(count); diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index bfc1582b468e17..c6136702c7cb43 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -586,7 +586,7 @@ class Parser : public AsyncWrap, public StreamListener { // Once we’re done here, either indicate that the HTTP parser buffer // is free for re-use, or free() the data if it didn’t come from there // in the first place. - OnScopeLeave on_scope_leave([&]() { + auto on_scope_leave = OnScopeLeave([&]() { if (buf.base == env()->http_parser_buffer()) env()->set_http_parser_buffer_in_use(false); else diff --git a/src/node_i18n.cc b/src/node_i18n.cc index ecc0528e76f8c6..c68e01e1074a4a 100644 --- a/src/node_i18n.cc +++ b/src/node_i18n.cc @@ -217,7 +217,7 @@ class ConverterObject : public BaseObject, Converter { result.AllocateSufficientStorage(limit); UBool flush = (flags & CONVERTER_FLAGS_FLUSH) == CONVERTER_FLAGS_FLUSH; - OnScopeLeave cleanup([&]() { + auto cleanup = OnScopeLeave([&]() { if (flush) { // Reset the converter state. converter->bomSeen_ = false; diff --git a/src/node_options.cc b/src/node_options.cc index 695d7cee6541cc..8d97791f79b60a 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -809,7 +809,7 @@ void GetOptions(const FunctionCallbackInfo& args) { per_process::cli_options->per_isolate = env->isolate_data()->options(); auto original_per_env = per_process::cli_options->per_isolate->per_env; per_process::cli_options->per_isolate->per_env = env->options(); - OnScopeLeave on_scope_leave([&]() { + auto on_scope_leave = OnScopeLeave([&]() { per_process::cli_options->per_isolate->per_env = original_per_env; per_process::cli_options->per_isolate = original_per_isolate; }); diff --git a/src/node_os.cc b/src/node_os.cc index b6fb305948e234..131e38055685c2 100644 --- a/src/node_os.cc +++ b/src/node_os.cc @@ -302,7 +302,7 @@ static void GetUserInfo(const FunctionCallbackInfo& args) { return args.GetReturnValue().SetUndefined(); } - OnScopeLeave free_passwd([&]() { uv_os_free_passwd(&pwd); }); + auto free_passwd = OnScopeLeave([&]() { uv_os_free_passwd(&pwd); }); Local error; diff --git a/src/node_process_methods.cc b/src/node_process_methods.cc index f2fb5cf38cf211..7e2af379079c4c 100644 --- a/src/node_process_methods.cc +++ b/src/node_process_methods.cc @@ -362,7 +362,7 @@ static void DebugProcess(const FunctionCallbackInfo& args) { LPTHREAD_START_ROUTINE* handler = nullptr; DWORD pid = 0; - OnScopeLeave cleanup([&]() { + auto cleanup = OnScopeLeave([&]() { if (process != nullptr) CloseHandle(process); if (thread != nullptr) CloseHandle(thread); if (handler != nullptr) UnmapViewOfFile(handler); diff --git a/src/node_worker.cc b/src/node_worker.cc index b35b4040cabfb7..9f2da4c9de2d19 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -242,7 +242,7 @@ void Worker::Run() { SealHandleScope outer_seal(isolate_); DeleteFnPtr env_; - OnScopeLeave cleanup_env([&]() { + auto cleanup_env = OnScopeLeave([&]() { if (!env_) return; env_->set_can_call_into_js(false); Isolate::DisallowJavascriptExecutionScope disallow_js(isolate_, diff --git a/src/node_zlib.cc b/src/node_zlib.cc index 30fef0ff1d4d57..fdcf685caf2e5b 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -386,7 +386,7 @@ class CompressionStream : public AsyncWrap, public ThreadPoolWork { // v8 land! void AfterThreadPoolWork(int status) override { AllocScope alloc_scope(this); - OnScopeLeave on_scope_leave([&]() { Unref(); }); + auto on_scope_leave = OnScopeLeave([&]() { Unref(); }); write_in_progress_ = false; diff --git a/src/util.h b/src/util.h index c8bb44721f6250..f2d3f355f9f713 100644 --- a/src/util.h +++ b/src/util.h @@ -42,6 +42,12 @@ #include #include +#ifdef __GNUC__ +#define MUST_USE_RESULT __attribute__((warn_unused_result)) +#else +#define MUST_USE_RESULT +#endif + namespace node { // Maybe remove kPathSeparator when cpp17 is ready @@ -494,14 +500,37 @@ class BufferValue : public MaybeStackBuffer { // silence a compiler warning about that. template inline void USE(T&&) {} -// Run a function when exiting the current scope. -struct OnScopeLeave { - std::function fn_; +template +struct OnScopeLeaveImpl { + Fn fn_; + bool active_; + + explicit OnScopeLeaveImpl(Fn&& fn) : fn_(std::move(fn)), active_(true) {} + ~OnScopeLeaveImpl() { if (active_) fn_(); } - explicit OnScopeLeave(std::function fn) : fn_(std::move(fn)) {} - ~OnScopeLeave() { fn_(); } + OnScopeLeaveImpl(const OnScopeLeaveImpl& other) = delete; + OnScopeLeaveImpl& operator=(const OnScopeLeaveImpl& other) = delete; + OnScopeLeaveImpl(OnScopeLeaveImpl&& other) + : fn_(std::move(other.fn_)), active_(other.active_) { + other.active_ = false; + } + OnScopeLeaveImpl& operator=(OnScopeLeaveImpl&& other) { + if (this == &other) return *this; + this->~OnScopeLeave(); + new (this)OnScopeLeaveImpl(std::move(other)); + return *this; + } }; +// Run a function when exiting the current scope. Used like this: +// auto on_scope_leave = OnScopeLeave([&] { +// // ... run some code ... +// }); +template +inline MUST_USE_RESULT OnScopeLeaveImpl OnScopeLeave(Fn&& fn) { + return OnScopeLeaveImpl{std::move(fn)}; +} + // Simple RAII wrapper for contiguous data that uses malloc()/free(). template struct MallocedBuffer { @@ -679,12 +708,6 @@ constexpr T RoundUp(T a, T b) { return a % b != 0 ? a + b - (a % b) : a; } -#ifdef __GNUC__ -#define MUST_USE_RESULT __attribute__((warn_unused_result)) -#else -#define MUST_USE_RESULT -#endif - class SlicedArguments : public MaybeStackBuffer> { public: inline explicit SlicedArguments(