perf(policy-engine): drop per-request deep copy of policy params - #2617
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughPolicy execution no longer deep-copies parameters for request, response, or streaming callbacks. Each callback receives ChangesPolicy parameter sharing
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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
🤖 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 `@gateway/gateway-runtime/policy-engine/internal/executor/chain.go`:
- Around line 143-145: Update the policy dispatch around OnRequestHeaders so
policies cannot mutate the shared spec.Parameters.Raw map: replace the raw-map
argument with the established read-only accessor, or create a defensive
per-request clone before invoking the callback. Preserve the existing parameter
values while ensuring concurrent requests never share a mutable map instance.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1498b86e-8d1b-4be6-b24b-db619f6e8ef6
📒 Files selected for processing (2)
gateway/gateway-runtime/policy-engine/internal/executor/chain.gogateway/gateway-runtime/policy-engine/internal/executor/chain_test.go
💤 Files with no reviewable changes (1)
- gateway/gateway-runtime/policy-engine/internal/executor/chain_test.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
deepCopyParams did a full JSON marshal/unmarshal round-trip on spec.Parameters.Raw at all six execution phases on every request. Params are an immutable snapshot published once at chain-build time and shared read-only across concurrent requests, so the copy was pure overhead — it accounted for ~30% of the ext_proc CPU tree. - Pass spec.Parameters.Raw directly to each policy On* method - Remove deepCopyParams and the now-unused encoding/json import - Drop the three orphaned TestDeepCopyParams_* tests Verified with go test -race on the executor and kernel packages (kernel covers the production chain-build/publish path). An A/B benchmark with a realistic nested param map showed ~30-40x faster execution and ~14x fewer allocations per policy.
- Remove TestParameterCloneFailure_SpanStatus_AllPhases and its helpers (badParamsSpec, assertParameterCloneFailure) from the executor package — these tested the now-deleted deepCopyParams error path. - Update TestProcessSpanStatus_PolicyError500 in the kernel package to trigger handlePolicyError via a mockEvaluatorError CEL evaluator instead of the removed channel-value deepCopyParams failure trick. - Add newSpanStatusServerWithCEL and buildChainWithPolicyAndCondition helpers to support CEL-error injection in extproc span status tests. Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
3 participants
Purpose
The policy chain executor deep-copied each policy's parameters on every request, at all six execution phases (request/response headers, request/response body, streaming request/response body). The copy was a full JSON marshal/unmarshal round-trip over
spec.Parameters.Raw. Profiling attributed roughly 30% of the ext_proc CPU tree to this deep copy, along with the corresponding allocation churn.Policy parameters are an immutable snapshot: they are published once at chain-build time (on the xDS push / config load) and shared read-only across concurrent request handlers. Config updates build entirely new chain objects and swap them in atomically, so a live chain's parameters are never mutated. The per-request copy was therefore pure overhead.
Goals
Eliminate the redundant per-request parameter copy and the allocations it produced, without changing observable behavior.
Approach
spec.Parameters.Rawdirectly to each policyOn*method instead of a per-request deep copy.deepCopyParamshelper and the now-unusedencoding/jsonimport.TestDeepCopyParams_*unit tests.User stories
N/A
Documentation
N/A — internal performance optimization with no user-facing or API changes.
Automation tests
Security checks
Samples
N/A
Related PRs
N/A
Test environment
Go (policy-engine module); linux/amd64 and darwin/arm64.
Related Issues
Related #2615
Checklist
Remarks
An A/B benchmark with a realistic nested parameter map (a combined JWT-validation + rate-limit config) through a body-buffering policy showed roughly 30-40x faster chain execution and ~14x fewer allocations per policy once the copy was removed. The repo's existing executor benchmarks use empty parameter maps, where the old code short-circuited before the JSON round-trip, so they did not surface this cost.
The safety of sharing the map now rests on the convention that policies treat parameters as read-only. A hard guardrail (e.g. a read-only accessor type in the SDK) would be a separate, breaking follow-up and is out of scope here.