Conversation
Every evaluate() and Program.execute() call rebuilt the cel-rust context from scratch: each variable was re-boxed and each registered Python function was re-wrapped in a closure, so the per-call cost scaled with the size of the Python Context rather than the expression. Executing a pre-compiled `1 + 2` against a Context carrying the 47 extended-stdlib functions (the CLI's setup) cost ~7.5 us against ~0.13 us with no context; a Context with 200 variables cost ~32 us. The Context now builds that environment on first use and caches it behind a Mutex as an Arc<cel::Context<'static>>. add_variable, add_function and update drop the cache; set_variable_resolver does not need to, because the resolver is bound per call in a child scope (cel-rust's Context::new_inner_scope), which keeps the same lookup order as before (resolver, then registered variables). Handing out an Arc rather than borrowing the Python object means a callback may mutate, or re-enter evaluation with, the Context it is registered on; the mutation applies from the next evaluation. Dict contexts are still built per call, since a dict can change without notice. Both cases above now execute in ~0.15 us. The dict and no-context paths are unchanged within noise. Also factors the Python-function wrapper and the compile step into helpers shared by evaluate() and compile(), and corrects the panic message for an execution-time panic, which called itself a parser error. Claude-Session: https://claude.ai/code/session_019WbvXZFm8Nb2LXF2kiWoWW
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 202397ecd7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
update() applies entries in order and returns early on the first bad key or value, so with the invalidation at the end a failed call left earlier entries applied but the cache untouched: evaluations kept returning the old value until some later successful mutator happened to drop the cache. Every mutator now drops it first. Nothing can repopulate the cache during the mutator because it holds &mut self. Tests pin both the failed-update and failed-add_variable cases. Claude-Session: https://claude.ai/code/session_019WbvXZFm8Nb2LXF2kiWoWW
Resolves the CHANGELOG Unreleased conflict with #52; src/context.rs merged cleanly (the getters sit alongside the cache-invalidating mutators). Claude-Session: https://claude.ai/code/session_019WbvXZFm8Nb2LXF2kiWoWW
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Every
evaluate()andProgram.execute()call rebuilt the cel-rust context from scratch: each variable re-boxed into aBox<dyn Val>, each registered Python function re-wrapped in a fresh closure. So the per-call cost scaled with the size of the PythonContext, not the expression. The CLI, which registers all 47 extended-stdlib functions, paid ~7.5 µs of setup to evaluate1 + 2.What
Contextnow builds its cel environment on first use and caches it (Mutex<Option<Arc<cel::Context<'static>>>>).add_variable,add_functionandupdatedrop the cache.set_variable_resolverdoes not need to: the resolver is bound per call in a child scope via cel-rust'sContext::new_inner_scope, which keeps exactly the lookup order the resolver had on the root (resolver → registered variables → type identifiers), and leaves the root untouched and shareable.The evaluation borrows an
Arcclone rather than the Python object, so a callback may mutate, or re-enter evaluation with, theContextit was registered on; the mutation applies from the next evaluation, which is also what happened before (the old code copied variables and functions out before executing).Dict contexts are still materialised per call, because a dict can change between calls without notice.
Also in this PR, because the same code was being rewritten: the Python-function wrapper and the compile step are factored into helpers shared by
evaluate()/compile(), and the panic message for an execution-time panic no longer calls itself a parser error.Numbers
minof 3 × 50 000 iterations, release build, same machine. "Before" ismain's Rust (built from the #44 branch, which only touches Python).Program.execute()against…Context, 2 varsContext, 2 vars + 1 Python fn (called)Context, 2 vars + resolverContextwith the 47 extended-stdlib functionsContextwith 200 varsThe no-context and dict rows are within run-to-run noise (an alternating A/B of 7 × 100 000 iterations gave 0.13–0.14 vs 0.13–0.14 and 0.45–0.48 vs 0.42–0.48).
Tests
New
tests/test_context_reuse.pypins the contract: every mutator is visible on the next evaluation (including replacing a function and replacing the resolver); resolver precedence over registered variables is unchanged; a callback can mutate the context it runs under and can callevaluate()/execute()with that same context; oneContextserves many programs; aContextshared across 8 threads returns correct results. 528 passed, 1 skipped, 5 xfailed.cargo fmt --check,cargo clippy -D warnings,cargo test, ruff and mypy clean.Note for merging: this and #44 both add to the CHANGELOG's Unreleased section, so whichever lands second will need a trivial conflict resolution there.
https://claude.ai/code/session_019WbvXZFm8Nb2LXF2kiWoWW
Generated by Claude Code