TimerTask improvements #791
Conversation
davidor
force-pushed
the
use-gc-module-in-timer-task
branch
from
69d3c58 to
fd773c0
Compare
| res_mt.__index = _M | ||
| return res_mt | ||
| return { | ||
| __gc = function() _M.unregister_task(id) end, |
There was a problem hiding this comment.
Can't there be generic __gc metamethod ? To avoid allocating extra table for each timer.
There was a problem hiding this comment.
I don't think so because it needs the 'id'.
There was a problem hiding this comment.
But it can get the id from self no ? It will get the table as first argument, so it can take the id from there.
| end | ||
|
|
||
| schedule_next(self) | ||
| local t_diff = ngx_now() - t_start |
There was a problem hiding this comment.
I we are doing this, I'd rather go for monotonic time. It is not that hard to do:
https://github.com/3scale/apicast/blob/ea7966080dce42eb0d92b9fda85c99de8700e2ca/benchmark/template.lua#L14-L19
| local t_diff = ngx_now() - t_start | ||
|
|
||
| local delay_for_next = interval - t_diff | ||
| if delay_for_next < 0 then delay_for_next = 0 end |
There was a problem hiding this comment.
I think LuaJIT recommends using math.max instead of conditions (as they can't be JITed).
http://wiki.luajit.org/Numerical-Computing-Performance-Guide
| schedule_next(self) | ||
| local t_diff = ngx_now() - t_start | ||
|
|
||
| local delay_for_next = interval - t_diff |
There was a problem hiding this comment.
Is it really needed?
I think it is just fine to delay the execution. Documenting it is way easier than doing this. Also it could end up in situation when are two called immediately after each other because they took more than the delay.
There was a problem hiding this comment.
I think from a user perspective is more natural to expect a consistent reporting every X seconds. But there are no guarantees anyway because some reports might take more than the interval specified in the config. I don't think it's too important, we can remove it to simplify this a bit.
There was a problem hiding this comment.
I'd prefer it to be simpler. It can be documented pretty easily.
For the record, ngx.timer.every behaves quite stupidly and will schedule timer every N seconds regardless how long the timer takes.
davidor
force-pushed
the
use-gc-module-in-timer-task
branch
3 times, most recently
from
e027d51 to
be4556f
Compare
|
Comments addressed, some of them no longer apply after removing #791 (comment) |
| _M.unregister_task(self.id) | ||
| end | ||
|
|
||
| local function mt() |
There was a problem hiding this comment.
Now there is no need to allocate a metatable per instance and can just share one, right?
TimerTask is now built calling GC.set_metatable_gc. That method returns an object for which type() returns "userdata". Luassert only allows creating stubs on objects for which type() returns "table".
So instances can be garbage-collected
davidor
force-pushed
the
use-gc-module-in-timer-task
branch
from
be4556f to
998ed26
Compare
This PR implements several improvements in the
TimerTaskmodule:GCmodule introduced in the previous PR Workaround to make __gc work on tables #790self.