fix: revised TimerTaskManager and add some comments #2776
Conversation
WalkthroughThe changes primarily focus on renaming variables and updating thread names in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
☢️ Bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/net/src/dispatch_thread.cc (2 hunks)
- src/net/src/dispatch_thread.h (1 hunks)
- src/net/src/net_util.cc (4 hunks)
- src/net/src/net_util.h (1 hunks)
Files skipped from review due to trivial changes (1)
- src/net/src/dispatch_thread.h
Additional comments not posted (7)
src/net/src/net_util.h (3)
56-56: Comment addition is clear and useful.The comment provides clarity on the return value of the
AddTimerTaskmethod.
57-57: Return type change is appropriate.Changing the return type to
int32_tensures type consistency and precision.
60-60: Const-correctness improvement.Updating the
Emptymethod to be a const member function ensures it does not modify the object's state.src/net/src/net_util.cc (2)
70-75: Comment addition is clear and useful.The detailed comment provides guidance for future enhancements, improving maintainability.
Line range hint
47-84: Logic update and return type change are appropriate.The updates simplify the timeout calculation for
epoll_waitand improve type consistency. Ensure the function's behavior is verified.Verification successful
Verified: The logic update and return type change in
ExecTimerTaskare appropriate.The updates simplify the timeout calculation for
epoll_waitand improve type consistency. The function's behavior is correctly handled inTimerTaskThread::ThreadMain.
src/net/src/net_util.cc:ExecTimerTaskis used to set the timeout forNetPoll.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the behavior of the updated ExecTimerTask method. # Test: Search for the function usage. Expect: Proper handling of the new logic. rg --type cpp $'ExecTimerTask'Length of output: 218
Script:
#!/bin/bash # Extract lines around the usage of ExecTimerTask in src/net/src/net_util.cc rg --type cpp -A 10 -B 10 'ExecTimerTask' src/net/src/net_util.ccLength of output: 1197
src/net/src/dispatch_thread.cc (2)
67-70: Naming consistency improvements.Renaming
timerTaskThread_totimer_task_thread_and updating the thread name to "DispacherTimerTaskThread" improve clarity and consistency.
91-91: Naming consistency improvement.Renaming
timerTaskThread_totimer_task_thread_improves clarity and consistency.
|
“下一个要执行的定时任务的过期时间戳 - 当前时间戳，直接让epoll等待这个差值即可，没必要找什么mininum time interval，减去这部分逻辑的同时还能降低复杂度” 关键是 epoll wait 的过期时间并不精准呀 |
"The expiration timestamp of the next scheduled task to be executed - the current timestamp, just let epoll wait for the difference. There is no need to find a minimum time interval. Subtracting this part of the logic can also reduce the complexity." The key is The expiration time of epoll wait is not accurate. |
src/net/src/net_util.cc
Outdated
| return min_interval_ms_; | ||
|
|
||
| if (exec_queue_.empty()) { | ||
| //no task to exec, epoll will use -1 as timeout value, and sink into endless wait |
There was a problem hiding this comment.
好嘛，原始的 RenewMinIntervalMs() 函数避免使用 -1，你这干脆来个 -1
src/net/src/net_util.cc
Outdated
| @@ -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) { | |||
|
epoll_wait timeout 设置为 -1 尤为不合适，咱们这里一个网络 worker 是要处理网络和超时事件，如果只处理网络，那你设定为 -1 还是可以的 |
It is particularly inappropriate to set epoll_wait timeout to -1. We have a network worker here that handles network and timeout events. If it only handles the network, then you can set it to -1. |
This reverts commit a6f26ff.
2 add comments for TimerTaskManager
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/net/src/dispatch_thread.cc (2 hunks)
- src/net/src/net_util.h (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/net/src/dispatch_thread.cc
- src/net/src/net_util.h
AlexStocks
merged commit 1de6598
into
OpenAtomFoundation:unstable
* 1 rename DisPatcherThread::timerTaskThread_ to timer_task_thread_ * 2 add comments for TimerTaskManager --------- Co-authored-by: cheniujh <[email protected]>
…on#2776) * 1 rename DisPatcherThread::timerTaskThread_ to timer_task_thread_ * 2 add comments for TimerTaskManager --------- Co-authored-by: cheniujh <[email protected]>
…on#2776) * 1 rename DisPatcherThread::timerTaskThread_ to timer_task_thread_ * 2 add comments for TimerTaskManager --------- Co-authored-by: cheniujh <[email protected]>
1 原本的TimerTaskManager正确性没有问题，但是逻辑有改进的空间，具体地：TimerTaskManager::ExecTimerTask()会返回一个timeout, 用于给epoll_wait作为timeout使用，原本返回的这个timeout值是当前定时器内所有任务中执行间隔最小的那个时间间隔的值，但实际上可以直接返回：下一个要执行的定时任务的过期时间戳 - 当前时间戳，直接让epoll等待这个差值即可，没必要找什么mininum time interval，减去这部分逻辑的同时还能降低复杂度(经过讨论，如果只是为了精简逻辑，增加可读性的话，还是暂时不动这块逻辑，所以本pr去掉了这部分的改动，后续如果有改造的需要，可以再去参考本pr的历史commit)
2 加了一些注释，如果将来要增加动态添加/删除定时任务的特性（届时如果有这个需求的话），可以按照注释的思路继续改造
3 规范了一处命名以和pika保持一致：timerTaskThread_改为了 timer_task_thread_
1 The correctness of the original TimerTaskManager was not an issue, but there is room for logical improvement. Specifically, TimerTaskManager::ExecTimerTask() returns a timeout that is used as the timeout for epoll_wait. Originally, this returned timeout value was the minimum interval among all tasks in the current timer. However, it can directly return the difference between the expiration timestamp of the next scheduled task and the current timestamp. This difference can be used directly for epoll to wait, without the need to find the minimum time interval.2 Added some comments. If there is a future requirement to add features for dynamically adding/deleting timer tasks, the modifications can be made following the approach outlined in the comments.
3 Standardized a naming convention to keep it consistent with pika:
timerTaskThread_was changed totimer_task_thread_.