From 8c4cdbe87bc9d0066c1f93ce8b97ca3df7d22fbd Mon Sep 17 00:00:00 2001 From: cheniujh <1271435567@qq.com> Date: Wed, 3 Jul 2024 18:26:48 +0800 Subject: [PATCH 1/8] changed the returned timeout of TimerTaskManager --- src/net/src/dispatch_thread.cc | 2 +- src/net/src/net_util.cc | 46 +++++++++++++--------------------- src/net/src/net_util.h | 12 ++++----- 3 files changed, 25 insertions(+), 35 deletions(-) diff --git a/src/net/src/dispatch_thread.cc b/src/net/src/dispatch_thread.cc index 647d254d10..6fbe97373e 100644 --- a/src/net/src/dispatch_thread.cc +++ b/src/net/src/dispatch_thread.cc @@ -66,7 +66,7 @@ int DispatchThread::StartThread() { // Adding timer tasks and run timertaskThread timer_task_thread_.AddTimerTask("blrpop_blocking_info_scan", 250, true, [this] { this->ScanExpiredBlockedConnsOfBlrpop(); }); - timer_task_thread_.set_thread_name("TimerTaskThread"); + timer_task_thread_.set_thread_name("DispacherTimerTaskThread"); timer_task_thread_.StartThread(); return ServerThread::StartThread(); } diff --git a/src/net/src/net_util.cc b/src/net/src/net_util.cc index 7efbb0f6cd..2669740ed4 100644 --- a/src/net/src/net_util.cc +++ b/src/net/src/net_util.cc @@ -35,31 +35,31 @@ uint32_t TimerTaskManager::AddTimerTask(const std::string& task_name, int interv int64_t next_expired_time = NowInMs() + interval_ms; exec_queue_.insert({next_expired_time, new_task.task_id}); - if (min_interval_ms_ > interval_ms || min_interval_ms_ == -1) { - min_interval_ms_ = interval_ms; - } // return the id of this task return new_task.task_id; } + int64_t TimerTaskManager::NowInMs() { auto now = std::chrono::system_clock::now(); return std::chrono::time_point_cast(now).time_since_epoch().count(); } -int TimerTaskManager::ExecTimerTask() { + +int32_t TimerTaskManager::ExecTimerTask() { std::vector fired_tasks_; int64_t now_in_ms = NowInMs(); - // traverse in ascending order - for (auto pair = exec_queue_.begin(); pair != exec_queue_.end(); pair++) { - if (pair->exec_ts <= now_in_ms) { - auto it = id_to_task_.find(pair->id); + // traverse in ascending order, and exec expired tasks + for (auto pair : exec_queue_) { + if (pair.exec_ts <= now_in_ms) { + auto it = id_to_task_.find(pair.id); assert(it != id_to_task_.end()); it->second.fun(); - fired_tasks_.push_back({pair->exec_ts, pair->id}); + fired_tasks_.push_back({pair.exec_ts, pair.id}); now_in_ms = NowInMs(); } else { break; } } + for (auto task : fired_tasks_) { exec_queue_.erase(task); auto it = id_to_task_.find(task.id); @@ -69,15 +69,19 @@ int TimerTaskManager::ExecTimerTask() { exec_queue_.insert({now_in_ms + it->second.interval_ms, task.id}); } else { // this task only need to be exec once, completely remove this task - int interval_del = it->second.interval_ms; id_to_task_.erase(task.id); - if (interval_del == min_interval_ms_) { - RenewMinIntervalMs(); - } } } - return min_interval_ms_; + + if (exec_queue_.empty()) { + //to avoid wasting of cpu resources, epoll use 5000ms as timeout value when no task to exec + return 5000; + } + int32_t gap_between_now_and_next_task = static_cast(exec_queue_.begin()->exec_ts - NowInMs()); + gap_between_now_and_next_task = gap_between_now_and_next_task < 0 ? 0 : gap_between_now_and_next_task; + return gap_between_now_and_next_task; } + bool TimerTaskManager::DelTimerTaskByTaskId(uint32_t task_id) { // remove the task auto task_to_del = id_to_task_.find(task_id); @@ -87,11 +91,6 @@ bool TimerTaskManager::DelTimerTaskByTaskId(uint32_t task_id) { int interval_del = task_to_del->second.interval_ms; id_to_task_.erase(task_to_del); - // renew the min_interval_ms_ - if (interval_del == min_interval_ms_) { - RenewMinIntervalMs(); - } - // remove from exec queue ExecTsWithId target_key = {-1, 0}; for (auto pair : exec_queue_) { @@ -106,15 +105,6 @@ bool TimerTaskManager::DelTimerTaskByTaskId(uint32_t task_id) { return true; } -void TimerTaskManager::RenewMinIntervalMs() { - min_interval_ms_ = -1; - for (auto pair : id_to_task_) { - if (pair.second.interval_ms < min_interval_ms_ || min_interval_ms_ == -1) { - min_interval_ms_ = pair.second.interval_ms; - } - } -} - TimerTaskThread::~TimerTaskThread() { if (!timer_task_manager_.Empty()) { LOG(INFO) << "TimerTaskThread exit !!!"; diff --git a/src/net/src/net_util.h b/src/net/src/net_util.h index 7b8cd364f3..02a9696969 100644 --- a/src/net/src/net_util.h +++ b/src/net/src/net_util.h @@ -59,24 +59,24 @@ class TimerTaskManager { ~TimerTaskManager() = default; uint32_t AddTimerTask(const std::string& task_name, int interval_ms, bool repeat_exec, const std::function &task); - //return the newest min_minterval_ms + //return the time gap between now and next task-expired time, which can be used as the timeout value of epoll int ExecTimerTask(); bool DelTimerTaskByTaskId(uint32_t task_id); - int GetMinIntervalMs() const { return min_interval_ms_; } int64_t NowInMs(); - void RenewMinIntervalMs(); - bool Empty(){ return 0 == last_task_id_; } + bool Empty() const { return 0 == last_task_id_; } private: //items stored in std::set are ascending ordered, we regard it as an auto sorted queue std::set exec_queue_; std::unordered_map id_to_task_; uint32_t last_task_id_{0}; - int min_interval_ms_{-1}; }; - +/* + * For simplicity, current version of TimerTaskThread has no lock inside and all task should be registered before TimerTaskThread started, + * but if you have the needs of dynamically add/remove timer task after TimerTaskThread started, you can simply add a mutex to protect the timer_task_manager_ + */ class TimerTaskThread : public Thread { public: TimerTaskThread(){ From 2a1b95075f47355773f707a9896287161faccaf6 Mon Sep 17 00:00:00 2001 From: cheniujh <1271435567@qq.com> Date: Wed, 10 Jul 2024 18:00:40 +0800 Subject: [PATCH 2/8] add comment for static cast --- src/net/src/net_util.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/net/src/net_util.cc b/src/net/src/net_util.cc index 2669740ed4..a7722bc469 100644 --- a/src/net/src/net_util.cc +++ b/src/net/src/net_util.cc @@ -77,6 +77,8 @@ int32_t TimerTaskManager::ExecTimerTask() { //to avoid wasting of cpu resources, epoll use 5000ms as timeout value when no task to exec return 5000; } + // gap_between_now_and_next_task will be used as epoll_wait's para 'timeout' and which is an int32_t, + // so here we explicitly cast it to int32_t instead of let it be implicitly truncated when passing it to epoll_wait. int32_t gap_between_now_and_next_task = static_cast(exec_queue_.begin()->exec_ts - NowInMs()); gap_between_now_and_next_task = gap_between_now_and_next_task < 0 ? 0 : gap_between_now_and_next_task; return gap_between_now_and_next_task; From 0409f3f6f4aa19ff042bec10926925b5dc56dc8e Mon Sep 17 00:00:00 2001 From: cheniujh <1271435567@qq.com> Date: Thu, 18 Jul 2024 14:33:03 +0800 Subject: [PATCH 3/8] revised based on opinions --- src/net/src/net_util.cc | 8 ++++---- src/net/src/net_util.h | 12 +++++------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/src/net/src/net_util.cc b/src/net/src/net_util.cc index a7722bc469..4d35d75198 100644 --- a/src/net/src/net_util.cc +++ b/src/net/src/net_util.cc @@ -44,7 +44,7 @@ int64_t TimerTaskManager::NowInMs() { return std::chrono::time_point_cast(now).time_since_epoch().count(); } -int32_t TimerTaskManager::ExecTimerTask() { +int64_t TimerTaskManager::ExecTimerTask() { std::vector fired_tasks_; int64_t now_in_ms = NowInMs(); // traverse in ascending order, and exec expired tasks @@ -79,7 +79,7 @@ int32_t TimerTaskManager::ExecTimerTask() { } // gap_between_now_and_next_task will be used as epoll_wait's para 'timeout' and which is an int32_t, // so here we explicitly cast it to int32_t instead of let it be implicitly truncated when passing it to epoll_wait. - int32_t gap_between_now_and_next_task = static_cast(exec_queue_.begin()->exec_ts - NowInMs()); + int64_t gap_between_now_and_next_task = exec_queue_.begin()->exec_ts - NowInMs(); gap_between_now_and_next_task = gap_between_now_and_next_task < 0 ? 0 : gap_between_now_and_next_task; return gap_between_now_and_next_task; } @@ -132,9 +132,9 @@ int TimerTaskThread::StopThread() { } void* TimerTaskThread::ThreadMain() { - int timeout; + int32_t timeout; while (!should_stop()) { - timeout = timer_task_manager_.ExecTimerTask(); + timeout = static_cast(timer_task_manager_.ExecTimerTask()); net_multiplexer_->NetPoll(timeout); } return nullptr; diff --git a/src/net/src/net_util.h b/src/net/src/net_util.h index 02a9696969..d426e27b9c 100644 --- a/src/net/src/net_util.h +++ b/src/net/src/net_util.h @@ -21,9 +21,9 @@ namespace net { int Setnonblocking(int sockfd); - +using TimerTaskID = int64_t; struct TimedTask{ - uint32_t task_id; + TimerTaskID task_id; std::string task_name; int interval_ms; bool repeat_exec; @@ -34,7 +34,7 @@ struct ExecTsWithId { //the next exec time of the task, unit in ms int64_t exec_ts; //id of the task to be exec - uint32_t id; + TimerTaskID id; bool operator<(const ExecTsWithId& other) const{ if(exec_ts == other.exec_ts){ @@ -57,19 +57,17 @@ class TimerTaskManager { public: TimerTaskManager() = default; ~TimerTaskManager() = default; - uint32_t AddTimerTask(const std::string& task_name, int interval_ms, bool repeat_exec, const std::function &task); //return the time gap between now and next task-expired time, which can be used as the timeout value of epoll - int ExecTimerTask(); + int64_t ExecTimerTask(); bool DelTimerTaskByTaskId(uint32_t task_id); int64_t NowInMs(); bool Empty() const { return 0 == last_task_id_; } - private: //items stored in std::set are ascending ordered, we regard it as an auto sorted queue std::set exec_queue_; std::unordered_map id_to_task_; - uint32_t last_task_id_{0}; + TimerTaskID last_task_id_{0}; }; From b01e41a7c4049201b796ddb080b53a5d03733d9b Mon Sep 17 00:00:00 2001 From: cheniujh <1271435567@qq.com> Date: Thu, 18 Jul 2024 14:38:41 +0800 Subject: [PATCH 4/8] change the Empty() logic --- src/net/src/net_util.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/net/src/net_util.h b/src/net/src/net_util.h index d426e27b9c..a6777490dc 100644 --- a/src/net/src/net_util.h +++ b/src/net/src/net_util.h @@ -62,7 +62,7 @@ class TimerTaskManager { int64_t ExecTimerTask(); bool DelTimerTaskByTaskId(uint32_t task_id); int64_t NowInMs(); - bool Empty() const { return 0 == last_task_id_; } + bool Empty() const { return exec_queue_.empty(); } private: //items stored in std::set are ascending ordered, we regard it as an auto sorted queue std::set exec_queue_; From ed1a113d211c2d00f7d0ca52d60754b1fd427bd1 Mon Sep 17 00:00:00 2001 From: cheniujh <1271435567@qq.com> Date: Thu, 18 Jul 2024 14:40:51 +0800 Subject: [PATCH 5/8] remove duplicated comments --- src/net/src/net_util.h | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/net/src/net_util.h b/src/net/src/net_util.h index a6777490dc..4cccce36bc 100644 --- a/src/net/src/net_util.h +++ b/src/net/src/net_util.h @@ -47,12 +47,6 @@ struct ExecTsWithId { } }; -/* - * For simplicity, current version of TimerTaskThread has no lock inside and all task should be registered before TimerTaskThread started, - * but if you have the needs of dynamically add/remove timer task after TimerTaskThread started, you can simply add a mutex to protect - * the timer_task_manager_ and also a pipe to wake up the maybe being endless-wait epoll(if all task consumed, epoll will sink into - * endless wait) to implement the feature. - */ class TimerTaskManager { public: TimerTaskManager() = default; From 2c078543846dde3dd506590a281ab9e38c3895cb Mon Sep 17 00:00:00 2001 From: cheniujh <1271435567@qq.com> Date: Thu, 18 Jul 2024 14:33:03 +0800 Subject: [PATCH 6/8] revised based on opinions --- src/net/src/net_util.cc | 4 ++-- src/net/src/net_util.h | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/net/src/net_util.cc b/src/net/src/net_util.cc index 4d35d75198..334326a8c2 100644 --- a/src/net/src/net_util.cc +++ b/src/net/src/net_util.cc @@ -27,7 +27,7 @@ int Setnonblocking(int sockfd) { return flags; } -uint32_t TimerTaskManager::AddTimerTask(const std::string& task_name, int interval_ms, bool repeat_exec, +TimerTaskID TimerTaskManager::AddTimerTask(const std::string& task_name, int interval_ms, bool repeat_exec, const std::function& task) { TimedTask new_task = {last_task_id_++, task_name, interval_ms, repeat_exec, task}; id_to_task_[new_task.task_id] = new_task; @@ -84,7 +84,7 @@ int64_t TimerTaskManager::ExecTimerTask() { return gap_between_now_and_next_task; } -bool TimerTaskManager::DelTimerTaskByTaskId(uint32_t task_id) { +bool TimerTaskManager::DelTimerTaskByTaskId(TimerTaskID task_id) { // remove the task auto task_to_del = id_to_task_.find(task_id); if (task_to_del == id_to_task_.end()) { diff --git a/src/net/src/net_util.h b/src/net/src/net_util.h index 4cccce36bc..b30806c3b0 100644 --- a/src/net/src/net_util.h +++ b/src/net/src/net_util.h @@ -51,16 +51,16 @@ class TimerTaskManager { public: TimerTaskManager() = default; ~TimerTaskManager() = default; - uint32_t AddTimerTask(const std::string& task_name, int interval_ms, bool repeat_exec, const std::function &task); + TimerTaskID AddTimerTask(const std::string& task_name, int interval_ms, bool repeat_exec, const std::function &task); //return the time gap between now and next task-expired time, which can be used as the timeout value of epoll int64_t ExecTimerTask(); - bool DelTimerTaskByTaskId(uint32_t task_id); + bool DelTimerTaskByTaskId(TimerTaskID task_id); int64_t NowInMs(); bool Empty() const { return exec_queue_.empty(); } private: //items stored in std::set are ascending ordered, we regard it as an auto sorted queue std::set exec_queue_; - std::unordered_map id_to_task_; + std::unordered_map id_to_task_; TimerTaskID last_task_id_{0}; }; @@ -80,11 +80,11 @@ class TimerTaskThread : public Thread { int StopThread() override; void set_thread_name(const std::string& name) override { Thread::set_thread_name(name); } - uint32_t AddTimerTask(const std::string& task_name, int interval_ms, bool repeat_exec, const std::function &task){ + TimerTaskID AddTimerTask(const std::string& task_name, int interval_ms, bool repeat_exec, const std::function &task){ return timer_task_manager_.AddTimerTask(task_name, interval_ms, repeat_exec, task); }; - bool DelTimerTaskByTaskId(uint32_t task_id){ + bool DelTimerTaskByTaskId(TimerTaskID task_id){ return timer_task_manager_.DelTimerTaskByTaskId(task_id); }; From 09dd7a1219fabf9f42222d8797519c3207cf9263 Mon Sep 17 00:00:00 2001 From: cheniujh <1271435567@qq.com> Date: Thu, 18 Jul 2024 18:03:19 +0800 Subject: [PATCH 7/8] revised from auto pair to const auto& task --- src/net/src/net_util.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/net/src/net_util.cc b/src/net/src/net_util.cc index 334326a8c2..361395acbd 100644 --- a/src/net/src/net_util.cc +++ b/src/net/src/net_util.cc @@ -48,12 +48,12 @@ int64_t TimerTaskManager::ExecTimerTask() { std::vector fired_tasks_; int64_t now_in_ms = NowInMs(); // traverse in ascending order, and exec expired tasks - for (auto pair : exec_queue_) { - if (pair.exec_ts <= now_in_ms) { - auto it = id_to_task_.find(pair.id); + for (const auto& task : exec_queue_) { + if (task.exec_ts <= now_in_ms) { + auto it = id_to_task_.find(task.id); assert(it != id_to_task_.end()); it->second.fun(); - fired_tasks_.push_back({pair.exec_ts, pair.id}); + fired_tasks_.push_back({task.exec_ts, task.id}); now_in_ms = NowInMs(); } else { break; From 72e9d495ae2668a98650c4f92352de938fb2b8de Mon Sep 17 00:00:00 2001 From: cheniujh <1271435567@qq.com> Date: Thu, 18 Jul 2024 18:08:12 +0800 Subject: [PATCH 8/8] removed some comments --- src/net/src/net_util.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/net/src/net_util.cc b/src/net/src/net_util.cc index 361395acbd..c52c07f80d 100644 --- a/src/net/src/net_util.cc +++ b/src/net/src/net_util.cc @@ -77,8 +77,7 @@ int64_t TimerTaskManager::ExecTimerTask() { //to avoid wasting of cpu resources, epoll use 5000ms as timeout value when no task to exec return 5000; } - // gap_between_now_and_next_task will be used as epoll_wait's para 'timeout' and which is an int32_t, - // so here we explicitly cast it to int32_t instead of let it be implicitly truncated when passing it to epoll_wait. + int64_t gap_between_now_and_next_task = exec_queue_.begin()->exec_ts - NowInMs(); gap_between_now_and_next_task = gap_between_now_and_next_task < 0 ? 0 : gap_between_now_and_next_task; return gap_between_now_and_next_task;