perf(acl) rewrite for performance, jit and caching #2736
Conversation
kong/plugins/acl/handler.lua
Outdated
| local block = (reverse.type == WHITE) | ||
| for i = 1, #acls do | ||
| if reverse.groups[acls[i].group] then | ||
| block = (reverse.type == BLACK) |
There was a problem hiding this comment.
Side note: while quite neat, this logic also requires a certain cognitive load on the reader... There could be a simpler (just as efficient way probably) of deciding whether to block or not (and since this is cached anyways, we barely care about a slightly less performance solution).
Not a blocker though imo!
There was a problem hiding this comment.
updated this one, I had the same thought before, just picked one
kong/plugins/acl/handler.lua
Outdated
|
|
||
| else | ||
| -- allowed, create the header | ||
| local str_acls = {} |
There was a problem hiding this comment.
perf: we can pre-allocate the slots in this table with new_tab(#acls, 0)
And instead of the double O(n) #acls, do:
local n = #acls
local str_acls = new_tab(n, 0)
for i = 1, n do
-- ..
endThere was a problem hiding this comment.
updated, though imo the local n = #acls is a bit over the top, considering the same cognitive load argument. But hey, this was a performance rewrite 😄
There was a problem hiding this comment.
We do this quite commonly across (newer parts of) the code base (or any LuaJIT written for OpenResty components)
kong/plugins/acl/handler.lua
Outdated
| end | ||
|
|
||
| if block then | ||
| if cached_result == true then -- NOTE: we only catch the boolean here! |
There was a problem hiding this comment.
Could we rename cached_result to cached_block to preserve the semantics around that value through the code?
kong/plugins/acl/handler.lua
Outdated
| block = true | ||
| break | ||
| end | ||
| end | ||
| else |
There was a problem hiding this comment.
style: missing an empty line above this else
thibaultcha
added
pr/ready (but hold merge)
Tieske
force-pushed
the
perf/acl-plugin
branch
2 times, most recently
from
39df3c1 to
808f0e0
Compare
Tieske
force-pushed
the
perf/acl-plugin
branch
from
808f0e0 to
d614787
Compare
Tieske
force-pushed
the
perf/acl-plugin
branch
from
d614787 to
6753c4e
Compare
Rewritten logic.
I put some serious lightning into its flux capacitor, it will now take you back to the future any time.
(not to mention we dropped the awesome
utils.table_containsusage from this one)