You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "zhengJade (via GitHub)" <gi...@apache.org> on 2024/11/13 11:39:09 UTC
[PR] epoll bthread deal first (brpc)
zhengJade opened a new pull request, #2819:
URL: https://github.com/apache/brpc/pull/2819
### What problem does this PR solve?
对 brpc 先有性能做了提升
Problem Summary:
目前的 brpc 网络事件存在两个问题
1. epoll 线程和其他 bthread 具有相同优先级，导致 epoll 线程可能被一小段时间在队列中无响应，引起 worker queue 任务数量分布不均匀，多对一的 steal。
2. 网络事件无差别 signal，增加了系统调用。
### What is changed and the side effects?
epoll 线程不在 queue 中，独立出来
Side effects:
- Performance effects(性能影响): 性能平均提升 20%-25%
- Breaking backward compatibility(向后兼容性): 兼容
### 压测结果
- worker num：9
- core: 64
- arch: Intel(R) Xeon(R) CPU E5-2640 v3 @ 2.60GHz
- 工具 rpc_press
- proto: echo
#### 优化前
- 1w
- 5w
- 10w
#### 优化后
1w
- 5w
- 10w
---
### Check List:
- Please make sure your changes are compilable(请确保你的更改可以通过编译).
- When providing us with a new feature, it is best to add related tests(如果你向我们增加一个新的功能, 请添加相关测试).
- Please follow [Contributor Covenant Code of Conduct](https://github.com/apache/brpc/blob/master/CODE_OF_CONDUCT.md).(请遵循贡献者准则).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2834043417
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r1995118485
##########
src/bthread/task_group.h:
##########
@@ -278,6 +285,7 @@ friend class TaskControl;
// Worker thread id.
pid_t _tid;
+ std::unordered_set<bthread_t> _epoll_tids;
Review Comment:
删除了
##########
src/bthread/task_control.cpp:
##########
@@ -575,4 +586,11 @@ bvar::LatencyRecorder* TaskControl::create_exposed_pending_time() {
return pt;
}
+void TaskControl::set_group_epoll_tid(bthread_tag_t tag, bthread_t tid) {
+ auto groups = tag_group(tag);
+ const size_t ngroup = tag_ngroup(tag).load(butil::memory_order_acquire);
Review Comment:
删除了
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r2006910823
##########
src/bthread/task_control.h:
##########
@@ -96,6 +97,12 @@ friend bthread_t init_for_pthread_stack_trace();
void stack_trace(std::ostream& os, bthread_t tid);
std::string stack_trace(bthread_t tid);
#endif // BRPC_BTHREAD_TRACER
+ // Only deal once when init epoll bthread.
+ void set_group_epoll_tid(bthread_tag_t tag, bthread_t tid);
+
+ void push_priority_q(bthread_tag_t tag, bthread_t tid) {
+ _priority_qs[tag].push(tid);
Review Comment:
@chenBright 再看看呢
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r2041500195
##########
src/brpc/event_dispatcher_epoll.cpp:
##########
@@ -70,7 +70,8 @@ int EventDispatcher::Start(const bthread_attr_t* consumer_thread_attr) {
// Set _thread_attr before creating epoll thread to make sure
// everyting seems sane to the thread.
_thread_attr = consumer_thread_attr ?
- *consumer_thread_attr : BTHREAD_ATTR_NORMAL;
+ *consumer_thread_attr : _thread_attr;
Review Comment:
> 默认的attr就是EPOLL，这里是不是可以改成这样更直观一些：
>
> ```
> if (consumer_thread_attr) {
> _thread_attr = *consumer_thread_attr | BTHREAD_GLOBAL_PRIORITY
> }
> ```
我仔细看了下，这里可以这样改的
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r2043383387
##########
src/bthread/task_control.cpp:
##########
@@ -430,6 +435,11 @@ int TaskControl::_destroy_group(TaskGroup* g) {
bool TaskControl::steal_task(bthread_t* tid, size_t* seed, size_t offset) {
auto tag = tls_task_group->tag();
+
+ if (_priority_qs[tag].steal(tid)) {
Review Comment:
同一个 tag 下的 worker 肯定会存在竞争的，这个应该算是预期之内，性能损失量级跟下文的 g->_rq.steal(tid) 相当，原来 steal 的竞争产生的损失约为 N * M（N 为 tag 数量，常数级，M 为 g->_rq.steal(tid) 单次时间），现在为 (N + 1) * M
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r1853350137
##########
src/bthread/task_control.cpp:
##########
@@ -441,6 +453,18 @@ void TaskControl::signal_task(int num_task, bthread_tag_t tag) {
if (num_task > 2) {
num_task = 2;
}
+ if (ParkingLot::_waiting_count.load(std::memory_order_acquire) == 0) {
+ if (FLAGS_bthread_min_concurrency > 0 &&
+ _concurrency.load(butil::memory_order_relaxed) < FLAGS_bthread_concurrency) {
+ // TODO: Reduce this lock
+ BAIDU_SCOPED_LOCK(g_task_control_mutex);
+ if (_concurrency.load(butil::memory_order_acquire) < FLAGS_bthread_concurrency) {
+ add_workers(1, tag);
+ }
Review Comment:
这里没必要重复写一遍add_workers逻辑。在后面`if (num_task > 0)`中加上`ParkingLot::_waiting_count.load(std::memory_order_acquire) > 0`就可以了吧？
std::memory_order_acquire -> butil::memory_order_acquire ? 有必要用memory_order_acquire吗？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2899732430
> @zhengJade Thank you for your contribution!
PR 有问题直接圈我来修，最近换工作有点忙，才来看
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright merged PR #2819:
URL: https://github.com/apache/brpc/pull/2819
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2581958586
> 2\. bthread_attrflags_t
> 如果网络事件非常频繁的话，是不是可以直接让epoll thread不切走呢？比如把这里: https://github.com/apache/brpc/blob/master/src/brpc/socket.cpp#L2265 改成bthread_start_background是否也可以提升性能呢？
如果不切走，那非 epoll worker 是不是只能通过 steal 的方式来执行任务了？，队列更不均匀
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2526733718
> > * epoll 线程和其他 bthread 具有相同优先级，导致 epoll 线程可能被一小段时间在队列中无响应，引起 worker queue 任务数量分布不均匀，多对一的 steal。
> > * 网络事件无差别 signal，增加了系统调用。
>
> 能否分别测一下这两个优化各自提升多少性能呢？ 我觉得第二个可能更重要一些，而且也更通用，可以把第二个优化单独提一个PR，这样更容易合入
较少 signal 这个我单独优化过，基本没什么提升，然后分析了原因才发现是因为压力大的时候，Q 其实是不平均的，直接的一个原因就是 epoll 的 tid 有时候会很难被 steal 到，这导致所有的 worker 呈现出，要做完当前所有的任务才可以进行 epoll，在某一瞬间导致有些 worker 进入了 wait 状态，然后整体就看起来，worker 没有 wait 状态的时间很少很少，所以就是没啥作用，于是才想着更改 epoll 的优先级
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2489919968
能否在bthread这个层面上，创建协程的时候设置一个属性，使它具有更高的优先级呢？现在这里专门设置了epoll_XXX，感觉有点特化。
另外，总是让epoll的bthread优先调度的话，会不会造成堆积很多任务的情况呢？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2490512341
> > 总是让epoll的bthread优先调度的话，会不会造成堆积很多任务的情况呢？
>
> 现在只有一个连接，epoll bthread调度不是很频繁。是不是可以起多一些client和epoll bthread再测一下呢？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2526622151
> * epoll 线程和其他 bthread 具有相同优先级，导致 epoll 线程可能被一小段时间在队列中无响应，引起 worker queue 任务数量分布不均匀，多对一的 steal。
> * 网络事件无差别 signal，增加了系统调用。
能否分别测一下这两个优化各自提升多少性能呢？
我觉得第二个可能更重要一些，而且也更通用，可以把第二个优化单独提一个PR，这样更容易合入
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r1996856220
##########
src/bthread/task_control.h:
##########
@@ -96,6 +97,12 @@ friend bthread_t init_for_pthread_stack_trace();
void stack_trace(std::ostream& os, bthread_t tid);
std::string stack_trace(bthread_t tid);
#endif // BRPC_BTHREAD_TRACER
+ // Only deal once when init epoll bthread.
+ void set_group_epoll_tid(bthread_tag_t tag, bthread_t tid);
+
+ void push_priority_q(bthread_tag_t tag, bthread_t tid) {
+ _priority_qs[tag].push(tid);
Review Comment:
这一块需要修改的话，可以rebase一下。#2916 修复了一个导致UT经常失败的bug。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r2030365543
##########
src/bthread/task_control.h:
##########
@@ -153,6 +157,7 @@ friend bthread_t init_for_pthread_stack_trace();
std::vector<bvar::PassiveStatus<double>*> _tagged_cumulated_worker_time;
std::vector<bvar::PerSecond<bvar::PassiveStatus<double>>*> _tagged_worker_usage_second;
std::vector<bvar::Adder<int64_t>*> _tagged_nbthreads;
+ std::vector<WorkStealingQueue<bthread_t>> _priority_qs;
Review Comment:
_priority_queues
##########
src/bthread/task_control.h:
##########
@@ -97,6 +97,10 @@ friend bthread_t init_for_pthread_stack_trace();
std::string stack_trace(bthread_t tid);
#endif // BRPC_BTHREAD_TRACER
+ void push_priority_q(bthread_tag_t tag, bthread_t tid) {
Review Comment:
push_priority_queue
##########
src/bthread/task_control.cpp:
##########
@@ -430,6 +435,11 @@ int TaskControl::_destroy_group(TaskGroup* g) {
bool TaskControl::steal_task(bthread_t* tid, size_t* seed, size_t offset) {
auto tag = tls_task_group->tag();
+
+ if (_priority_qs[tag].steal(tid)) {
Review Comment:
所有线程都访问这个是否会造成全局竞争？
##########
src/brpc/event_dispatcher_epoll.cpp:
##########
@@ -70,7 +70,8 @@ int EventDispatcher::Start(const bthread_attr_t* consumer_thread_attr) {
// Set _thread_attr before creating epoll thread to make sure
// everyting seems sane to the thread.
_thread_attr = consumer_thread_attr ?
- *consumer_thread_attr : BTHREAD_ATTR_NORMAL;
+ *consumer_thread_attr : _thread_attr;
Review Comment:
默认的attr就是EPOLL，这里是不是可以改成这样更直观一些：
```
if (consumer_thread_attr) {
_thread_attr = *consumer_thread_attr | BTHREAD_GLOBAL_PRIORITY
}
```
##########
src/bthread/task_group.h:
##########
@@ -200,6 +200,8 @@ class TaskGroup {
return total_ns;
}
+ const bthread_attr_t& get_current_attr() { return _cur_meta->attr; }
Review Comment:
这个方法好像没有必要，目前只有task_group.cpp里面用到，那里可以直接访问到_cur_meta->attr
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2852876849
> > 我问一下，如果把epoll变成bthread_start_urgent和按照优先级队列处理epoll，最终执行的逻辑上看有什么区别呢？
>
> @yanglimingcn ”把epoll变成bthread_start_urgent“指的是哪部分？现在epoll bthread处理都是bthread_start_urgent处理读事件的吧。
我大概理解了，如果用bthread_start_background处理读事件，epoll会一直占有一个worker，这个PR不会让epoll一直占用一个worker，但是也会让epoll线程优先调度。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2899704705
@zhengJade Thank you for your contribution!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2899730819
> > > 我问一下，如果把epoll变成bthread_start_urgent和按照优先级队列处理epoll，最终执行的逻辑上看有什么区别呢？
> >
> >
> > @yanglimingcn ”把epoll变成bthread_start_urgent“指的是哪部分？现在epoll bthread处理都是bthread_start_urgent处理读事件的吧。
>
> 我大概理解了，如果用bthread_start_background处理读事件，epoll会一直占有一个worker，这个PR不会让epoll一直占用一个worker，但是也会让epoll线程优先调度。
是这样的，要让任务分在不同的 worker 而不能通过 steal 的方式
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-3089621989
> 多 client 场景中会出现大片 RPC 超时 [E110]Fail to connect Socket ... : Connection timed out [E1008]Reached timeout=2500ms
> 
@zhengJade 可能是#3030 这个问题导致的。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2703963457
> 如果确实希望epoll bthread 优先处理，把它启动的bthread_start_urgent变成可配置的bthread_start_urgent/background可行吗
这里的bthread_start_urgent是精心设计的吧，cache局部性更优。
epoll事件处理完之后，也还是有优先处理的需求吧？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2489940901
> 总是让epoll的bthread优先调度的话，会不会造成堆积很多任务的情况呢？
现在只有一个连接，epoll bthread调度不是很频繁。是不是可以起多一些client和epoll bthread再测一下呢？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2702675196
> 这个PR啥时候能merge上去啊
@zhoukangsheng 好像需要社区的大佬们再看看，很久没回我了
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2771867928
> LGTM
>
> @wwbmmm @yanglimingcn 有时间再看看。
这个还有希望合进去嘛
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r2193910342
##########
src/bthread/task_control.cpp:
##########
@@ -430,6 +435,11 @@ int TaskControl::_destroy_group(TaskGroup* g) {
bool TaskControl::steal_task(bthread_t* tid, size_t* seed, size_t offset) {
auto tag = tls_task_group->tag();
+
+ if (_priority_qs[tag].steal(tid)) {
Review Comment:
@zhengJade 我理解，原来的steal在遍历worker的时候起始点和步长不一样，遍历的worker会错开。但是现在变成了steal同一个，tag内的竞争变激烈了，那么这里的M（g->_rq.steal(tid) 单次时间）也会变大了。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "rowe0x (via GitHub)" <gi...@apache.org>.
rowe0x commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2576595080
优化前后长尾延迟没看到明显的变化？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "MJY-HUST (via GitHub)" <gi...@apache.org>.
MJY-HUST commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r1906923179
##########
src/bthread/parking_lot.h:
##########
@@ -30,6 +30,7 @@ namespace bthread {
// Park idle workers.
class BAIDU_CACHELINE_ALIGNMENT ParkingLot {
public:
+ static butil::atomic<int> _waiting_count;
Review Comment:
这里 `_waiting_count` 作为全局静态变量感觉不能有效减少 signal 的次数，因为即使在 `_waiting_count ` != 0 的场景，仍然存在parking_lot list中有的pl存在waiting thread，有的pl不存在。在 signal_task 中是先随机选择一个pl来signal，选择的 pl 不一定存在 waiting thread。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade closed pull request #2819: epoll bthread deal first
URL: https://github.com/apache/brpc/pull/2819
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2490551846
> 能否在bthread这个层面上，创建协程的时候设置一个属性，使它具有更高的优先级呢？现在这里专门设置了epoll_XXX，感觉有点特化。 另外，总是让epoll的bthread优先调度的话，会不会造成堆积很多任务的情况呢？
我觉得可以增加一个属性来表示高优先级，但是要说明一点，就是 epoll 的 任务被 steal 的前提肯定是有 worker 需要 steal from task_control，所以可证明 worker 自己的 queue 应该没有任务，证明他是空闲的，这个时候应该优先让其响应网络事件，这有两个好处
1. 保证网络任务能够及时响应。
2. 可以让所有的 worker 的 run_q 相对均匀，因为 q 中大量任务来源不是 steal，而是 deal event。
另一个问题，就是是否会造成堆积：
如果我们不响应 epoll 就会造成堆积，证明此刻的网络请求数量是大于 brpc 的处理能力的，否则我们不需要通过增加 steal 的次数这种比起 pop queue 更费时间的操作来加快某时刻处理存量 bthread 的能力。这是牺牲了 RT 带来的，感觉不合理。而且 QPS 高时候更容易造成 RT 抖动。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "wanghenshui (via GitHub)" <gi...@apache.org>.
wanghenshui commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2489943405
1 你说得对，是有点特化，这么改简单点
如果是带优先级的bthread 可能就得改调度了，涉及到任务抢占/饥饿问题，可能麻烦一点
2 你说的也对可能会堆积，但是epoll任务是比较少的
如果改了优先级，会存在你说的优先级高的任务太多导致其他任务饿死
Ted Mostly
***@***.***
---Original---
From: ***@***.***>
Date: Thu, Nov 21, 2024 10:10 AM
To: ***@***.***>;
Cc: "Ted ***@***.******@***.***>;
Subject: Re: [apache/brpc] epoll bthread deal first (PR #2819)
能否在bthread这个层面上，创建协程的时候设置一个属性，使它具有更高的优先级呢？现在这里专门设置了epoll_XXX，感觉有点特化。
另外，总是让epoll的bthread优先调度的话，会不会造成堆积很多任务的情况呢？
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.Message ID: ***@***.***>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2489938094
> 能否在bthread这个层面上，创建协程的时候设置一个属性，使它具有更高的优先级呢？现在这里专门设置了epoll_XXX，感觉有点特化。
+1。
进一步抽象出一个高优先级队列，使用WorkStealingQueue来实现：
1. 简化实现逻辑。
2. 后续还有高优先级的bthread，只需要设置对应的bthread_attrflags_t即可。
这样是不是更好呢？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2834123326
LGTM
> > 如果确实希望epoll bthread 优先处理，把它启动的bthread_start_urgent变成可配置的bthread_start_urgent/background可行吗
>
> 这里的bthread_start_urgent是精心设计的吧，cache局部性更优。
>
> 直接配成bthread_start_backgruond好像不太合适，epoll bthread相当于独占线程了？
>
> 最后一个事件处理使用bthread_start_urgent，之前的事件处理使用bthread_start_backgruond，这样是不是好一点？
@zhengJade @chenBright 我问一下，如果把epoll变成bthread_start_urgent和按照优先级队列处理epoll，最终执行的逻辑上看有什么区别呢？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2703793985
如果确实希望epoll bthread 优先处理，把它启动的bthread_start_urgent变成可配置的bthread_start_urgent/background可行吗
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2719769290
> > > 如果确实希望epoll bthread 优先处理，把它启动的bthread_start_urgent变成可配置的bthread_start_urgent/background可行吗
> >
> >
> > 这里的bthread_start_urgent是精心设计的吧，cache局部性更优。
> > 直接配成bthread_start_backgruond好像不太合适，epoll bthread相当于独占线程了？
> > 最后一个事件处理使用bthread_start_urgent，之前的事件处理使用bthread_start_backgruond，这样是不是好一点？
>
> @chenBright 这里如何得知是最后一个事件呢
目前没法知道。但是应该也会有类似的问题，bthread_start_backgruond出的bthread和epoll bthread也不一定能得到及时的调度。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r2041499486
##########
src/bthread/task_control.cpp:
##########
@@ -430,6 +435,11 @@ int TaskControl::_destroy_group(TaskGroup* g) {
bool TaskControl::steal_task(bthread_t* tid, size_t* seed, size_t offset) {
auto tag = tls_task_group->tag();
+
+ if (_priority_qs[tag].steal(tid)) {
Review Comment:
> 所有线程都访问这个是否会造成全局竞争？
@wwbmmm
这里的 _priority_qs 我记得本身就是线程安全的
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "wanghenshui (via GitHub)" <gi...@apache.org>.
wanghenshui commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2489942731
> 能否在bthread这个层面上，创建协程的时候设置一个属性，使它具有更高的优先级呢？现在这里专门设置了epoll_XXX，感觉有点特化。 另外，总是让epoll的bthread优先调度的话，会不会造成堆积很多任务的情况呢？
1 你说得对，是有点特化，这么改简单点
如果是带优先级的bthread 可能就得改调度了，涉及到任务抢占/饥饿问题，可能麻烦一点
2 你说的也对可能会堆积，但是epoll任务是比较少的
如果改了优先级，会存在你说的优先级高的任务太多导致其他任务饿死
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r1875215957
##########
src/bthread/task_control.cpp:
##########
@@ -441,6 +453,18 @@ void TaskControl::signal_task(int num_task, bthread_tag_t tag) {
if (num_task > 2) {
num_task = 2;
}
+ if (ParkingLot::_waiting_count.load(std::memory_order_acquire) == 0) {
+ if (FLAGS_bthread_min_concurrency > 0 &&
+ _concurrency.load(butil::memory_order_relaxed) < FLAGS_bthread_concurrency) {
+ // TODO: Reduce this lock
+ BAIDU_SCOPED_LOCK(g_task_control_mutex);
+ if (_concurrency.load(butil::memory_order_acquire) < FLAGS_bthread_concurrency) {
+ add_workers(1, tag);
+ }
Review Comment:
这里我当时写的时候其实不能够确定这一步是否一定要求 _concurrency 值的准确性，即使数量有误差，后面再判断的时候也能 add worker 出来，但是考虑到原来的使用的是 memory_order_acquire，就延用了
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r1875149987
##########
src/brpc/event_dispatcher.cpp:
##########
@@ -55,6 +55,7 @@ void InitializeGlobalDispatchers() {
FLAGS_usercode_in_pthread ? BTHREAD_ATTR_PTHREAD : BTHREAD_ATTR_NORMAL;
attr.tag = (BTHREAD_TAG_DEFAULT + i) % FLAGS_task_group_ntags;
CHECK_EQ(0, g_edisp[i * FLAGS_event_dispatcher_num + j].Start(&attr));
+ bthread_epoll_tid_set(i, g_edisp[i].Tid());
Review Comment:
这里应该是 g_edisp[i * FLAGS_event_dispatcher_num + j] 吧？
##########
src/brpc/event_dispatcher.h:
##########
@@ -133,6 +133,8 @@ template <typename T> friend class IOEvent;
// Returns 0 on success, -1 otherwise and errno is set
int UnregisterEvent(IOEventDataId event_data_id, int fd, bool pollin);
+ bthread_t Tid() const { return _tid; }
Review Comment:
简单getter用小写开头的全名方式
##########
src/bthread/parking_lot.h:
##########
@@ -30,6 +30,7 @@ namespace bthread {
// Park idle workers.
class BAIDU_CACHELINE_ALIGNMENT ParkingLot {
public:
+ static butil::atomic<int> _waiting_count;
Review Comment:
这个变量不应该是public的，可以提供一个getter函数
##########
src/bthread/task_group.h:
##########
@@ -189,6 +190,11 @@ class TaskGroup {
bthread_tag_t tag() const { return _tag; }
+ // Thread Unsafe
+ void add_epoll_tid(bthread_t tid) { _epoll_tids.emplace(tid); }
+
+ bool cur_epoll_tid() { return _epoll_tids.count(current_tid()) > 0; }
Review Comment:
叫is_current_epoll_tid更合适？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2526803281
如果网络事件非常频繁的话，是不是可以直接让epoll thread不切走呢？比如把这里: https://github.com/apache/brpc/blob/master/src/brpc/socket.cpp#L2265
改成bthread_start_background是否也可以提升性能呢？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r2041505792
##########
src/bthread/task_group.h:
##########
@@ -200,6 +200,8 @@ class TaskGroup {
return total_ns;
}
+ const bthread_attr_t& get_current_attr() { return _cur_meta->attr; }
Review Comment:
这个 func 是静态成员函数，好像是没办法直接使用非静态成员变量的
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r1992640728
##########
src/bthread/task_group.h:
##########
@@ -199,6 +200,10 @@ class TaskGroup {
total_ns += butil::cputhread_time_ns() - _last_cpu_clock_ns;
return total_ns;
}
+ // Thread Unsafe
+ void add_epoll_tid(bthread_t tid) { _epoll_tids.emplace(tid); }
Review Comment:
高优标记放在bthread TaskMeta::attr::flags中，是不是更合适？未来有其他高优需求，就不需要再额外支持了。
##########
src/bthread/task_group.h:
##########
@@ -278,6 +285,7 @@ friend class TaskControl;
// Worker thread id.
pid_t _tid;
+ std::unordered_set<bthread_t> _epoll_tids;
Review Comment:
用butil::FlatSet?
##########
src/bthread/task_control.cpp:
##########
@@ -575,4 +586,11 @@ bvar::LatencyRecorder* TaskControl::create_exposed_pending_time() {
return pt;
}
+void TaskControl::set_group_epoll_tid(bthread_tag_t tag, bthread_t tid) {
+ auto groups = tag_group(tag);
+ const size_t ngroup = tag_ngroup(tag).load(butil::memory_order_acquire);
Review Comment:
groups.load()？
##########
src/brpc/event_dispatcher.h:
##########
@@ -133,6 +133,8 @@ template <typename T> friend class IOEvent;
// Returns 0 on success, -1 otherwise and errno is set
int UnregisterEvent(IOEventDataId event_data_id, int fd, bool pollin);
+ bthread_t Tid() const { return _tid; }
Review Comment:
+1
##########
src/bthread/task_control.h:
##########
@@ -96,6 +97,12 @@ friend bthread_t init_for_pthread_stack_trace();
void stack_trace(std::ostream& os, bthread_t tid);
std::string stack_trace(bthread_t tid);
#endif // BRPC_BTHREAD_TRACER
+ // Only deal once when init epoll bthread.
+ void set_group_epoll_tid(bthread_tag_t tag, bthread_t tid);
+
+ void push_priority_q(bthread_tag_t tag, bthread_t tid) {
+ _priority_qs[tag].push(tid);
Review Comment:
如果_priority_qs扩容了，是不是会有问题？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2586114214
> > 能否在bthread这个层面上，创建协程的时候设置一个属性，使它具有更高的优先级呢？现在这里专门设置了epoll_XXX，感觉有点特化。
>
> +1。 进一步抽象出一个高优先级队列，使用WorkStealingQueue来实现：
>
> 1. 简化实现逻辑。
> 2. 后续还有高优先级的bthread，只需要设置对应的bthread_attrflags_t即可。
>
> 这样是不是更好呢？
@chenBright 这里我还是把队列加入到 control 里面了，没有采用之前的 attr + priority queue 的原因是因为只要是在 task_group 里面做优先级，很大概率会使的 epoll 任务始终被困在一个 worker 上，但是目前的问题不只是因为 epoll 没有被先做，而是因为 epoll 产生的任务无法很好的分在 worker queue 里面，最好是把 epoll 交给 control 管理，让下面的 worker 均匀的偷到。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "legionxiong (via GitHub)" <gi...@apache.org>.
legionxiong commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2833892270
> 多 client 场景中会出现大片 RPC 超时 [E110]Fail to connect Socket ... : Connection timed out [E1008]Reached timeout=2500ms 
从我这边测试的情况来看， 应该是进入到了 TaskGroup::push_rq 的死循环了。 有个 server 无法再和其他 client 建立 socket 连接， 我们在 server 端设置的 pthread worker 数量是 6
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r2004723967
##########
src/bthread/task_control.h:
##########
@@ -96,6 +97,12 @@ friend bthread_t init_for_pthread_stack_trace();
void stack_trace(std::ostream& os, bthread_t tid);
std::string stack_trace(bthread_t tid);
#endif // BRPC_BTHREAD_TRACER
+ // Only deal once when init epoll bthread.
+ void set_group_epoll_tid(bthread_tag_t tag, bthread_t tid);
+
+ void push_priority_q(bthread_tag_t tag, bthread_t tid) {
+ _priority_qs[tag].push(tid);
Review Comment:
我发现我其实初始化过了
那就不 rebase 了吧
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2702690765
PR https://github.com/apache/brpc/pull/2907 单独实现了第二个优化，看起来更优雅一些。
第一个优化可以再讨论一下 @chenBright
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r2062971688
##########
src/bthread/task_group.cpp:
##########
@@ -429,12 +429,19 @@ int TaskGroup::start_foreground(TaskGroup** pg,
} else {
// NOSIGNAL affects current task, not the new task.
RemainedFn fn = NULL;
- if (g->current_task()->about_to_quit) {
+ auto& cur_attr = g->_cur_meta->attr;
+ if (cur_attr.flags & BTHREAD_GLOBAL_PRIORITY) {
+ fn = priority_to_run;
+ } else if (g->current_task()->about_to_quit) {
fn = ready_to_run_in_worker_ignoresignal;
} else {
fn = ready_to_run_in_worker;
}
- ReadyToRunArgs args = { g->_cur_meta, (bool)(using_attr.flags & BTHREAD_NOSIGNAL) };
+ ReadyToRunArgs args = {
+ g->tag(),
Review Comment:
格式化一下这块？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2716289023
> > PR #2907 单独实现了第二个优化，看起来更优雅一些。 第一个优化可以再讨论一下 @chenBright
>
> @zhengJade #2907 已经合了，可以rebase master更新一下这个PR吗？
>
> 可以再测一下，观察#2897 加的指标，看看epoll调度是否变快了吗？
@chenBright 单独的第一个优化 #2907 其实没有任何优化，对系统的调用其实也没有减少，因为 brpc 的 worker 在 waiting 和 runnning 的状态之间切换十分频繁，所以 waiting count 总是大于 0，在我多次的测试下也发现，几百次的 wake 也就能触发一次 优化，所以我之前就不认为单独合入第一个优化有意义，感觉是为了合入而合入。我看看 #2897 咋用。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2717035278
> > PR #2907 单独实现了第二个优化，看起来更优雅一些。 第一个优化可以再讨论一下 @chenBright
>
> @zhengJade #2907 已经合了，可以rebase master更新一下这个PR吗？
>
> 可以再测一下，观察#2897 加的指标，看看epoll调度是否变快了吗？
@chenBright 以下是 5w QPS，3 客户端，压了一段时间后 rt， p9999，readQPS 的表现
更新前
更新后
可以看到的是，rt 上有很小幅度的上升，但是 p9999 会更好也会更稳定，同时 readQPS 也更高，因此 RT 有一点小幅度上升是合理的。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r1995119831
##########
src/bthread/task_control.h:
##########
@@ -96,6 +97,12 @@ friend bthread_t init_for_pthread_stack_trace();
void stack_trace(std::ostream& os, bthread_t tid);
std::string stack_trace(bthread_t tid);
#endif // BRPC_BTHREAD_TRACER
+ // Only deal once when init epoll bthread.
+ void set_group_epoll_tid(bthread_tag_t tag, bthread_t tid);
+
+ void push_priority_q(bthread_tag_t tag, bthread_t tid) {
+ _priority_qs[tag].push(tid);
Review Comment:
_priority_qs 目前其实不会扩容，因为 epoll 的 tid 数量在启动的时候就决定了，而且使用的是 workStealQueue，这个应该是线程安全的吧
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2716293661
> > 如果确实希望epoll bthread 优先处理，把它启动的bthread_start_urgent变成可配置的bthread_start_urgent/background可行吗
>
> 这里的bthread_start_urgent是精心设计的吧，cache局部性更优。
>
> 直接配成bthread_start_backgruond好像不太合适，epoll bthread相当于独占线程了？
>
> 最后一个事件处理使用bthread_start_urgent，之前的事件处理使用bthread_start_backgruond，这样是不是好一点？
@chenBright 这里如何得知是最后一个事件呢
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2899729686
> 多 client 场景中会出现大片 RPC 超时 [E110]Fail to connect Socket ... : Connection timed out [E1008]Reached timeout=2500ms 
这里是多少 client 呢，还有，是否有其他配置变动呢，比如说配置了多少 worker
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r1851691050
##########
src/bthread/parking_lot.h:
##########
@@ -40,7 +41,7 @@ class BAIDU_CACHELINE_ALIGNMENT ParkingLot {
int val;
};
- ParkingLot() : _pending_signal(0) {}
+ ParkingLot() : _pending_signal(0){}
Review Comment:
对的 我改一下
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "wanghenshui (via GitHub)" <gi...@apache.org>.
wanghenshui commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r1841526767
##########
src/bthread/task_control.cpp:
##########
@@ -401,6 +402,17 @@ int TaskControl::_destroy_group(TaskGroup* g) {
bool TaskControl::steal_task(bthread_t* tid, size_t* seed, size_t offset) {
auto tag = tls_task_group->tag();
+ bool except_state = true;
+ // epoll tid should be stolen first.
+ for (auto &epoll_state : _epoll_tid_states[tag]) {
+ if (epoll_state.second.compare_exchange_strong(
Review Comment:
bool except_state = true;是不是应该在这个循环里
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2526709128
> > > 能否在bthread这个层面上，创建协程的时候设置一个属性，使它具有更高的优先级呢？现在这里专门设置了epoll_XXX，感觉有点特化。 另外，总是让epoll的bthread优先调度的话，会不会造成堆积很多任务的情况呢？
> >
> >
> > @yanglimingcn 我觉得可以增加一个属性来表示高优先级，但是要说明一点，就是 epoll 的 任务被 steal 的前提肯定是有 worker 需要 steal from task_control，所以可证明 worker 自己的 queue 应该没有任务，证明他是空闲的，这个时候应该优先让其响应网络事件，这有两个好处
> >
> > 1. 保证网络任务能够及时响应。
> > 2. 可以让所有的 worker 的 run_q 相对均匀，因为 q 中大量任务来源不是 steal，而是 deal event。
> > 另一个问题，就是是否会造成堆积：
> > 如果我们不响应 epoll 就会造成堆积，证明此刻的网络请求数量是大于 brpc 的处理能力的，否则我们不需要通过增加 steal 的次数这种比起 pop queue 更费时间的操作来加快某时刻处理存量 bthread 的能力。这是牺牲了 RT 带来的，感觉不合理。而且 QPS 高时候更容易造成 RT 抖动。
>
> ```
> // Find next task to run, if none, switch to idle thread of the group.
> ```
>
> #ifndef BTHREAD_FAIR_WSQ const bool popped = g->_rq.pop(&next_tid); #else const bool popped = g->_rq.steal(&next_tid); #endif
>
> 我看这块的代码是把任务的调度反转了，能达到先执行epoll的目的？
这里我应该没修改过吧，我是把 epoll 的 tid 单独提取出来了，优先 steal 这个 tid
> > > 能否在bthread这个层面上，创建协程的时候设置一个属性，使它具有更高的优先级呢？现在这里专门设置了epoll_XXX，感觉有点特化。 另外，总是让epoll的bthread优先调度的话，会不会造成堆积很多任务的情况呢？
> >
> >
> > @yanglimingcn 我觉得可以增加一个属性来表示高优先级，但是要说明一点，就是 epoll 的 任务被 steal 的前提肯定是有 worker 需要 steal from task_control，所以可证明 worker 自己的 queue 应该没有任务，证明他是空闲的，这个时候应该优先让其响应网络事件，这有两个好处
> >
> > 1. 保证网络任务能够及时响应。
> > 2. 可以让所有的 worker 的 run_q 相对均匀，因为 q 中大量任务来源不是 steal，而是 deal event。
> > 另一个问题，就是是否会造成堆积：
> > 如果我们不响应 epoll 就会造成堆积，证明此刻的网络请求数量是大于 brpc 的处理能力的，否则我们不需要通过增加 steal 的次数这种比起 pop queue 更费时间的操作来加快某时刻处理存量 bthread 的能力。这是牺牲了 RT 带来的，感觉不合理。而且 QPS 高时候更容易造成 RT 抖动。
>
> ```
> // Find next task to run, if none, switch to idle thread of the group.
> ```
>
> #ifndef BTHREAD_FAIR_WSQ const bool popped = g->_rq.pop(&next_tid); #else const bool popped = g->_rq.steal(&next_tid); #endif
>
> 我看这块的代码是把任务的调度反转了，能达到先执行epoll的目的？
我应该没有修改过这个，我是把 epoll 的 tid 提取出来了，然后优先 steal 这个，来达到优先执行 epoll 的目的
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r1875217593
##########
src/brpc/event_dispatcher.cpp:
##########
@@ -55,6 +55,7 @@ void InitializeGlobalDispatchers() {
FLAGS_usercode_in_pthread ? BTHREAD_ATTR_PTHREAD : BTHREAD_ATTR_NORMAL;
attr.tag = (BTHREAD_TAG_DEFAULT + i) % FLAGS_task_group_ntags;
CHECK_EQ(0, g_edisp[i * FLAGS_event_dispatcher_num + j].Start(&attr));
+ bthread_epoll_tid_set(i, g_edisp[i].Tid());
Review Comment:
是的，我本地二次修改的时候没提交上来
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2490950097
> > 能否在bthread这个层面上，创建协程的时候设置一个属性，使它具有更高的优先级呢？现在这里专门设置了epoll_XXX，感觉有点特化。 另外，总是让epoll的bthread优先调度的话，会不会造成堆积很多任务的情况呢？
>
> @yanglimingcn 我觉得可以增加一个属性来表示高优先级，但是要说明一点，就是 epoll 的 任务被 steal 的前提肯定是有 worker 需要 steal from task_control，所以可证明 worker 自己的 queue 应该没有任务，证明他是空闲的，这个时候应该优先让其响应网络事件，这有两个好处
>
> 1. 保证网络任务能够及时响应。
> 2. 可以让所有的 worker 的 run_q 相对均匀，因为 q 中大量任务来源不是 steal，而是 deal event。
> 另一个问题，就是是否会造成堆积：
> 如果我们不响应 epoll 就会造成堆积，证明此刻的网络请求数量是大于 brpc 的处理能力的，否则我们不需要通过增加 steal 的次数这种比起 pop queue 更费时间的操作来加快某时刻处理存量 bthread 的能力。这是牺牲了 RT 带来的，感觉不合理。而且 QPS 高时候更容易造成 RT 抖动。
// Find next task to run, if none, switch to idle thread of the group.
#ifndef BTHREAD_FAIR_WSQ
const bool popped = g->_rq.pop(&next_tid);
#else
const bool popped = g->_rq.steal(&next_tid);
#endif
我看这块的代码是把任务的调度反转了，能达到先执行epoll的目的？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2489944238
@zhengJade 延时性能的提升主要是因为epoll bthread优先调度了吗？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2490582269
> @zhengJade 延时性能的提升主要是因为epoll bthread优先调度了吗？
@chenBright
从理论上看应该有两方面原因
1. epoll 响应更及时，导致整个 brpc 的处理优先级逻辑发生了变化，以前是所有的 worker 尽量先处理完所有的任务，才能保证有新的 event 进来，当然，没处理完也有会，这取决于是否 steal 到了 epoll 任务，这带来的变化就是，所有空闲的 worker 会立刻拿到 epoll，然后填充自己的 queue，这样可以保证，每个 worker 的 queue 都有一定量的任务，减少 steal 的频率。
2. 任务分布均匀后，wait 的频率降低，可以降低 signal 的频率，只有有任务等待在通知。减少系统调用。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2490597540
> > 能否在bthread这个层面上，创建协程的时候设置一个属性，使它具有更高的优先级呢？现在这里专门设置了epoll_XXX，感觉有点特化。 另外，总是让epoll的bthread优先调度的话，会不会造成堆积很多任务的情况呢？
>
> 1 你说得对，是有点特化，这么改简单点
>
> 如果是带优先级的bthread 可能就得改调度了，涉及到任务抢占/饥饿问题，可能麻烦一点
>
> 2 你说的也对可能会堆积，但是epoll任务是比较少的
>
> 如果改了优先级，会存在你说的优先级高的任务太多导致其他任务饿死
@wanghenshui
是的，当前的改动是特化的，因为目前我只分析出了 epoll 这种任务同优先级带来的问题，所以就只修改了 epoll。
我们是可以增加一个 priority queue，这样可以对使用 bthread 的人开发增加更多的可能性，但是还需要完善，像你说的，随意使用 priority queue 可能会带来饥饿问题，在这个策略没想好之前，可以通过限制 queue 的大小，来一定程度保证不会饥饿，或者不开放这个接口，是 private 的，只作为内部使用。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r2041844626
##########
src/bthread/task_control.cpp:
##########
@@ -430,6 +435,11 @@ int TaskControl::_destroy_group(TaskGroup* g) {
bool TaskControl::steal_task(bthread_t* tid, size_t* seed, size_t offset) {
auto tag = tls_task_group->tag();
+
+ if (_priority_qs[tag].steal(tid)) {
Review Comment:
不是说线程安全的问题，我的意思是这个可能影响性能。我不确定，因为实际场景有很多，可能在你的场景里这个性能不是问题，但是在某些场景里可能会有问题，比如event_dispatcher_num很大，有很多epoll thread的情况？
##########
src/bthread/task_group.h:
##########
@@ -200,6 +200,8 @@ class TaskGroup {
return total_ns;
}
+ const bthread_attr_t& get_current_attr() { return _cur_meta->attr; }
Review Comment:
可以在func里用g->_cur_meta->attr
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2742463997
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r1995120152
##########
src/bthread/task_group.h:
##########
@@ -199,6 +200,10 @@ class TaskGroup {
total_ns += butil::cputhread_time_ns() - _last_cpu_clock_ns;
return total_ns;
}
+ // Thread Unsafe
+ void add_epoll_tid(bthread_t tid) { _epoll_tids.emplace(tid); }
Review Comment:
说的对，我改好了
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2526735379
最近新春，公司太忙了，没时间搞这个 PR (><)，等我这边喘口气，改成优先队列，然后多客户端再测试一次
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "legionxiong (via GitHub)" <gi...@apache.org>.
legionxiong commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2833372438
多 client 场景中会出现大片 RPC 超时
[E110]Fail to connect Socket ... : Connection timed out
[E1008]Reached timeout=2500ms
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r1995775619
##########
src/bthread/task_control.h:
##########
@@ -96,6 +97,12 @@ friend bthread_t init_for_pthread_stack_trace();
void stack_trace(std::ostream& os, bthread_t tid);
std::string stack_trace(bthread_t tid);
#endif // BRPC_BTHREAD_TRACER
+ // Only deal once when init epoll bthread.
+ void set_group_epoll_tid(bthread_tag_t tag, bthread_t tid);
+
+ void push_priority_q(bthread_tag_t tag, bthread_t tid) {
+ _priority_qs[tag].push(tid);
Review Comment:
1. _priority_qs扩容取决于tag吧？现在tag数量在TaskControl::init就确定了，是不是在TaskControl::init中_priority_qs.resize(FLAGS_task_group_ntags)，就安全了？
2. 需要像bthread_setconcurrency_by_tag一样，校验tag，保证不越界吗？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r1998048965
##########
src/bthread/task_control.h:
##########
@@ -96,6 +97,12 @@ friend bthread_t init_for_pthread_stack_trace();
void stack_trace(std::ostream& os, bthread_t tid);
std::string stack_trace(bthread_t tid);
#endif // BRPC_BTHREAD_TRACER
+ // Only deal once when init epoll bthread.
+ void set_group_epoll_tid(bthread_tag_t tag, bthread_t tid);
+
+ void push_priority_q(bthread_tag_t tag, bthread_t tid) {
+ _priority_qs[tag].push(tid);
Review Comment:
> 这一块需要修改的话，可以rebase一下。#2916 修复了一个导致UT经常失败的bug。
这里我 reserve 一下，应该是不用校验的，我看过链路了，在这个之前其他地方应该都校验过很多次了
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2775892073
> > LGTM
> > @wwbmmm @yanglimingcn 有时间再看看。
>
> 这个还有希望合进去嘛
@wwbmmm @yanglimingcn ping
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2849354669
> 我问一下，如果把epoll变成bthread_start_urgent和按照优先级队列处理epoll，最终执行的逻辑上看有什么区别呢？
@yanglimingcn ”把epoll变成bthread_start_urgent“指的是哪部分？现在epoll bthread处理都是bthread_start_urgent处理读事件的吧。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r1909938624
##########
src/bthread/parking_lot.h:
##########
@@ -30,6 +30,7 @@ namespace bthread {
// Park idle workers.
class BAIDU_CACHELINE_ALIGNMENT ParkingLot {
public:
+ static butil::atomic<int> _waiting_count;
Review Comment:
这里减少的是都繁忙情况下不要 signal，而不是说能够准确的 signal 阻塞的 pl，在比较繁忙的情况是实际上是不存在部分 wait，部分 wake 的，而且 sigal 的时候有返回值的，如果返回失败会接着 signal 另一个 pl
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2581959165
> 优化前后长尾延迟没看到明显的变化？
长尾是指？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhoukangsheng (via GitHub)" <gi...@apache.org>.
zhoukangsheng commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2696028124
这个PR啥时候能merge上去啊
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2703646545
> PR #2907 单独实现了第二个优化，看起来更优雅一些。 第一个优化可以再讨论一下 @chenBright
@zhengJade #2907 已经合了，可以rebase master更新一下这个PR吗？
可以再测一下，观察#2897 加的指标，看看epoll调度是否变快了吗？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r1849423762
##########
src/bthread/parking_lot.h:
##########
@@ -40,7 +41,7 @@ class BAIDU_CACHELINE_ALIGNMENT ParkingLot {
int val;
};
- ParkingLot() : _pending_signal(0) {}
+ ParkingLot() : _pending_signal(0){}
Review Comment:
这里的空格不需要删吧
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on code in PR #2819:
URL: https://github.com/apache/brpc/pull/2819#discussion_r1847934237
##########
src/bthread/task_control.cpp:
##########
@@ -401,6 +402,17 @@ int TaskControl::_destroy_group(TaskGroup* g) {
bool TaskControl::steal_task(bthread_t* tid, size_t* seed, size_t offset) {
auto tag = tls_task_group->tag();
+ bool except_state = true;
+ // epoll tid should be stolen first.
+ for (auto &epoll_state : _epoll_tid_states[tag]) {
+ if (epoll_state.second.compare_exchange_strong(
Review Comment:
是的，之前因为考虑只有一个 tag，设置在外面了，我修改一下
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2490017008
> > 能否在bthread这个层面上，创建协程的时候设置一个属性，使它具有更高的优先级呢？现在这里专门设置了epoll_XXX，感觉有点特化。 另外，总是让epoll的bthread优先调度的话，会不会造成堆积很多任务的情况呢？
>
> 1 你说得对，是有点特化，这么改简单点
>
> 如果是带优先级的bthread 可能就得改调度了，涉及到任务抢占/饥饿问题，可能麻烦一点
这样的特化，可能在一些其它用户哪里有些不适用，要尽量保持和原来的兼容性。
>
> 2 你说的也对可能会堆积，但是epoll任务是比较少的
>
> 如果改了优先级，会存在你说的优先级高的任务太多导致其他任务饿死
epoll任务的多少，是由客户端那边决定的，这个不太好预期。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] epoll bthread deal first (brpc)
Posted by "zhengJade (via GitHub)" <gi...@apache.org>.
zhengJade commented on PR #2819:
URL: https://github.com/apache/brpc/pull/2819#issuecomment-2490558740
> > 总是让epoll的bthread优先调度的话，会不会造成堆积很多任务的情况呢？
>
> 现在只有一个连接，epoll bthread调度不是很频繁。是不是可以起多一些client和epoll bthread再测一下呢？
好的，我再测试一下多 client 的效果
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org