perf(response-transformer) accumulate chunks of response using table #2977
Conversation
Concatenate strings in buffer every time body_filter occurs when receiving response from an upstream can lead strong overhead. The right way would be to add chunks to a table, and do a concat when eof occurs. Programming in Lua - 11.6 – String Buffers: https://www.lua.org/pil/11.6.html Discussion in kong mailing list: https://groups.google.com/forum/#!topic/konglayer/vWU5Y7Qsb_s
| @@ -14,7 +14,8 @@ end | |||
|
|
|||
| function ResponseTransformerHandler:access(conf) | |||
| ResponseTransformerHandler.super.access(self) | |||
| ngx.ctx.buffer = "" | |||
| ngx.ctx.chunks = {} | |||
| ngx.ctx.chunk_number = 1 | |||
There was a problem hiding this comment.
though it also applied on the original code, my comment would be to use a better namespaced name for these variables. The ngx.ctx table is shared for the lifetime of the request and hence has a risk of name colissions, especially with very generic names as chunks.
So maybe ngx.ctx.rt_body_chunks ?
There was a problem hiding this comment.
By the way, perhaps, better kind of namespacing is to create a separate table for each plugin.
f.e. ctx.response_transformer = { body_chunks = {}, body_chunk_number = 1 }
But this is breaking change and need to be tested hard if plugins get access to shared data.
| ngx.arg[1] = body | ||
| else | ||
| ngx.ctx.buffer = ngx.ctx.buffer .. chunk | ||
| ngx.ctx.chunks[ngx.ctx.chunk_number] = chunk | ||
| ngx.ctx.chunk_number = ngx.ctx.chunk_number + 1 |
There was a problem hiding this comment.
in this code there are a lot of ngx.ctx accesses. Since the lookup of ctx in ngx is rather expensive (it has some meta table magic), it would be better to cache it in a local.
-- at the top
local ctx = ngx.ctx`and then replace all ngx.ctx.whatever to ctx.whatever
There was a problem hiding this comment.
context cached in local variables in both ('access' and 'body_filter') functions
hishamhm
added
the
pr/changes requested
Yes, ready to push fixes. Which is more acceptable - add a new commit or replace existing one? |
|
@r-alekseev Since this PR is atomic enough so that it will result in a single commit, you can just add new commits and we'll squash-merge them later. |
fixing review comments: cache ngx.ctx in local rename context variables to avoid collisions
* move ctx local closer to usages
Concatenate strings in buffer every time body_filter occurs
when receiving response from an upstream can lead strong overhead.
The right way would be to add chunks to a table, and do a concat
when eof occurs.
Programming in Lua - 11.6 – String Buffers:
https://www.lua.org/pil/11.6.html
Discussion in kong mailing list:
https://groups.google.com/forum/#!topic/konglayer/vWU5Y7Qsb_s