symcc: add SymSchema for amortized SymEntities across request envs#2467
symcc: add SymSchema for amortized SymEntities across request envs#2467tomlikestorock wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new SymSchema API in cedar-policy-symcc to amortize the expensive SymEntities::of_schema computation across many request environments, enabling callers to efficiently build multiple SymEnvs that share precomputed symbolic entities.
Changes:
- Add
SymSchema(public) to precompute and reuseSymEntitiesper schema viasym_env(&RequestEnv). - Update
SymEntitiesto wrap its internal map inArc<...>to enable cheap sharing acrossSymEnvinstances. - Add integration tests to assert
SymSchema::sym_envmatchesSymEnv::new, reuses entity storage across calls, and rejects unknown actions; update exports and changelog.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cedar-policy-symcc/tests/integration_tests.rs | Adds tests covering SymSchema correctness, reuse semantics, and error behavior. |
| cedar-policy-symcc/src/symcc/symbolizer.rs | Updates SymEntities construction to wrap maps in Arc. |
| cedar-policy-symcc/src/symcc/interpretation.rs | Updates SymEntities::interpret to return an Arc-backed SymEntities. |
| cedar-policy-symcc/src/symcc/env.rs | Changes SymEntities storage to Arc and adds the new SymSchema type/API. |
| cedar-policy-symcc/src/symcc/encoder.rs | Updates unit tests to construct SymEntities with Arc::new(...). |
| cedar-policy-symcc/src/symcc.rs | Re-exports SymSchema from the symcc module. |
| cedar-policy-symcc/src/lib.rs | Re-exports SymSchema at the crate root. |
| cedar-policy-symcc/CHANGELOG.md | Documents the new SymSchema API in the Unreleased section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn sym_env(&self, req_env: &RequestEnv) -> crate::err::Result<SymEnv> { | ||
| let env = Environment::from_request_env(req_env, self.schema.as_ref()) | ||
| .ok_or_else(|| crate::err::Error::ActionNotInSchema(req_env.action().to_string()))?; | ||
| Ok(SymEnv { | ||
| request: SymRequest::of_request_type(&env.req_ty)?, | ||
| entities: self.entities.clone(), | ||
| }) | ||
| } |
There was a problem hiding this comment.
This is a pre-existing issue in SymEnv::new (same ok_or_else(ActionNotInSchema) on line 56). SymSchema::sym_env matches the existing behavior. Should I change both?
There was a problem hiding this comment.
I think changing both makes sense, no need to block this PR though.
eec9299 to
ab52d19
Compare
Wrap SymEntities inner BTreeMap in Arc for cheap cloning. Add SymSchema struct that precomputes SymEntities once per schema and produces SymEnv instances via sym_env(), reusing the shared entities. Callers checking many request environments against the same schema (e.g. policy subsumption with large action lists) avoid rebuilding symbolic entities per environment: - without SymSchema: ~3.7s/env - with SymSchema: ~0.9ms/env + 1.86s one-time build Signed-off-by: Thomas Hill <thill@cloudflare.com>
ab52d19 to
8b682e1
Compare
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 82.35% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 88.23% Status: PASSED ✅ Details
|
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 82.35% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 88.23% Status: PASSED ✅ Details
|
SymEntities reverted to a plain BTreeMap wrapper — the Arc now lives on SymEnv.entities and SymSchema.entities, keeping SymEntities a value type while preserving cheap Arc-clone sharing in SymSchema::sym_env(). Signed-off-by: Thomas Hill <thill@cloudflare.com>
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 81.25% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 88.23% Status: PASSED ✅ Details
|
|
Looks good to me! I wonder if we want to eventually update the |
john-h-kastner-aws
left a comment
There was a problem hiding this comment.
approving with a comment on naming. No specific better name in mind, but probably worth a bit of thought.
I wonder if we want to eventually update the CompiledPolicy/CompiledPolicySet::compile API or add a new top level API to take a SymSchema to avoid the duplicate work and memory.
I think we should. Probably change CompiledPolicySet::compile to accept the new SymSchema, and leaving CompiledPolicySet::compile_with_custom_symenv as the escape hatch in case that doesn't work for some use case.
For that we'd want to update the Lean model to match, but it should be trivial.
| /// [`SymSchema::sym_env`], which is significantly cheaper than rebuilding the | ||
| /// symbolic entities per environment (as [`SymEnv::new`] does). | ||
| #[derive(Debug, Clone)] | ||
| pub struct SymSchema { |
There was a problem hiding this comment.
I'm not sure that SymSchema is the best name here since the schema isn't symbolic, the entities are. Would CompiledSchema be better? I don't know that it actually makes more sense, but we have "compiled" the schema into SymEntities. Any thoughts @lianah ?
There was a problem hiding this comment.
I think CompiledSchema sounds good. Also, it's similar to CompiledPolicy which follows a similar pattern and contains the type checked policy + the symbolic representation of the policy.
There was a problem hiding this comment.
Since Liana's agrees, @tomlikestorock , can you do the rename?
There was a problem hiding this comment.
oh sure, no problem
Description of changes
Add SymSchema to cedar-policy-symcc for amortizing the expensive SymEntities::of_schema computation across multiple request environments.
Callers checking many request environments against the same schema (e.g. policy subsumption with large action lists) previously rebuilt symbolic entities per environment (~3.7s each in my perhaps pathological case). SymSchema precomputes them once (~1.86s for me) and produces SymEnv instances via sym_env() that share the entities via Arc (0.9ms each).
Example usage:
I benchmarked this against the non-deprecated symcc with good results. Since this wraps the symcc functionality I don't believe it requires a lean change.
Since this is a new public type I figured this is a new minor version at least.
Issue #, if available
None, perhaps a continuation cousin of #2439. I can make a new issue if desired.
Checklist for requesting a review
The change in this PR is (choose one, and delete the other options):
cedar-policy(e.g., addition of a new API).I confirm that this PR (choose one, and delete the other options):
I confirm that
cedar-spec(choose one, and delete the other options):I confirm that
docs.cedarpolicy.com(choose one, and delete the other options):