chore: Optimize parial rego execution byte allocations #6144
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
Identify time + alloc cost before optimizing
Emyrk
commented
Feb 9, 2023
This reverts commit 0ef4b5c. This addition made a unit test fail with a supporting statemetn
johnstcn
reviewed
Feb 10, 2023
| @@ -166,6 +168,21 @@ func NewAuthorizer(registry prometheus.Registerer) *RegoAuthorizer { | |||
| if err != nil { | |||
| panic(xerrors.Errorf("compile rego: %w", err)) | |||
| } | |||
|
|
|||
| partialQuery, err = rego.New( | |||
There was a problem hiding this comment.
review: this is inside queryOnce.Do()
johnstcn
reviewed
Feb 10, 2023
| }), | ||
| rego.Query("data.authz.allow = true"), | ||
| rego.Module("policy.rego", policy), | ||
| ).PrepareForPartial(context.Background()) |
There was a problem hiding this comment.
review: I wondered whether it would make sense to pass through a higher-level context here (e.g. NewAuthorizer(ctx, registry)) but the place we call NewAuthorizer doesn't appear to have any application-level context already available.
johnstcn
approved these changes
Feb 10, 2023
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.
Preventing some allocations by reusing the same prepared partial query. Unfortunately the bulk of the work happens when the inputs are passed in, which cannot be done in advance.
The unit tests that are most affected by this change are
Ownerrole users. Since the partial execution results in a single empty query. Meaning most of the memory allocations happens before the input is passed in.Before
After
EMPTY POLICY
This is an illustration if
inputhas 0 impact on the output prepared queries. I did this to illustrate the speedup of thePartial()call on the rego policy.You can see the difference if you use an empty
policy.regoso that theinputsdo not really affect the output. This shows that the impact ofinputsis a magnitude larger than the compilation of the original policy.Before
After