perf(context): skip unused CEL env funcs#2014
Closed
adityathebe wants to merge 1 commit into
Closed
Conversation
RunTemplate constructed every registered CEL env function before evaluating any template, even for simple CEL expressions that did not call those functions. This created avoidable closure and cel.Function allocation churn on cache-hit evaluations. Only append registered CEL env options when the expression calls the corresponding function, while preserving support for legacy registry keys that end in Cel. Add a benchmark for cache-hit CEL evaluation with varying registered function counts.
Contributor
Walkthrough
ChangesSelective CEL env function registration
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Benchstat (Other)Base: 📊 1 minor regression(s) (all within 5% threshold)
✅ 7 improvement(s)
Full benchstat output |
Benchstat (RLS)Base: 📊 2 minor regression(s) (all within 5% threshold)
✅ 7 improvement(s)
Full benchstat output |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bench/template_cel_env_bench_test.go`:
- Around line 41-54: The benchmark test uses b.Fatal for assertions instead of
gomega expectations, which violates the coding guidelines. Replace the b.Fatal
error checks and the b.Fatal boolean check in the benchmark loop with gomega
assertions by first creating a gomega instance with gomega.NewWithT(b), then use
Expect to verify the error is nil and the result ok is true. Specifically,
replace the two b.Fatal calls related to the ctx.RunTemplateBool function calls
with gomega Expect assertions that check for nil errors and that ok evaluates to
true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60adbd86-1fbc-48a3-a163-78893cb2293c
📒 Files selected for processing (3)
bench/template_cel_env_bench_test.gocontext/template.gocontext/template_test.go
Comment on lines
+41
to
+54
| if _, err := ctx.RunTemplateBool(tmpl, env); err != nil { | ||
| b.Fatal(err) | ||
| } | ||
|
|
||
| b.ReportAllocs() | ||
| b.ResetTimer() | ||
| for i := 0; i < b.N; i++ { | ||
| ok, err := ctx.RunTemplateBool(tmpl, env) | ||
| if err != nil { | ||
| b.Fatal(err) | ||
| } | ||
| if !ok { | ||
| b.Fatal("expected expression to evaluate true") | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
# Locate the benchmark test file and check its structure
find . -name "template_cel_env_bench_test.go" -o -name "*_test.go" -path "*/bench/*" 2>/dev/nullRepository: flanksource/duty
Length of output: 323
🏁 Script executed:
# Read the benchmark test file
cat -n bench/template_cel_env_bench_test.goRepository: flanksource/duty
Length of output: 3651
🏁 Script executed:
# Check the imports and lines around 41-54 in detail
sed -n '1,80p' bench/template_cel_env_bench_test.go | cat -nRepository: flanksource/duty
Length of output: 3149
Use gomega assertions in this _test.go benchmark.
This benchmark uses assertion-style b.Fatal checks instead of gomega expectations. Please switch to gomega.NewWithT(b) and Expect(...) for these checks.
Proposed patch
import (
"fmt"
"testing"
"time"
dutyctx "github.com/flanksource/duty/context"
"github.com/flanksource/gomplate/v3"
"github.com/google/cel-go/cel"
celtypes "github.com/google/cel-go/common/types"
"github.com/google/cel-go/common/types/ref"
+ . "github.com/onsi/gomega"
)
@@
for _, registeredFuncs := range []int{0, 18, 64} {
b.Run(fmt.Sprintf("registered=%02d", registeredFuncs), func(b *testing.B) {
+ g := NewWithT(b)
installBenchmarkCelEnvFuncs(b, registeredFuncs)
@@
- if _, err := ctx.RunTemplateBool(tmpl, env); err != nil {
- b.Fatal(err)
- }
+ _, err := ctx.RunTemplateBool(tmpl, env)
+ g.Expect(err).NotTo(HaveOccurred())
@@
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
- ok, err := ctx.RunTemplateBool(tmpl, env)
- if err != nil {
- b.Fatal(err)
- }
- if !ok {
- b.Fatal("expected expression to evaluate true")
- }
+ ok, err := ctx.RunTemplateBool(tmpl, env)
+ g.Expect(err).NotTo(HaveOccurred())
+ g.Expect(ok).To(BeTrue())
}
})
}
}Per coding guidelines, **/*_test.go: "Always use github.com/onsi/gomega package for assertions in test files" and "use the gomega.NewWithT(t) approach for assertions."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if _, err := ctx.RunTemplateBool(tmpl, env); err != nil { | |
| b.Fatal(err) | |
| } | |
| b.ReportAllocs() | |
| b.ResetTimer() | |
| for i := 0; i < b.N; i++ { | |
| ok, err := ctx.RunTemplateBool(tmpl, env) | |
| if err != nil { | |
| b.Fatal(err) | |
| } | |
| if !ok { | |
| b.Fatal("expected expression to evaluate true") | |
| } | |
| _, err := ctx.RunTemplateBool(tmpl, env) | |
| g.Expect(err).NotTo(HaveOccurred()) | |
| b.ReportAllocs() | |
| b.ResetTimer() | |
| for i := 0; i < b.N; i++ { | |
| ok, err := ctx.RunTemplateBool(tmpl, env) | |
| g.Expect(err).NotTo(HaveOccurred()) | |
| g.Expect(ok).To(BeTrue()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bench/template_cel_env_bench_test.go` around lines 41 - 54, The benchmark
test uses b.Fatal for assertions instead of gomega expectations, which violates
the coding guidelines. Replace the b.Fatal error checks and the b.Fatal boolean
check in the benchmark loop with gomega assertions by first creating a gomega
instance with gomega.NewWithT(b), then use Expect to verify the error is nil and
the result ok is true. Specifically, replace the two b.Fatal calls related to
the ctx.RunTemplateBool function calls with gomega Expect assertions that check
for nil errors and that ok evaluates to true.
Source: Coding guidelines
Gavel resultsGavel exited with code . |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
RunTemplate built every registered CEL env function before evaluating templates, even for simple CEL expressions that did not call any registered functions.
This caused avoidable allocation churn on cache-hit CEL evaluations because DB/catalog/gitops function declarations and closures were rebuilt every call.
Only append registered CEL env options when the CEL expression calls the corresponding function. Add a focused benchmark covering cache-hit CEL evaluation with different registered function counts.
Summary by CodeRabbit
Performance
Tests