brpc icon indicating copy to clipboard operation
brpc copied to clipboard

Restruct event_dispatcher source file

Open guodongxiaren opened this issue 2 years ago • 7 comments

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++源文件后缀。

guodongxiaren avatar Aug 14 '22 13:08 guodongxiaren

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"

zyearn avatar Aug 14 '22 19:08 zyearn

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

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

wwbmmm avatar Aug 15 '22 02:08 wwbmmm

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

guodongxiaren avatar Aug 15 '22 17:08 guodongxiaren

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

wasphin avatar Aug 16 '22 14:08 wasphin

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

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

guodongxiaren avatar Aug 16 '22 14:08 guodongxiaren

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

zyearn avatar Aug 16 '22 20:08 zyearn

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

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

guodongxiaren avatar Aug 17 '22 05:08 guodongxiaren

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

https://github.com/apache/incubator-brpc/issues/1894

zyearn avatar Aug 17 '22 19:08 zyearn