Skip to content

fix: cel env after cache lookup#161

Merged
moshloop merged 2 commits into
mainfrom
fix/cel-env-after-cache-lookup
Jun 23, 2026
Merged

fix: cel env after cache lookup#161
moshloop merged 2 commits into
mainfrom
fix/cel-env-after-cache-lookup

Conversation

@adityathebe

@adityathebe adityathebe commented Jun 23, 2026

Copy link
Copy Markdown
Member

Summary by CodeRabbit

  • Tests

    • Added benchmarks for expression evaluation performance testing across different configuration sizes.
  • Refactor

    • Optimized expression caching to skip unnecessary environment setup on cache hits.
  • Chores

    • Enhanced CI/CD benchmark workflow to run expanded performance test suite on pull requests.

RunExpressionContext built the full CEL env option set (GetCelEnv, including kubernetes.Library() and one cel.Variable per env key) on every call before consulting the compiled-program cache, then discarded it on cache hits, since cel.NewEnv only runs to compile a new program.

This made GetCelEnv (notably kubernetes.Library()) the dominant lifetime allocator on the CEL path: in a production config-db heap profile it accounted for ~38% of alloc_space, called on every evaluation.

Move the cache lookup ahead of GetCelEnv and construct env options only inside the cache-miss branch where cel.NewEnv actually consumes them. Cache-hit steady state now skips GetCelEnv entirely. Behavior is unchanged: the cached cel.Program already carries its variable bindings and Eval still receives the serialized data.
Add BenchmarkRunExpressionContext (cacheHit smallEnv/largeEnv) guarding the cache-hit steady state where GetCelEnv must not run, modeled on the production config-item env shape with no duty/config-db dependencies.

Wire it into the benchmark workflow alongside BenchmarkSerialize: base-vs-head benchstat, a report comment posted to the PR, and the existing 5% regression gate. Copy the new bench file into the base worktree so both runs execute the same suite, only the implementation under test differing.
@github-actions github-actions Bot added the build label Jun 23, 2026
@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 08d5f45e-cb09-4c5c-997b-eacafd7d4c79

📥 Commits

Reviewing files that changed from the base of the PR and between d6bef8e and 23e1cff.

📒 Files selected for processing (3)
  • .github/workflows/benchmark.yml
  • run_expression_bench_test.go
  • template.go

Walkthrough

RunExpressionContext in template.go is refactored to defer CEL environment option construction (GetCelEnv, function wrapping, CelEnvs) into the cache-miss branch, avoiding allocations on cache hits. A new BenchmarkRunExpressionContext benchmark measures this hot path across small and large env shapes, and the CI benchmark workflow is updated to include it.

Changes

CEL Cache-Hit Optimization and Benchmarking

Layer / File(s) Summary
Defer envOptions to cache-miss path
template.go
GetCelEnv, function wrapping, and CelEnvs population are moved inside the prg == nil block so they are skipped on cache hits; comments document the deferral intent.
BenchmarkRunExpressionContext and CI wiring
run_expression_bench_test.go, .github/workflows/benchmark.yml
New benchmark defines a constant CEL expression, runs small and large sub-benchmarks that warm the cache once then measure allocations per iteration; CI workflow copies the new benchmark file into the base worktree and expands both base and head go test -bench filters to include BenchmarkRunExpressionContext.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: deferring CEL environment option construction until after cache lookup in RunExpressionContext, which is an optimization to avoid unnecessary work on cache hits.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/cel-env-after-cache-lookup
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch fix/cel-env-after-cache-lookup

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown

Benchstat

Base: d6bef8ed6b708ce16cf9a1e721bd46168af35aab
Head: 23e1cff2d7f984b0bd4be5abf9f8dc80d5516aad

6 improvement(s)
Benchmark Base Head Change p-value
RunExpressionContext/cacheHit/smallEnv-4 8593.0 664.0 -92.27% 0.002
RunExpressionContext/cacheHit/smallEnv-4 187.00 20.00 -89.30% 0.002
RunExpressionContext/cacheHit/smallEnv-4 9.388µ 1.810µ -80.72% 0.002
RunExpressionContext/cacheHit/largeEnv-4 10.235Ki 2.445Ki -76.11% 0.002
RunExpressionContext/cacheHit/largeEnv-4 269.0 101.0 -62.45% 0.002
RunExpressionContext/cacheHit/largeEnv-4 18.60µ 10.85µ -41.66% 0.002
Full benchstat output
goos: linux
goarch: amd64
pkg: github.com/flanksource/gomplate/v3
cpu: AMD EPYC 7763 64-Core Processor                
                                         │ bench-base.txt │           bench-head.txt           │
                                         │     sec/op     │   sec/op     vs base               │
RunExpressionContext/cacheHit/smallEnv-4     9.388µ ± 24%   1.810µ ± 1%  -80.72% (p=0.002 n=6)
RunExpressionContext/cacheHit/largeEnv-4     18.60µ ±  1%   10.85µ ± 8%  -41.66% (p=0.002 n=6)
Serialize/Size-10-4                          37.17µ ±  3%   36.68µ ± 0%        ~ (p=0.065 n=6)
Serialize/Size-100-4                         354.5µ ±  1%   351.3µ ± 2%        ~ (p=0.240 n=6)
Serialize/Size-1000-4                        3.521m ±  1%   3.529m ± 0%        ~ (p=0.485 n=6)
Serialize/Size-10000-4                       40.91m ±  1%   40.49m ± 2%        ~ (p=0.132 n=6)
Serialize_NoNativeTypes/Size-100-4           90.65µ ±  1%   91.53µ ± 2%        ~ (p=0.485 n=6)
Serialize_NoNativeTypes/Size-1000-4          921.4µ ±  1%   924.9µ ± 4%        ~ (p=0.093 n=6)
Serialize_NoNativeTypes/Size-10000-4         9.330m ±  1%   9.282m ± 0%        ~ (p=0.093 n=6)
geomean                                      399.3µ         312.5µ       -21.75%

                                         │ bench-base.txt │            bench-head.txt             │
                                         │      B/op      │     B/op      vs base                 │
RunExpressionContext/cacheHit/smallEnv-4      8593.0 ± 0%     664.0 ± 0%  -92.27% (p=0.002 n=6)
RunExpressionContext/cacheHit/largeEnv-4    10.235Ki ± 0%   2.445Ki ± 0%  -76.11% (p=0.002 n=6)
Serialize/Size-10-4                          11.06Ki ± 0%   11.06Ki ± 0%        ~ (p=1.000 n=6) ¹
Serialize/Size-100-4                         95.52Ki ± 0%   95.51Ki ± 0%        ~ (p=0.701 n=6)
Serialize/Size-1000-4                        952.7Ki ± 0%   952.7Ki ± 0%        ~ (p=0.732 n=6)
Serialize/Size-10000-4                       10.06Mi ± 1%   10.05Mi ± 0%        ~ (p=1.000 n=6)
Serialize_NoNativeTypes/Size-100-4           36.33Ki ± 0%   36.33Ki ± 0%        ~ (p=1.000 n=6)
Serialize_NoNativeTypes/Size-1000-4          365.6Ki ± 0%   365.6Ki ± 0%        ~ (p=0.545 n=6)
Serialize_NoNativeTypes/Size-10000-4         3.584Mi ± 0%   3.584Mi ± 0%        ~ (p=0.303 n=6)
geomean                                      152.0Ki        97.56Ki       -35.84%
¹ all samples are equal

                                         │ bench-base.txt │            bench-head.txt            │
                                         │   allocs/op    │  allocs/op   vs base                 │
RunExpressionContext/cacheHit/smallEnv-4      187.00 ± 0%    20.00 ± 0%  -89.30% (p=0.002 n=6)
RunExpressionContext/cacheHit/largeEnv-4       269.0 ± 0%    101.0 ± 0%  -62.45% (p=0.002 n=6)
Serialize/Size-10-4                            268.0 ± 0%    268.0 ± 0%        ~ (p=1.000 n=6) ¹
Serialize/Size-100-4                          2.518k ± 0%   2.518k ± 0%        ~ (p=1.000 n=6) ¹
Serialize/Size-1000-4                         25.80k ± 0%   25.80k ± 0%        ~ (p=1.000 n=6)
Serialize/Size-10000-4                        265.1k ± 0%   265.0k ± 0%        ~ (p=1.000 n=6)
Serialize_NoNativeTypes/Size-100-4             911.0 ± 0%    911.0 ± 0%        ~ (p=1.000 n=6) ¹
Serialize_NoNativeTypes/Size-1000-4           9.758k ± 0%   9.758k ± 0%        ~ (p=1.000 n=6) ¹
Serialize_NoNativeTypes/Size-10000-4          100.1k ± 0%   100.1k ± 0%        ~ (p=0.197 n=6)
geomean                                       3.896k        2.725k       -30.04%
¹ all samples are equal

@moshloop moshloop merged commit f34f0dc into main Jun 23, 2026
9 checks passed
@moshloop moshloop deleted the fix/cel-env-after-cache-lookup branch June 23, 2026 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants