src: prevent extra copies of TimerWrap::TimerCb

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 <[email protected]>
nodejs · Nov 2, 2021 · bc7efeb · bc7efeb
1 parent dd52c05
commit bc7efeb
Show file tree

Hide file tree

Showing 5 changed files with 42 additions and 20 deletions.
diff --git a/node.gyp b/node.gyp
@@ -642,6 +642,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',

diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc
@@ -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"
 

diff --git a/src/timer_wrap-inl.h b/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_
diff --git a/src/timer_wrap.cc b/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_);
@@ -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();

diff --git a/src/timer_wrap.h b/src/timer_wrap.h
@@ -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_; }
@@ -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;