Skip to content
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

Added timer wheel/heap/rbtree module #191

Closed
wants to merge 1 commit into from

Conversation

chaizhenhua
Copy link

Hi, all

I have written a timer module for nginx/tengine. Perhaps you would consider adding it to tengine.
The orignal module is here.
I've done some simple tests, you can do more tests before release.
Hope this helps.


ngx_int_t (*init)(ngx_cycle_t *cycle);
void (*done)(ngx_cycle_t *cycle);
} ngx_timer_actions_t;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

请问
ngx_timer_actions_t
因为什么原因要独立出来?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi, cfsego
这是为了与原有Nginx代码保持一致, 同时可以把timer的子模块独立出来

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我明白了。参见event module

@zhuzhaoyuan
Copy link
Member

Hi 振华!

好久没联系了,多谢你的pull request :)
timer的这个改动我们之前就有个这个计划,你是否可以review一下我们这个之前的pull request:#53
然后大家一起综合一下?

#include <ngx_event_timer.h>

#define NGX_TIMER_MODULE 0x454d4954 /* "TIME" */
#define NGX_TIMER_CONF NGX_MAIN_CONF|NGX_DIRECT_CONF
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我确实是对NGX_TIMER_CONF的值定义存有异议。
毕竟ngx core module 中的不少指令就是直接定义的NGX_MAIN_CONF|NGX_DIRECT_CONF|.....
另外NGX_TIMER_CONF的存在感非常稀薄,因为你是将所有timer模块的指令统统copy到你的core模块的commands里面,而且conf的位置也是由你来计算的,所以NGX_MAIN_CONF|NGX_DIRECT_CONF也没有什么实际意义。

另一个想法,能否将将配置搞成

evnets {
     use epoll;

     timer {
         use time_wheel;
        .....
   }
}

这样的格式。因为timer应该算是events的一部分。另外,拷贝commands的方式很奇怪了,按block处理是不是更好?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我也是这样认为的, 现在的做法, 代码上看确实不够优雅,
不过 timer_resolution 是nginx core module 配置的. 最后的配置就会变成下面这样子, 相关的配置不在同一层级, 在配置的角度来看就很不方便了.

timer_resolution 100ms;

events {
      timer {
           time_wheel_max 2m;
           use time_wheel;
     }
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NGX_TIMER_CONF 这个宏确实有必要改一下. 配置上的逻辑还需要再斟酌.

@chaizhenhua
Copy link
Author

好啊!已经叁考#53和review,加入了heap4并简化了conf部分的代码。

在 2013-3-7,15:30,Joshua Zhu [email protected] 写道:

Hi 振华!

好久没联系了,多谢你的pull request :)
timer的这个改动我们之前就有个这个计划,你是否可以review一下我们这个之前的pull request:#53
然后大家一起综合一下?


Reply to this email directly or view it on GitHub.

@yaoweibin
Copy link
Member

赞,神速啊。 @dinic看下有没有问题。

|| ngx_strncmp(old_tcf->use->name.data,
value[1].data, value[1].len)) {
ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
"when the server runs without a master process "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

从哪里判断没有master进程的?cf->cycle->old_cycle->conf_ctx 有值只说明这个master进程是reload的时候了吧?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个是个bug, 等下处理.

@ghost ghost assigned dinic Mar 25, 2013
@zhuzhaoyuan
Copy link
Member

可以参考这里再调整一下风格:
http://tengine.taobao.org/book/appendix_a.html#nginx-100

@chaizhenhua
Copy link
Author

@zhuzhaoyuan 已经做了风格调整,我在想是不是需要个工具专门做这件事。人工调风格感觉有些麻烦,我之前一直用astyle,可惜这个工具没有那么多细节微调的选项。

@cfsego
Copy link
Member

cfsego commented Jul 30, 2014

不合并,关闭

@cfsego cfsego closed this Jul 30, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants