[RFC,v2,01/16] apps: common: event_loop: Take callbacks by rvalue ref
diff mbox series

Message ID 20250114182143.1773762-2-pobrn@protonmail.com
State Superseded
Headers show
Series
  • apps: lc-compliance: Multi-stream tests
Related show

Commit Message

Barnabás Pőcze Jan. 14, 2025, 6:21 p.m. UTC
Using a const lvalue reference to `std::function<>` is not ideal
because it forces a copy to happen. Use an rvalue reference and
`std::move()` to avoid that.

Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
---
 src/apps/common/event_loop.cpp | 16 ++++++++--------
 src/apps/common/event_loop.h   |  8 ++++----
 2 files changed, 12 insertions(+), 12 deletions(-)

Comments

Jacopo Mondi Jan. 22, 2025, 8:56 a.m. UTC | #1
Hi Barnabás

On Tue, Jan 14, 2025 at 06:21:51PM +0000, Barnabás Pőcze wrote:
> Using a const lvalue reference to `std::function<>` is not ideal
> because it forces a copy to happen. Use an rvalue reference and
> `std::move()` to avoid that.
>
> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> ---
>  src/apps/common/event_loop.cpp | 16 ++++++++--------
>  src/apps/common/event_loop.h   |  8 ++++----
>  2 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/src/apps/common/event_loop.cpp b/src/apps/common/event_loop.cpp
> index f7f9afa0c..bc8cf17ab 100644
> --- a/src/apps/common/event_loop.cpp
> +++ b/src/apps/common/event_loop.cpp
> @@ -50,20 +50,20 @@ void EventLoop::exit(int code)
>  	event_base_loopbreak(base_);
>  }
>
> -void EventLoop::callLater(const std::function<void()> &func)
> +void EventLoop::callLater(std::function<void()> &&func)

I was very puzzled as I thought you were pointing out copies here

>  {
>  	{
>  		std::unique_lock<std::mutex> locker(lock_);
> -		calls_.push_back(func);
> +		calls_.push_back(std::move(func));

But they're instead here

I wonder how costly this actually is, if it's about copying a pointer
over it is not different than moving ? (if not that it invalidates the
passed-in object).

However, this doesn't hurt
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

>  	}
>
>  	event_base_once(base_, -1, EV_TIMEOUT, dispatchCallback, this, nullptr);
>  }
>
>  void EventLoop::addFdEvent(int fd, EventType type,
> -			   const std::function<void()> &callback)
> +			   std::function<void()> &&callback)
>  {
> -	std::unique_ptr<Event> event = std::make_unique<Event>(callback);
> +	std::unique_ptr<Event> event = std::make_unique<Event>(std::move(callback));
>  	short events = (type & Read ? EV_READ : 0)
>  		     | (type & Write ? EV_WRITE : 0)
>  		     | EV_PERSIST;
> @@ -85,9 +85,9 @@ void EventLoop::addFdEvent(int fd, EventType type,
>  }
>
>  void EventLoop::addTimerEvent(const std::chrono::microseconds period,
> -			      const std::function<void()> &callback)
> +			      std::function<void()> &&callback)
>  {
> -	std::unique_ptr<Event> event = std::make_unique<Event>(callback);
> +	std::unique_ptr<Event> event = std::make_unique<Event>(std::move(callback));
>  	event->event_ = event_new(base_, -1, EV_PERSIST, &EventLoop::Event::dispatch,
>  				  event.get());
>  	if (!event->event_) {
> @@ -131,8 +131,8 @@ void EventLoop::dispatchCall()
>  	call();
>  }
>
> -EventLoop::Event::Event(const std::function<void()> &callback)
> -	: callback_(callback), event_(nullptr)
> +EventLoop::Event::Event(std::function<void()> &&callback)
> +	: callback_(std::move(callback)), event_(nullptr)
>  {
>  }
>
> diff --git a/src/apps/common/event_loop.h b/src/apps/common/event_loop.h
> index ef129b9aa..d7d012c76 100644
> --- a/src/apps/common/event_loop.h
> +++ b/src/apps/common/event_loop.h
> @@ -33,18 +33,18 @@ public:
>  	int exec();
>  	void exit(int code = 0);
>
> -	void callLater(const std::function<void()> &func);
> +	void callLater(std::function<void()> &&func);
>
>  	void addFdEvent(int fd, EventType type,
> -			const std::function<void()> &handler);
> +			std::function<void()> &&handler);
>
>  	using duration = std::chrono::steady_clock::duration;
>  	void addTimerEvent(const std::chrono::microseconds period,
> -			   const std::function<void()> &handler);
> +			   std::function<void()> &&handler);
>
>  private:
>  	struct Event {
> -		Event(const std::function<void()> &callback);
> +		Event(std::function<void()> &&callback);
>  		~Event();
>
>  		static void dispatch(int fd, short events, void *arg);
> --
> 2.48.0
>
>

Patch
diff mbox series

diff --git a/src/apps/common/event_loop.cpp b/src/apps/common/event_loop.cpp
index f7f9afa0c..bc8cf17ab 100644
--- a/src/apps/common/event_loop.cpp
+++ b/src/apps/common/event_loop.cpp
@@ -50,20 +50,20 @@  void EventLoop::exit(int code)
 	event_base_loopbreak(base_);
 }
 
-void EventLoop::callLater(const std::function<void()> &func)
+void EventLoop::callLater(std::function<void()> &&func)
 {
 	{
 		std::unique_lock<std::mutex> locker(lock_);
-		calls_.push_back(func);
+		calls_.push_back(std::move(func));
 	}
 
 	event_base_once(base_, -1, EV_TIMEOUT, dispatchCallback, this, nullptr);
 }
 
 void EventLoop::addFdEvent(int fd, EventType type,
-			   const std::function<void()> &callback)
+			   std::function<void()> &&callback)
 {
-	std::unique_ptr<Event> event = std::make_unique<Event>(callback);
+	std::unique_ptr<Event> event = std::make_unique<Event>(std::move(callback));
 	short events = (type & Read ? EV_READ : 0)
 		     | (type & Write ? EV_WRITE : 0)
 		     | EV_PERSIST;
@@ -85,9 +85,9 @@  void EventLoop::addFdEvent(int fd, EventType type,
 }
 
 void EventLoop::addTimerEvent(const std::chrono::microseconds period,
-			      const std::function<void()> &callback)
+			      std::function<void()> &&callback)
 {
-	std::unique_ptr<Event> event = std::make_unique<Event>(callback);
+	std::unique_ptr<Event> event = std::make_unique<Event>(std::move(callback));
 	event->event_ = event_new(base_, -1, EV_PERSIST, &EventLoop::Event::dispatch,
 				  event.get());
 	if (!event->event_) {
@@ -131,8 +131,8 @@  void EventLoop::dispatchCall()
 	call();
 }
 
-EventLoop::Event::Event(const std::function<void()> &callback)
-	: callback_(callback), event_(nullptr)
+EventLoop::Event::Event(std::function<void()> &&callback)
+	: callback_(std::move(callback)), event_(nullptr)
 {
 }
 
diff --git a/src/apps/common/event_loop.h b/src/apps/common/event_loop.h
index ef129b9aa..d7d012c76 100644
--- a/src/apps/common/event_loop.h
+++ b/src/apps/common/event_loop.h
@@ -33,18 +33,18 @@  public:
 	int exec();
 	void exit(int code = 0);
 
-	void callLater(const std::function<void()> &func);
+	void callLater(std::function<void()> &&func);
 
 	void addFdEvent(int fd, EventType type,
-			const std::function<void()> &handler);
+			std::function<void()> &&handler);
 
 	using duration = std::chrono::steady_clock::duration;
 	void addTimerEvent(const std::chrono::microseconds period,
-			   const std::function<void()> &handler);
+			   std::function<void()> &&handler);
 
 private:
 	struct Event {
-		Event(const std::function<void()> &callback);
+		Event(std::function<void()> &&callback);
 		~Event();
 
 		static void dispatch(int fd, short events, void *arg);