used table functions of LuaJIT for better performance. #3673
Conversation
k8s-ci-robot
added
cncf-cla: yes
| @@ -46,15 +49,14 @@ local function flush(premature) | |||
| return | |||
| end | |||
|
|
|||
| local current_metrics_batch = metrics_batch | |||
There was a problem hiding this comment.
You are changing the behaviour here. The idea was to take current batch as soon as timer calls flush function and reset the main queue so that it can accept more metrics and continue with encoding and sending the current metrics batch while the other one is being filled.
With your change we will potentially lose more metrics. Imagine timer (flush function) kicks in right before the time when the current batch is full. It'll start encoding the metrics and during this time every new metric will be dropped. Given cjson is quite fast at encoding there'll probably not many metrics being dropped but if we can avoid that why not.
There was a problem hiding this comment.
got it. I will fix that.
There was a problem hiding this comment.
@ElvinEfendi fixed.
used table.clone to clone table: https://github.com/openresty/luajit2/blob/v2.1-agentzh/README#L65
| ngx.log(ngx.WARN, "omitting metrics for the request, current batch is full") | ||
| return | ||
| end | ||
|
|
||
| table.insert(metrics_batch, metrics()) | ||
| metrics_batch[metrics_size + 1] = metrics() |
There was a problem hiding this comment.
Does this custom table implementation not have insert?
There was a problem hiding this comment.
it has the insert method, but the table.insert is an O(n) operation due to the lj_tab_len call inside LuaJIT.
We can avoid it in hot loops for better performance.
| -- if an Nginx worker processes more than (MAX_BATCH_SIZE/FLUSH_INTERVAL) RPS then it will start dropping metrics | ||
| local MAX_BATCH_SIZE = 10000 | ||
| local FLUSH_INTERVAL = 1 -- second | ||
|
|
||
| local metrics_batch = new_tab(MAX_BATCH_SIZE, 0) |
There was a problem hiding this comment.
What's the second argument for? Why 0? A link to docs would be sufficient too.
There was a problem hiding this comment.
doc: https://github.com/openresty/luajit2/blob/v2.1-agentzh/doc/extensions.html
table.new(narray, nhash), the second argument is for hash.
|
Thanks for the contribution @moonming ! Any reason why not use nkeys as well for counting? /lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ElvinEfendi, moonming The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What this PR does / why we need it:
used
table.newandtable.clearfunctions of LuaJIT for better performance.Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #Special notes for your reviewer: