perf(cel): build static CEL env once and Extend per call#162
Conversation
|
Warning Review limit reached
More reviews will be available in 51 minutes and 6 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughA shared base CEL environment is now built once via ChangesCEL Static Environment Caching
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
BenchstatBase: 2 minor regression(s) (all within 5% threshold)
6 improvement(s)
Full benchstat output |
adityathebe
force-pushed
the
perf/cel-static-base-env
branch
from
bcc0a70 to
29397b2
Compare
GetCelEnv rebuilt the entire CEL environment -- including kubernetes.Library() and the validation of all its declarations -- on every expression compile. That makes the compile (cache-miss) path the single largest source of CEL allocation, and it runs on every evaluation of an expression that attaches a CelEnv (e.g. catalog.query), which are non-cacheable by design. Build the environment-independent options once into a cached base *cel.Env (sync.OnceValues + EagerlyValidateDeclarations) and layer the per-call variables, registered types and caller functions on top with Env.Extend, which deep-copies and is safe to share across goroutines. Compile-path benchmark (BenchmarkRunExpressionContextCompile/smallEnv): before: 425747 B/op, 5442 allocs/op, 324us/op after: 199132 B/op, 2254 allocs/op, 179us/op (~53% bytes, ~59% allocs) Cache-hit path is unchanged (base env is untouched on a hit). GetCelEnv is retained for external callers and now shares the static option set.
adityathebe
force-pushed
the
perf/cel-static-base-env
branch
from
29397b2 to
c1718f6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
run_expression_bench_test.go (1)
165-168: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHoist
Templateconstruction out of the timed loop.Line 165 creates a new
Templateand[]cel.EnvOption{noopFn}every iteration, which adds benchmark-only allocation noise. Build it once beforeb.ResetTimer()and reuse it.♻️ Proposed fix
b.Run(name, func(b *testing.B) { env := benchExprEnv(withConfig) + tmpl := Template{ + Expression: expression, + CelEnvs: []cel.EnvOption{noopFn}, + } b.ReportAllocs() b.ResetTimer() for i := 0; i < b.N; i++ { - out, err := RunExpression(env, Template{ - Expression: expression, - CelEnvs: []cel.EnvOption{noopFn}, - }) + out, err := RunExpression(env, tmpl) if err != nil { b.Fatal(err) }🤖 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 `@run_expression_bench_test.go` around lines 165 - 168, Hoist the per-iteration Template and []cel.EnvOption{noopFn} construction out of the benchmark loop in RunExpressionBenchmark so the timed section only measures RunExpression. Create the Template once before b.ResetTimer(), store the reusable Template and CelEnvs, and pass them into RunExpression inside the loop to remove benchmark-only allocation noise.
🤖 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 `@cel.go`:
- Line 8: The Docker build image is still on an older Go version that cannot
compile code using sync.OnceValues. Update the Dockerfile base image from
golang:1.19-alpine to a Go 1.21+ image so it matches the module and CI
toolchain, and ensure the build stage uses a Go version that supports the
symbols introduced in cel.go.
---
Nitpick comments:
In `@run_expression_bench_test.go`:
- Around line 165-168: Hoist the per-iteration Template and
[]cel.EnvOption{noopFn} construction out of the benchmark loop in
RunExpressionBenchmark so the timed section only measures RunExpression. Create
the Template once before b.ResetTimer(), store the reusable Template and
CelEnvs, and pass them into RunExpression inside the loop to remove
benchmark-only allocation noise.
🪄 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: 1d12c9d7-5066-4e77-9df7-d681529aff0d
📒 Files selected for processing (3)
cel.gorun_expression_bench_test.gotemplate.go
2 participants
Problem
GetCelEnvrebuilt the entire CEL environment on every expression compile —funcs.CelEnvOption,kubernetes.Library(), the cel-go extensions, stdlib, gomplate's custom functions — and thencel.NewEnvvalidated all of those declarations from scratch. In production heap profiles this compile path was the single largest source of CEL allocation.It runs more often than the cache-hit benchmark suggests:
CelEnv(e.g. duty'scatalog.query,gitops.source, …), which are non-cacheable by design because their bindings capture the request context (IsCacheable()isfalsewhenlen(CelEnvs) != 0).Change
Build the environment-independent options once into a cached base
*cel.Envand layer only the per-call delta on top withEnv.Extend:staticCelEnvOptions()— the constant options (libraries, stdlib, extensions, gomplate funcs).baseCelEnv—sync.OnceValues+cel.EagerlyValidateDeclarations(true)so the heavy declarations are validated a single time for the process.RunExpressionContextnow compiles againstbase.Extend(perCallOptions...), where the per-call options are: registered native types (typeAdapters), onecel.Variableper env key, callerFunctions, andCelEnvs.cel.Env.Extenddeep-copies the env (and copies the type registry), so the cached base is never mutated and is safe to share across goroutines.EagerlyValidateDeclarationsis what letsExtendreuse the base's validated declarations and only validate the small per-call delta.GetCelEnvis retained for external callers and now sharesstaticCelEnvOptions(). This also fixes a latent aliasing bug wherevar opts = funcs.CelEnvOptioncould append into the package-level slice's backing array.Benchmark
New
BenchmarkRunExpressionContextCompileexercises the compile (cache-miss) path.arm64, same machine, before vs after:The cache-hit path is unchanged — the base env is neither built nor extended on a hit.
Verification
go test .passes;go vet/gofmtclean.-racesmoke test (32 goroutines hammering the compile path concurrently) — no data races on the shared base env.go.work: CEL templating tests pass.RegisterType/typeAdapterskept off the cached base and applied per-call, so runtime type registration still takes effect.Notes for reviewers
GetCelEnv/RegisterTypeacross the flanksource Go repos, but both are kept working for out-of-tree consumers.Functionsattached, which keeps them out of the program cache; fixing that would let this base-env work also collapse their first-compile cost.Summary by CodeRabbit