Restruct event_dispatcher source file #1888

guodongxiaren · 2022-08-14T13:32:11Z

event_dispatcher.cpp 同时兼容Linux和Mac系统，大量使用了条件编译的宏，这种条件编译的代码占了这个文件的大部分。

导致这个代码文件可读性比较差，另外后续如果扩充其他操作系统、其他事件驱动的API，则event_dispatcher可维护性也比较差。
参考Redis中事件驱动库Ae的源码组织形式：

#ifdef HAVE_EVPORT
#include "ae_evport.c"
#else
    #ifdef HAVE_EPOLL
    #include "ae_epoll.c"
    #else
        #ifdef HAVE_KQUEUE
        #include "ae_kqueue.c"
        #else
        #include "ae_select.c"
        #endif
    #endif
#endif

使用include 源文件的方式来隔离不同的操作系统的实现差异，单个文件可读性大大提升，也方便后续扩充其他系统的支持（只需要添加一个新的文件即可）

新增的两个文件之所以后缀名之所以改成了cxx是因为当前的编译脚本会自动对cpp文件编译成 .o ，但是新增的两个文件是无法单独编译的，它们其实是会被选择性地加入到event_dispatcher.cpp中一起编译。自己不能编译。所以改成了编译脚本当前不会自动独立编译的后缀cxx，并且cxx也是一种常用的C++源文件后缀。

zyearn · 2022-08-14T19:56:51Z

Bazel的编译挂了：

[1,115 / 1,457] 11 actions, 2 running
    Compiling src/brpc/esp_message.cpp; 1s linux-sandbox
    Compiling src/brpc/details/usercode_backup_pool.cpp; 0s linux-sandbox
    [Sched] Compiling src/brpc/uri.cpp; 10s
    [Sched] Compiling src/brpc/details/rtmp_utils.cpp; 9s
    [Sched] Compiling src/brpc/details/tcmalloc_extension.cpp; 8s
    [Sched] Compiling src/brpc/details/ssl_helper.cpp; 8s
    [Sched] Compiling src/brpc/ts.cpp; 7s
    [Sched] Compiling src/brpc/details/mesalink_ssl_helper.cpp; 6s ...
src/brpc/event_dispatcher.cpp:68:14: fatal error: 'brpc/event_dispatcher_epoll.cxx' file not found
    #include "brpc/event_dispatcher_epoll.cxx"

wwbmmm · 2022-08-15T02:15:57Z

新增的两个文件之所以后缀名之所以改成了cxx是因为当前的编译脚本会自动对cpp文件编译成 .o ，但是新增的两个文件是无法单独编译的，它们其实是会被选择性地加入到event_dispatcher.cpp中一起编译。自己不能编译。所以改成了编译脚本当前不会自动独立编译的后缀cxx，并且cxx也是一种常用的C++源文件后缀。

可以搞两个cpp文件，然后cpp文件里面用条件编译

guodongxiaren · 2022-08-15T17:14:26Z

Bazel的编译挂了：


[1,115 / 1,457] 11 actions, 2 running

    Compiling src/brpc/esp_message.cpp; 1s linux-sandbox

    Compiling src/brpc/details/usercode_backup_pool.cpp; 0s linux-sandbox

    [Sched] Compiling src/brpc/uri.cpp; 10s

    [Sched] Compiling src/brpc/details/rtmp_utils.cpp; 9s

    [Sched] Compiling src/brpc/details/tcmalloc_extension.cpp; 8s

    [Sched] Compiling src/brpc/details/ssl_helper.cpp; 8s

    [Sched] Compiling src/brpc/ts.cpp; 7s

    [Sched] Compiling src/brpc/details/mesalink_ssl_helper.cpp; 6s ...

src/brpc/event_dispatcher.cpp:68:14: fatal error: 'brpc/event_dispatcher_epoll.cxx' file not found

    #include "brpc/event_dispatcher_epoll.cxx"

Fix了 @zyearn

* Update Makefile * Update CMakeLists.txt * Update BUILD.bazel

wasphin · 2022-08-16T14:22:39Z

编译脚本里也可以根据环境选择不同的源文件或移除不需要编译的源文件，或者简单点像伟冰说的加个大宏去控制。

guodongxiaren · 2022-08-16T14:24:48Z

编译脚本里也可以根据环境选择不同的源文件或移除不需要编译的源文件，或者简单点像伟冰说的加个大宏去控制。

嗯，最新代码已经改成修改编译脚本了。源文件后缀还用cpp。

src/brpc/event_dispatcher_epoll.cpp

src/brpc/event_dispatcher_kqueue.cpp

zyearn · 2022-08-16T20:33:17Z

src/brpc/event_dispatcher_kqueue.cpp

+                             *consumer_thread_attr : BTHREAD_ATTR_NORMAL);
+
+    //_consumer_thread_attr is used in StartInputEvent(), assign flag NEVER_QUIT to it will cause new bthread
+    // that created by epoll_wait() never to quit.


这里的注释也要对应改一下

epoll_wait 改成 kevent？

应该是的

zyearn · 2022-08-16T20:33:29Z

src/brpc/event_dispatcher_kqueue.cpp

+
+    //_consumer_thread_attr is used in StartInputEvent(), assign flag NEVER_QUIT to it will cause new bthread
+    // that created by epoll_wait() never to quit.
+    _epoll_thread_attr = _consumer_thread_attr | BTHREAD_NEVER_QUIT;


_kqueue_thread_attr?

成员变量是头文件中定义的，两个平台相关的cpp文件是共用头文件的。如果要改类成员的名字最好是改成一个更为通用的名字？不然就只能在头文件中这样写了：

#if defined(OS_LINUX) bthread_attr_t _epoll_thread_attr; #elif defined(OS_MACOSX) bthread_attr_t _kqueue_thread_attr; #endif

是不是改成event_multiplexing_thread_attr更通用一些？

发现这个变量只在单个成员函数中调用到，所以我从头文件中删了，改成cpp函数内局部变量了。避免命名问题。

zyearn · 2022-08-16T20:34:02Z

src/brpc/event_dispatcher_kqueue.cpp

+    int rc = bthread_start_background(
+        &_tid, &_epoll_thread_attr, RunThis, this);
+    if (rc) {
+        LOG(FATAL) << "Fail to create epoll/kqueue thread: " << berror(rc);


zyearn · 2022-08-16T20:34:27Z

src/brpc/event_dispatcher_kqueue.cpp

+    // remove the fd from epoll. More badly, the fd will not be removable
+    // from epoll again! If the fd was level-triggered and there's data left,
+    // epoll_wait will keep returning events of the fd continuously, making
+    // program abnormal.


这个注释其实我不太明白。。EventDispatcher内部实现的时候不是用的边缘触么？对EPOLL使用了EPOLLET，对kqueue使用了EV_CLEAR。为什么这里说If the fd was level-triggered，好像两个平台都没有水平触发的情况吧。

我理解这里是虚拟语气

zyearn · 2022-08-16T20:34:41Z

src/brpc/event_dispatcher_kqueue.cpp

+        if (_stop) {
+            // epoll_ctl/epoll_wait should have some sort of memory fencing
+            // guaranteeing that we(after epoll_wait) see _stop set before
+            // epoll_ctl.


zyearn · 2022-08-16T20:36:47Z

这个PR merge以后是否可以再开一个issue记录一下EventDispatcher有些函数名的implementation specific问题，比如目前EventDispatcher::AddEpollOut表明只针对epoll有效。

guodongxiaren · 2022-08-17T05:00:22Z

这个PR merge以后是否可以再开一个issue记录一下EventDispatcher有些函数名的implementation specific问题，比如目前EventDispatcher::AddEpollOut表明只针对epoll有效。

目前是源文件针对不同的平台有不同的实现，但是头文件是通用的。AddEpollOut在Linux上是epoll，mac上是kqueue。后面看看要不要改成更为通用的名字

zyearn · 2022-08-17T19:37:20Z

目前是源文件针对不同的平台有不同的实现，但是头文件是通用的。AddEpollOut在Linux上是epoll，mac上是kqueue。后面看看要不要改成更为通用的名字

#1894

guodongxiaren added 2 commits August 14, 2022 11:56

split event_dispatcher

d00c200

rename suffix

e7addca

guodongxiaren and others added 2 commits August 15, 2022 13:37

Update event_dispatcher.cpp

146a787

fix bazel

3a9d77d

rename cxx to cpp && change build file (#8)

5d5873b

* Update Makefile * Update CMakeLists.txt * Update BUILD.bazel

guodongxiaren and others added 3 commits August 16, 2022 23:39

Update event_dispatcher.cpp

416b412

restore bazel

53ed565

add two source file in hdrs for bazel

b3d2b2d

zyearn reviewed Aug 16, 2022

View reviewed changes

guodongxiaren and others added 2 commits August 17, 2022 22:37

注释修改以及_epoll_thread_attr 移动到cpp文件中 (#9)

b45201d

fix comment

540409c

zyearn merged commit 93038ed into apache:master Aug 17, 2022

guodongxiaren deleted the restruct_event_dispatcher branch August 20, 2022 04:19

Huixxi added the enhancement improvements on existing features label Oct 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restruct event_dispatcher source file #1888

Restruct event_dispatcher source file #1888

guodongxiaren commented Aug 14, 2022 •

edited

Loading

zyearn commented Aug 14, 2022

wwbmmm commented Aug 15, 2022

guodongxiaren commented Aug 15, 2022

wasphin commented Aug 16, 2022

guodongxiaren commented Aug 16, 2022

zyearn Aug 16, 2022

guodongxiaren Aug 17, 2022

zyearn Aug 17, 2022

zyearn Aug 16, 2022

guodongxiaren Aug 17, 2022

zyearn Aug 17, 2022

guodongxiaren Aug 17, 2022

zyearn Aug 16, 2022

zyearn Aug 16, 2022

guodongxiaren Aug 17, 2022

zyearn Aug 17, 2022

zyearn Aug 16, 2022

guodongxiaren Aug 17, 2022

zyearn commented Aug 16, 2022

guodongxiaren commented Aug 17, 2022

zyearn commented Aug 17, 2022 •

edited

Loading

Restruct event_dispatcher source file #1888

Restruct event_dispatcher source file #1888

Conversation

guodongxiaren commented Aug 14, 2022 • edited Loading

zyearn commented Aug 14, 2022

wwbmmm commented Aug 15, 2022

guodongxiaren commented Aug 15, 2022

wasphin commented Aug 16, 2022

guodongxiaren commented Aug 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyearn commented Aug 16, 2022

guodongxiaren commented Aug 17, 2022

zyearn commented Aug 17, 2022 • edited Loading

guodongxiaren commented Aug 14, 2022 •

edited

Loading

zyearn commented Aug 17, 2022 •

edited

Loading