-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
epoll bthread deal first #2819
base: master
Are you sure you want to change the base?
epoll bthread deal first #2819
Conversation
src/bthread/task_control.cpp
Outdated
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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bool except_state = true;是不是应该在这个循环里
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
是的,之前因为考虑只有一个 tag,设置在外面了,我修改一下
能否在bthread这个层面上,创建协程的时候设置一个属性,使它具有更高的优先级呢?现在这里专门设置了epoll_XXX,感觉有点特化。 |
+1。
这样是不是更好呢? |
现在只有一个连接,epoll bthread调度不是很频繁。是不是可以起多一些client和epoll bthread再测一下呢? |
1 你说得对,是有点特化,这么改简单点 如果是带优先级的bthread 可能就得改调度了,涉及到任务抢占/饥饿问题,可能麻烦一点 2 你说的也对可能会堆积,但是epoll任务是比较少的 如果改了优先级,会存在你说的优先级高的任务太多导致其他任务饿死 |
@zhengJade 延时性能的提升主要是因为epoll bthread优先调度了吗? |
这样的特化,可能在一些其它用户哪里有些不适用,要尽量保持和原来的兼容性。
epoll任务的多少,是由客户端那边决定的,这个不太好预期。 |
|
@yanglimingcn 我觉得可以增加一个属性来表示高优先级,但是要说明一点,就是 epoll 的 任务被 steal 的前提肯定是有 worker 需要 steal from task_control,所以可证明 worker 自己的 queue 应该没有任务,证明他是空闲的,这个时候应该优先让其响应网络事件,这有两个好处
|
好的,我再测试一下多 client 的效果 |
@chenBright
|
@wanghenshui |
#ifndef BTHREAD_FAIR_WSQ 我看这块的代码是把任务的调度反转了,能达到先执行epoll的目的? |
src/bthread/task_control.cpp
Outdated
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); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里没必要重复写一遍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吗?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里我当时写的时候其实不能够确定这一步是否一定要求 _concurrency 值的准确性,即使数量有误差,后面再判断的时候也能 add worker 出来,但是考虑到原来的使用的是 memory_order_acquire,就延用了
@@ -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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里应该是 g_edisp[i * FLAGS_event_dispatcher_num + j] 吧?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
是的,我本地二次修改的时候没提交上来
能否分别测一下这两个优化各自提升多少性能呢? |
这里我应该没修改过吧,我是把 epoll 的 tid 单独提取出来了,优先 steal 这个 tid
我应该没有修改过这个,我是把 epoll 的 tid 提取出来了,然后优先 steal 这个,来达到优先执行 epoll 的目的 |
较少 signal 这个我单独优化过,基本没什么提升,然后分析了原因才发现是因为压力大的时候,Q 其实是不平均的,直接的一个原因就是 epoll 的 tid 有时候会很难被 steal 到,这导致所有的 worker 呈现出,要做完当前所有的任务才可以进行 epoll,在某一瞬间导致有些 worker 进入了 wait 状态,然后整体就看起来,worker 没有 wait 状态的时间很少很少,所以就是没啥作用,于是才想着更改 epoll 的优先级 |
最近新春,公司太忙了,没时间搞这个 PR (><),等我这边喘口气,改成优先队列,然后多客户端再测试一次 |
如果网络事件非常频繁的话,是不是可以直接让epoll thread不切走呢?比如把这里: https://github.com/apache/brpc/blob/master/src/brpc/socket.cpp#L2265 |
优化前后长尾延迟没看到明显的变化? |
@@ -30,6 +30,7 @@ namespace bthread { | |||
// Park idle workers. | |||
class BAIDU_CACHELINE_ALIGNMENT ParkingLot { | |||
public: | |||
static butil::atomic<int> _waiting_count; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里 _waiting_count
作为全局静态变量感觉不能有效减少 signal 的次数,因为即使在 _waiting_count
!= 0 的场景,仍然存在parking_lot list中有的pl存在waiting thread,有的pl不存在。在 signal_task 中是先随机选择一个pl来signal,选择的 pl 不一定存在 waiting thread。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里减少的是都繁忙情况下不要 signal,而不是说能够准确的 signal 阻塞的 pl,在比较繁忙的情况是实际上是不存在部分 wait,部分 wake 的,而且 sigal 的时候有返回值的,如果返回失败会接着 signal 另一个 pl
6e0b330
to
7ca337d
Compare
如果不切走,那非 epoll worker 是不是只能通过 steal 的方式来执行任务了?,队列更不均匀 |
长尾是指? |
@chenBright 这里我还是把队列加入到 control 里面了,没有采用之前的 attr + priority queue 的原因是因为只要是在 task_group 里面做优先级,很大概率会使的 epoll 任务始终被困在一个 worker 上,但是目前的问题不只是因为 epoll 没有被先做,而是因为 epoll 产生的任务无法很好的分在 worker queue 里面,最好是把 epoll 交给 control 管理,让下面的 worker 均匀的偷到。 |
What problem does this PR solve?
对 brpc 性能做了提升
Problem Summary:
目前的 brpc 网络事件存在两个问题
What is changed and the side effects?
epoll 线程不在 queue 中,独立出来
Side effects:
Performance effects(性能影响): 性能平均提升 20%-25%
Breaking backward compatibility(向后兼容性): 兼容
压测结果
优化前
优化后
1w
压测补充
@chenBright 这里补充了多客户端压测
多客户端压测,使用了 5 个客户端,各发 1w
优化前
5 个采集周期平均值
(370 + 401 + 406 + 392 + 365)/ 5 = 382.2 us
优化后
5 个采集周期平均值
(360 + 324 + 301 + 328 + 319)/ 5 = 326.4
(382.2 - 326.4) / 382.2 = 14.6%
大约有 14.6% 的一个提升
Check List: