Skip to content

Commit

Permalink
src: do not use std::function for OnScopeLeave
Browse files Browse the repository at this point in the history
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: #30134
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
addaleax committed Nov 7, 2019
1 parent 35ae49d commit 6072e01
Show file tree
Hide file tree
Showing 12 changed files with 45 additions and 22 deletions.
2 changes: 1 addition & 1 deletion src/api/callback.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
2 changes: 1 addition & 1 deletion src/large_pages/node_large_page.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});

Expand Down
2 changes: 1 addition & 1 deletion src/node_binding.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion src/node_env_var.cc
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ Local<Array> 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<Local<Value>, 256> env_v(count);
Expand Down
2 changes: 1 addition & 1 deletion src/node_http_parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/node_i18n.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -809,7 +809,7 @@ void GetOptions(const FunctionCallbackInfo<Value>& 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;
});
Expand Down
2 changes: 1 addition & 1 deletion src/node_os.cc
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ static void GetUserInfo(const FunctionCallbackInfo<Value>& args) {
return args.GetReturnValue().SetUndefined();
}

OnScopeLeave free_passwd([&]() { uv_os_free_passwd(&pwd); });
auto free_passwd = OnScopeLeave([&]() { uv_os_free_passwd(&pwd); });

Local<Value> error;

Expand Down
2 changes: 1 addition & 1 deletion src/node_process_methods.cc
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ static void DebugProcess(const FunctionCallbackInfo<Value>& 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);
Expand Down
2 changes: 1 addition & 1 deletion src/node_worker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ void Worker::Run() {
SealHandleScope outer_seal(isolate_);

DeleteFnPtr<Environment, FreeEnvironment> env_;
OnScopeLeave cleanup_env([&]() {
auto cleanup_env = OnScopeLeave([&]() {
if (!env_) return;
env_->set_can_call_into_js(false);
Isolate::DisallowJavascriptExecutionScope disallow_js(isolate_,
Expand Down
2 changes: 1 addition & 1 deletion src/node_zlib.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
45 changes: 34 additions & 11 deletions src/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,12 @@
#include <unordered_map>
#include <utility>

#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
Expand Down Expand Up @@ -494,14 +500,37 @@ class BufferValue : public MaybeStackBuffer<char> {
// silence a compiler warning about that.
template <typename T> inline void USE(T&&) {}

// Run a function when exiting the current scope.
struct OnScopeLeave {
std::function<void()> fn_;
template <typename Fn>
struct OnScopeLeaveImpl {
Fn fn_;
bool active_;

explicit OnScopeLeaveImpl(Fn&& fn) : fn_(std::move(fn)), active_(true) {}
~OnScopeLeaveImpl() { if (active_) fn_(); }

explicit OnScopeLeave(std::function<void()> 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 <typename Fn>
inline MUST_USE_RESULT OnScopeLeaveImpl<Fn> OnScopeLeave(Fn&& fn) {
return OnScopeLeaveImpl<Fn>{std::move(fn)};
}

// Simple RAII wrapper for contiguous data that uses malloc()/free().
template <typename T>
struct MallocedBuffer {
Expand Down Expand Up @@ -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<v8::Local<v8::Value>> {
public:
inline explicit SlicedArguments(
Expand Down

0 comments on commit 6072e01

Please sign in to comment.