[RFC,v2,04/16] apps: common: event_loop: Use single event source for deferred calls
diff mbox series

Message ID 20250114182143.1773762-5-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:22 p.m. UTC
Instead of calling `event_base_once()` every time a deferred call
is added to the loop, create an event source at construction, and
simply trigger that when a new deferred call is scheduled.

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

Comments

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

On Tue, Jan 14, 2025 at 06:22:05PM +0000, Barnabás Pőcze wrote:
> Instead of calling `event_base_once()` every time a deferred call
> is added to the loop, create an event source at construction, and
> simply trigger that when a new deferred call is scheduled.

I'm not familiar with libevent, so it's a bit hard for me to grasp why
this is better.

>
> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> ---
>  src/apps/common/event_loop.cpp | 48 +++++++++++++++++-----------------
>  src/apps/common/event_loop.h   |  5 +---
>  2 files changed, 25 insertions(+), 28 deletions(-)
>
> diff --git a/src/apps/common/event_loop.cpp b/src/apps/common/event_loop.cpp
> index bc8cf17ab..b6230f4ba 100644
> --- a/src/apps/common/event_loop.cpp
> +++ b/src/apps/common/event_loop.cpp
> @@ -21,12 +21,35 @@ EventLoop::EventLoop()
>  	evthread_use_pthreads();
>  	base_ = event_base_new();
>  	instance_ = this;
> +
> +	callsTrigger_ = event_new(base_, -1, EV_PERSIST, [](evutil_socket_t, short, void *closure) {
> +		auto *self = static_cast<EventLoop *>(closure);
> +
> +		for (;;) {
> +			std::function<void()> call;
> +
> +			{
> +				std::lock_guard locker(self->lock_);
> +				if (self->calls_.empty())
> +					break;
> +
> +				call = std::move(self->calls_.front());
> +				self->calls_.pop_front();
> +			}
> +
> +			call();
> +		}
> +	}, this);
> +	assert(callsTrigger_);
> +	event_add(callsTrigger_, nullptr);
>  }
>
>  EventLoop::~EventLoop()
>  {
>  	instance_ = nullptr;
>
> +	event_free(callsTrigger_);
> +
>  	events_.clear();
>  	event_base_free(base_);
>  	libevent_global_shutdown();
> @@ -57,7 +80,7 @@ void EventLoop::callLater(std::function<void()> &&func)
>  		calls_.push_back(std::move(func));
>  	}
>
> -	event_base_once(base_, -1, EV_TIMEOUT, dispatchCallback, this, nullptr);
> +	event_active(callsTrigger_, 0, 0);
>  }
>
>  void EventLoop::addFdEvent(int fd, EventType type,
> @@ -108,29 +131,6 @@ void EventLoop::addTimerEvent(const std::chrono::microseconds period,
>  	events_.push_back(std::move(event));
>  }
>
> -void EventLoop::dispatchCallback([[maybe_unused]] evutil_socket_t fd,
> -				 [[maybe_unused]] short flags, void *param)
> -{
> -	EventLoop *loop = static_cast<EventLoop *>(param);
> -	loop->dispatchCall();
> -}
> -
> -void EventLoop::dispatchCall()
> -{
> -	std::function<void()> call;
> -
> -	{
> -		std::unique_lock<std::mutex> locker(lock_);
> -		if (calls_.empty())
> -			return;
> -
> -		call = calls_.front();
> -		calls_.pop_front();
> -	}
> -
> -	call();
> -}
> -
>  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 760075885..aac5ed3cc 100644
> --- a/src/apps/common/event_loop.h
> +++ b/src/apps/common/event_loop.h
> @@ -65,11 +65,8 @@ private:
>  	int exitCode_;
>
>  	std::deque<std::function<void()>> calls_;
> +	::event *callsTrigger_ = nullptr;
>
>  	std::list<std::unique_ptr<Event>> events_;
>  	std::mutex lock_;
> -
> -	static void dispatchCallback(evutil_socket_t fd, short flags,
> -				     void *param);
> -	void dispatchCall();
>  };
> --
> 2.48.0
>
>
Barnabás Pőcze Jan. 24, 2025, 9:23 a.m. UTC | #2
Hi


2025. január 22., szerda 10:23 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:

> Hi Barnabás
> 
> On Tue, Jan 14, 2025 at 06:22:05PM +0000, Barnabás Pőcze wrote:
> > Instead of calling `event_base_once()` every time a deferred call
> > is added to the loop, create an event source at construction, and
> > simply trigger that when a new deferred call is scheduled.
> 
> I'm not familiar with libevent, so it's a bit hard for me to grasp why
> this is better.

The motivation is to avoid the constant allocations and related bookkeeping
due to `event_base_once()` since starting from a later change all request
completions go through the `EventLoop::callLater()` mechanism.


Regards,
Barnabás Pőcze


> 
> >
> > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > ---
> >  src/apps/common/event_loop.cpp | 48 +++++++++++++++++-----------------
> >  src/apps/common/event_loop.h   |  5 +---
> >  2 files changed, 25 insertions(+), 28 deletions(-)
> >
> > diff --git a/src/apps/common/event_loop.cpp b/src/apps/common/event_loop.cpp
> > index bc8cf17ab..b6230f4ba 100644
> > --- a/src/apps/common/event_loop.cpp
> > +++ b/src/apps/common/event_loop.cpp
> > @@ -21,12 +21,35 @@ EventLoop::EventLoop()
> >  	evthread_use_pthreads();
> >  	base_ = event_base_new();
> >  	instance_ = this;
> > +
> > +	callsTrigger_ = event_new(base_, -1, EV_PERSIST, [](evutil_socket_t, short, void *closure) {
> > +		auto *self = static_cast<EventLoop *>(closure);
> > +
> > +		for (;;) {
> > +			std::function<void()> call;
> > +
> > +			{
> > +				std::lock_guard locker(self->lock_);
> > +				if (self->calls_.empty())
> > +					break;
> > +
> > +				call = std::move(self->calls_.front());
> > +				self->calls_.pop_front();
> > +			}
> > +
> > +			call();
> > +		}
> > +	}, this);
> > +	assert(callsTrigger_);
> > +	event_add(callsTrigger_, nullptr);
> >  }
> >
> >  EventLoop::~EventLoop()
> >  {
> >  	instance_ = nullptr;
> >
> > +	event_free(callsTrigger_);
> > +
> >  	events_.clear();
> >  	event_base_free(base_);
> >  	libevent_global_shutdown();
> > @@ -57,7 +80,7 @@ void EventLoop::callLater(std::function<void()> &&func)
> >  		calls_.push_back(std::move(func));
> >  	}
> >
> > -	event_base_once(base_, -1, EV_TIMEOUT, dispatchCallback, this, nullptr);
> > +	event_active(callsTrigger_, 0, 0);
> >  }
> >
> >  void EventLoop::addFdEvent(int fd, EventType type,
> > @@ -108,29 +131,6 @@ void EventLoop::addTimerEvent(const std::chrono::microseconds period,
> >  	events_.push_back(std::move(event));
> >  }
> >
> > -void EventLoop::dispatchCallback([[maybe_unused]] evutil_socket_t fd,
> > -				 [[maybe_unused]] short flags, void *param)
> > -{
> > -	EventLoop *loop = static_cast<EventLoop *>(param);
> > -	loop->dispatchCall();
> > -}
> > -
> > -void EventLoop::dispatchCall()
> > -{
> > -	std::function<void()> call;
> > -
> > -	{
> > -		std::unique_lock<std::mutex> locker(lock_);
> > -		if (calls_.empty())
> > -			return;
> > -
> > -		call = calls_.front();
> > -		calls_.pop_front();
> > -	}
> > -
> > -	call();
> > -}
> > -
> >  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 760075885..aac5ed3cc 100644
> > --- a/src/apps/common/event_loop.h
> > +++ b/src/apps/common/event_loop.h
> > @@ -65,11 +65,8 @@ private:
> >  	int exitCode_;
> >
> >  	std::deque<std::function<void()>> calls_;
> > +	::event *callsTrigger_ = nullptr;
> >
> >  	std::list<std::unique_ptr<Event>> events_;
> >  	std::mutex lock_;
> > -
> > -	static void dispatchCallback(evutil_socket_t fd, short flags,
> > -				     void *param);
> > -	void dispatchCall();
> >  };
> > --
> > 2.48.0
> >
> >
>

Patch
diff mbox series

diff --git a/src/apps/common/event_loop.cpp b/src/apps/common/event_loop.cpp
index bc8cf17ab..b6230f4ba 100644
--- a/src/apps/common/event_loop.cpp
+++ b/src/apps/common/event_loop.cpp
@@ -21,12 +21,35 @@  EventLoop::EventLoop()
 	evthread_use_pthreads();
 	base_ = event_base_new();
 	instance_ = this;
+
+	callsTrigger_ = event_new(base_, -1, EV_PERSIST, [](evutil_socket_t, short, void *closure) {
+		auto *self = static_cast<EventLoop *>(closure);
+
+		for (;;) {
+			std::function<void()> call;
+
+			{
+				std::lock_guard locker(self->lock_);
+				if (self->calls_.empty())
+					break;
+
+				call = std::move(self->calls_.front());
+				self->calls_.pop_front();
+			}
+
+			call();
+		}
+	}, this);
+	assert(callsTrigger_);
+	event_add(callsTrigger_, nullptr);
 }
 
 EventLoop::~EventLoop()
 {
 	instance_ = nullptr;
 
+	event_free(callsTrigger_);
+
 	events_.clear();
 	event_base_free(base_);
 	libevent_global_shutdown();
@@ -57,7 +80,7 @@  void EventLoop::callLater(std::function<void()> &&func)
 		calls_.push_back(std::move(func));
 	}
 
-	event_base_once(base_, -1, EV_TIMEOUT, dispatchCallback, this, nullptr);
+	event_active(callsTrigger_, 0, 0);
 }
 
 void EventLoop::addFdEvent(int fd, EventType type,
@@ -108,29 +131,6 @@  void EventLoop::addTimerEvent(const std::chrono::microseconds period,
 	events_.push_back(std::move(event));
 }
 
-void EventLoop::dispatchCallback([[maybe_unused]] evutil_socket_t fd,
-				 [[maybe_unused]] short flags, void *param)
-{
-	EventLoop *loop = static_cast<EventLoop *>(param);
-	loop->dispatchCall();
-}
-
-void EventLoop::dispatchCall()
-{
-	std::function<void()> call;
-
-	{
-		std::unique_lock<std::mutex> locker(lock_);
-		if (calls_.empty())
-			return;
-
-		call = calls_.front();
-		calls_.pop_front();
-	}
-
-	call();
-}
-
 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 760075885..aac5ed3cc 100644
--- a/src/apps/common/event_loop.h
+++ b/src/apps/common/event_loop.h
@@ -65,11 +65,8 @@  private:
 	int exitCode_;
 
 	std::deque<std::function<void()>> calls_;
+	::event *callsTrigger_ = nullptr;
 
 	std::list<std::unique_ptr<Event>> events_;
 	std::mutex lock_;
-
-	static void dispatchCallback(evutil_socket_t fd, short flags,
-				     void *param);
-	void dispatchCall();
 };