From 359e2abada16357ccf5074f0b312ab9cd4300d86 Mon Sep 17 00:00:00 2001
From: Darshan Sen <darshan.sen@postman.com>
Date: Sat, 13 Nov 2021 03:17:42 +0530
Subject: [PATCH] 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 <darshan.sen@postman.com>

PR-URL: https://github.com/nodejs/node/pull/40665
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
---
 node.gyp               |  1 +
 src/inspector_agent.cc |  2 +-
 src/timer_wrap-inl.h   | 32 ++++++++++++++++++++++++++++++++
 src/timer_wrap.cc      | 18 +++---------------
 src/timer_wrap.h       |  9 +++++----
 5 files changed, 42 insertions(+), 20 deletions(-)
 create mode 100644 src/timer_wrap-inl.h

diff --git a/node.gyp b/node.gyp
index 2052917d7ee338..d881e8509d87ad 100644
--- a/node.gyp
+++ b/node.gyp
@@ -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',
diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc
index c4a3322c6d972f..fd9f514b9b6a7b 100644
--- 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
new file mode 100644
index 00000000000000..d60ea15ade7078
--- /dev/null
+++ 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
index 6660b8c958810c..2b4457df818b27 100644
--- 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
index dbc23b442bea39..ac8f00f0d470f5 100644
--- 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;