acl: Avoid allocations when checking implied ops #12079
Conversation
|
|
src/v/security/authorizer.h
Outdated
|
|
||
| switch (operation) { | ||
| case acl_operation::describe: { | ||
| std::array ops = { |
There was a problem hiding this comment.
i suppose it may get inline anyway?
There was a problem hiding this comment.
That's what I assumed yes.
Codegen is actually quite interesting: https://gcc.godbolt.org/z/vfd6Mfasn
Depending on the numbers clang actually generates some bithacks to shortcut. Also codegen can be different dependent on the numbers between using static and not.
However, looking at llvm-mca (you need to comment out the functions you are not interested in or copy to https://uica.uops.info/) it seems the non-static version generates more efficient code. This is obviously not a microbenchmark as it also depends on what we search but it gives a good idea I think.
There was a problem hiding this comment.
A local static constexpr usually seems the best to me, if it's possible: keeps the table local to the scope it is used, doesn't pose any chance of stack allocations a non-static array might, or the "lazy init" that a non-constexpr static might cause (i.e., when you start see guard variable for XYZ like this though there has to be something that doesn't let the compiler do static init in the .data or .rodata sections for that to happen).
H/e oddly here static non-constexpr has different codegen from the other options as Stephan points odd. Pretty odd and inexplicable (which is itself not really odd weird when talking about compiler opts).
There was a problem hiding this comment.
Switched to static constexpr
src/v/security/authorizer.h
Outdated
|
|
||
| switch (operation) { | ||
| case acl_operation::describe: { | ||
| std::array ops = { |
There was a problem hiding this comment.
A local static constexpr usually seems the best to me, if it's possible: keeps the table local to the scope it is used, doesn't pose any chance of stack allocations a non-static array might, or the "lazy init" that a non-constexpr static might cause (i.e., when you start see guard variable for XYZ like this though there has to be something that doesn't let the compiler do static init in the .data or .rodata sections for that to happen).
H/e oddly here static non-constexpr has different codegen from the other options as Stephan points odd. Pretty odd and inexplicable (which is itself not really odd weird when talking about compiler opts).
src/v/security/authorizer.h
Outdated
| @@ -131,6 +124,42 @@ class authorizer final { | |||
| } | |||
|
|
|||
| private: | |||
| /* | |||
| * Compute whether the implied operations are allowed based on the specified | |||
There was a problem hiding this comment.
I think maybe the comment is backwards, or I'm not parsing it correctly?
Isn't it:
Determine whether the given operation is allowed based on the implied allowed operations in acl_matches.
?
There was a problem hiding this comment.
Yes you are right - fixing.
`acl_implied_ops` was quite visible (~3%) in the callchains leading to `operator new()` in the LRC profile. It's called for every partition so this is not entirely surprising when using larger partition counts. Avoid the allocations by "inlining" the check into the function so that no vector has to be allocated.
StephanDollberg
dismissed stale reviews from travisdowns and dotnwat
via
74ea237
StephanDollberg
force-pushed
the
stephan/acl-less-alloc
branch
from
4ce7bb6
to
74ea237
Compare
|
/backport v23.1.x |
|
/backport v22.3.x |
|
Failed to run cherry-pick command. I executed the commands below: |
acl_implied_opswas quite visible (~3%) in the callchains leading tooperator new()in the LRC profile.It's called for every partition so this is not entirely surprising when using larger partition counts.
Avoid the allocations by "inlining" the check into the function so that no vector has to be allocated.
Backports Required
Release Notes