chore: Optimize rego policy input allocations #6135
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Manually convert to ast.Value instead of using generic json.Marshal conversion.
The optimized input is always compared to the normal json marshal parser.
johnstcn
approved these changes
Feb 9, 2023
| @@ -79,6 +80,8 @@ var ( | |||
| Site: permissions(map[string][]Action{ | |||
| ResourceWildcard.Type: {WildcardSymbol}, | |||
| }), | |||
| Org: map[string][]Permission{}, | |||
There was a problem hiding this comment.
review: these had been breaking comparison due to comparing empty vs nil slice
Comment on lines
+15
to
+18
| // Currently ast.Object.insert() is the slowest part of the process and allocates | ||
| // the most amount of bytes. This general approach copies all of our struct | ||
| // data and uses a lot of extra memory for handling things like sort order. | ||
| // A possible large improvement would be to implement the ast.Value interface directly. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Manually convert to ast.Value instead of using generic json.Marshal conversion.
Saves ~0.1ms from all users with roles + groups.
Tests
The test
TestRegoInputValueensures there is no difference in the optimized output to the prior json.Marshal method. So this PR is 100% safe! 🥳Results
Input allocations
This is the benchmark of just the saved allocations on the inputs.
JSONRegoValuewas the previous method.ManualRegoValueis new technique.33% reduction in the number of bytes allocated for a rather complex rbac subject. Actual savings depends on the actor and things like the number of groups they are in.
Broader RBAC benchmark impact
Time savings are measurable! Not an order of magnitude, but it is faster.
Before
After
Future work
If we implement
ast.Valueinterface directly for our maps (eg roles), we can reduce a lot more allocations.