Skip to content

Commit

Permalink
src: prevent extra copies of TimerWrap::TimerCb
Browse files Browse the repository at this point in the history
I noticed that we were taking `TimerCb` as a `const&` and then copying
that into the member. This is completely fine when the constructor is
called with an lvalue. However, when called with an rvalue, we can allow
the `std::function` to be moved into the member instead of falling back
to a copy, so I changed the constructors to take in universal
references. Also, `std::function` constructors can take in multiple
arguments, so I further modified the constructors to use variadic
templates.

Signed-off-by: Darshan Sen <darshan.sen@postman.com>

PR-URL: #40665
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
  • Loading branch information
RaisinTen authored and danielleadams committed Feb 1, 2022
1 parent 0377edc commit 9cc2bb7
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 20 deletions.
1 change: 1 addition & 0 deletions node.gyp
Expand Up @@ -560,6 +560,7 @@
'src/tracing/trace_event_common.h',
'src/tracing/traced_value.h',
'src/timer_wrap.h',
'src/timer_wrap-inl.h',
'src/tty_wrap.h',
'src/udp_wrap.h',
'src/util.h',
Expand Down
2 changes: 1 addition & 1 deletion src/inspector_agent.cc
Expand Up @@ -15,7 +15,7 @@
#include "node_process-inl.h"
#include "node_url.h"
#include "util-inl.h"
#include "timer_wrap.h"
#include "timer_wrap-inl.h"
#include "v8-inspector.h"
#include "v8-platform.h"

Expand Down
32 changes: 32 additions & 0 deletions src/timer_wrap-inl.h
@@ -0,0 +1,32 @@
#ifndef SRC_TIMER_WRAP_INL_H_
#define SRC_TIMER_WRAP_INL_H_

#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#include "timer_wrap.h"

#include <utility>

#include "env.h"
#include "uv.h"

namespace node {

template <typename... Args>
inline TimerWrap::TimerWrap(Environment* env, Args&&... args)
: env_(env), fn_(std::forward<Args>(args)...) {
uv_timer_init(env->event_loop(), &timer_);
timer_.data = this;
}

template <typename... Args>
inline TimerWrapHandle::TimerWrapHandle(Environment* env, Args&&... args) {
timer_ = new TimerWrap(env, std::forward<Args>(args)...);
env->AddCleanupHook(CleanupHook, this);
}

} // namespace node

#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#endif // SRC_TIMER_WRAP_INL_H_
18 changes: 3 additions & 15 deletions src/timer_wrap.cc
@@ -1,17 +1,12 @@
#include "timer_wrap.h" // NOLINT(build/include_inline)
#include "timer_wrap-inl.h"

#include "env-inl.h"
#include "memory_tracker-inl.h"
#include "timer_wrap.h"
#include "uv.h"

namespace node {

TimerWrap::TimerWrap(Environment* env, const TimerCb& fn)
: env_(env),
fn_(fn) {
uv_timer_init(env->event_loop(), &timer_);
timer_.data = this;
}

void TimerWrap::Stop() {
if (timer_.data == nullptr) return;
uv_timer_stop(&timer_);
Expand Down Expand Up @@ -48,13 +43,6 @@ void TimerWrap::OnTimeout(uv_timer_t* timer) {
t->fn_();
}

TimerWrapHandle::TimerWrapHandle(
Environment* env,
const TimerWrap::TimerCb& fn) {
timer_ = new TimerWrap(env, fn);
env->AddCleanupHook(CleanupHook, this);
}

void TimerWrapHandle::Stop() {
if (timer_ != nullptr)
return timer_->Stop();
Expand Down
9 changes: 5 additions & 4 deletions src/timer_wrap.h
Expand Up @@ -16,7 +16,9 @@ class TimerWrap final : public MemoryRetainer {
public:
using TimerCb = std::function<void()>;

TimerWrap(Environment* env, const TimerCb& fn);
template <typename... Args>
explicit inline TimerWrap(Environment* env, Args&&... args);

TimerWrap(const TimerWrap&) = delete;

inline Environment* env() const { return env_; }
Expand Down Expand Up @@ -50,9 +52,8 @@ class TimerWrap final : public MemoryRetainer {

class TimerWrapHandle : public MemoryRetainer {
public:
TimerWrapHandle(
Environment* env,
const TimerWrap::TimerCb& fn);
template <typename... Args>
explicit inline TimerWrapHandle(Environment* env, Args&&... args);

TimerWrapHandle(const TimerWrapHandle&) = delete;

Expand Down

0 comments on commit 9cc2bb7

Please sign in to comment.