[Ninja3047]

Assisted-by: Codex

Overview

This PR improves the copy constructor performance for the context structure. Instead of repeating the work done in the context() constructor and repopulating the env each time and calling std::time, we create a new container and copy the shared pointers from the parent env, which is much more efficient.

Additional information

This should reduce the cost of this bug fix: #30533

Requirements

• I have read and agree with the contributing guidelines
• AI usage disclosure: YES - I used AI to identify ways to improve the performance and study the fix. I also used it to help profile the resulting code and inspect the decompilation.

[CISC]

/bot review

[ggml-gh-bot]

Automated code review

Review of common/jinja/runtime.h - context copy constructor optimization. This is a small, single-purpose perf follow-up to PR #30533, touching one file, with the AI disclosure filled in. Checklists run: scope/quick-reject, security, general, ggml-adjacent (n/a), new-component/design (minor).

Blocking findings: none.

Correctness verification (looks good):

• mk_val<value_object>(*parent.env) resolves to the implicit copy constructor of value_object_t (value_object_t(const value_object_t &)) - the value_object_t(value & v) overload cannot bind to a value_object_t&. The implicit copy copies both parallel structures (val_obj ordered vector in the value_t base, plus the unordered lookup map) and has_builtins, so scope semantics are preserved: same entries, same order, same lookup map.
• has_builtins parity: parent env is always set to false in the primary ctor and never changed, and the copy propagates it. Equivalent to the old delegated-ctor behavior.
• true/True/false/False/none/None are stored as regular entries in the parent env, so they are carried by the copy exactly as the old rebuild loop did.
• Removed side effects (std::time(nullptr), the throwaway make_shared<std::string> for src before overwrite) were dead work only - current_time and src are overwritten from parent in both versions. No behavior change.
• One intentional difference: the env object's own stats field is now copied from the parent instead of being fresh. I checked all stats consumers (caps.cpp, runtime.cpp) - stats are only read on the values stored in the env, never on the env object itself, so this is harmless.
• Member init order (src, current_time, is_get_stats, env) matches declaration order, so no -Wreorder issues.
• Existing coverage in tests/test-jinja.cpp exercises for-loop rendering (which goes through context scope(ctx) and context loop_scope(scope)), so a regression here would be caught by the existing suite; no new test needed.

Will slow the review / worth a look:

(point 1) The change silently depends on the implicit copy constructor of value_object_t copying both the unordered map and the val_obj vector. value_object_t already has a trap here: its value_object_t(value & v) ctor rebuilds unordered from val_obj by hand, i.e., the class keeps two structures in sync manually. If someone later adds a custom copy ctor (or a new member) to value_object_t, this copy could silently drop the lookup map and break variable resolution in child scopes. A one-line comment in value_object_t (e.g. "copy ctor must copy both unordered and val_obj") or a brief note next to the context copy ctor would make that dependency explicit.

Nits:

(point 2) mk_val<value_object>(*parent.env) is fine and matches the file's idiom, but it relies on the extract_pointee unwrap; env(std::make_shared<value_object_t>(*parent.env)) would state the copy directly. Either is acceptable, just noting the simpler spelling.

(point 3) The comment "// inherit variables (for example, when entering a new scope)" now sits between init-list entries, which reads slightly oddly. Consider moving it to the line above the constructor signature, or leave as-is - minor.

Optional follow-up (not for this PR): context loop_scope(scope) at common/jinja/runtime.cpp:585 runs inside the per-item filtering loop, so a template with a select-style test still does one full env copy per item (O(N * V)). This PR shrinks the constant a lot; if profiling still shows it hot, a copy-on-write env (share the shared_ptr, clone on first set_val) would make scope creation O(1). Worth keeping in mind, not a request to change the approach here.

Style: ASCII only, comments concise, no unrelated changes. Good to merge from my side once (point 1) is considered.

This review was generated automatically by pi coding agent using zai-org/GLM-5.3. It may contain mistakes. Maintainers make the final call.