Skip to content

Env copy on write - #1405

Open
TristonianJones wants to merge 8 commits into
cel-expr:masterfrom
TristonianJones:env-copy-on-write
Open

Env copy on write#1405
TristonianJones wants to merge 8 commits into
cel-expr:masterfrom
TristonianJones:env-copy-on-write

Conversation

@TristonianJones

Copy link
Copy Markdown
Collaborator

Introduce copy-on-write semantics for checker/env.go

Closes #1398

| Benchmark Case | Before (ns/op) | After (ns/op) | Δ Time | Before (B/op) | After (B/op) | Δ Memory | Before (allocs) | After (allocs) | Δ Allocs |
| :--- | :---: | :---: | :---: | :---: | :---: | :---: | :---: | :---: | :---: |
| BenchmarkProgramPlan/Default | 8,153 | 930 | **-88.6%** | 8,320 | 1,416 | **-83.0%** | 36 | 26 | **-27.8%** |
| BenchmarkProgramPlan/OptimizeUnneeded | 7,344 | 1,150 | **-84.3%** | 8,784 | 1,512 | **-82.8%** | 50 | 32 | **-36.0%** |
| BenchmarkProgramPlan/OptimizeNeeded | 8,370 | 2,164 | **-74.1%** | 10,224 | 2,976 | **-70.9%** | 67 | 52 | **-22.4%** |
@TristonianJones
TristonianJones requested a review from l46kok August 6, 2026 23:17
Comment thread checker/scopes.go
Comment thread cel/env.go Outdated
@TristonianJones

Copy link
Copy Markdown
Collaborator Author

Done, PTAL.

Comment thread cel/env.go Outdated
if chk != nil {
// If the type-checker has already been instantiated, then the e.declarations have been
// validated within the chk instance.
chkOptsCopy = append(chkOptsCopy, checker.ValidatedDeclarations(chk))

@l46kok l46kok Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this check is now present on initChecker(), I wonder if we should remove this.

The case I'm wondering about is disabling a declaration on an extended child environment:

  1. baseEnv declares custom function foo
  2. compiling foo with baseEnv succeeds
  3. childEnv extends from baseEnv with DisableDeclaration on foo
  4. comping foo with childEnv should fail here

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, and yes, it should be removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Env.Extend deep-copies environment state that is almost always identical to the parent's

2 participants