Repository navigation
enhancement(compiler): cache external query paths - #1987
Conversation
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. |
There was a problem hiding this comment.
bruceg
left a comment
There was a problem hiding this comment.
LGTM but I have a question about a clone that remains
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9284f2fb7
ℹ️ 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".
Summary
VRL currently clones the lookup path every time it reads a field such as
.foo.bar. Those allocations add up in programs that read several fields per event.This PR builds the full path once during compilation and reuses it for event and metadata reads. Each query owns a single path.
existsanddelalso borrow that stored path throughQuery::external_path.Rust API change and Vector follow-up
Query::external_pathnow returnsOption<&OwnedTargetPath>. Callers that need an owned path can use.cloned().Vector's
set_semantic_meaningneeds this update when bumping VRL:That clone occurs once during compilation, when Vector stores the path in
MeaningList.Performance
Recorded AST measurements on Apple M4 Max, Rust 1.96.1, with the system allocator: 100 samples, 3 s warmup, 5 s measurement. Compilation and fresh target setup are excluded; runtime reset, execution and result/target drops are included.
Field reads and remap, compared with upstream
f363989fbebdbf63f2bcd2905d6daee0ccfa6d00:.a.a.b.a.b.c%a.b.c.(root control)Borrowed accessor, compared with the initial PR commit
f57fcc003:exists(.a.b.c)del(.a.b.c)These recorded results used the original standalone runner with
stdlib-base. On the current revision, the Cargo benchmark target isstdliband requires thedefaultandtestfeatures.external_field_readsis the Criterion filter for cases registered frombenches/runtime/mod.rs:For a new comparison, use the same
benches/runtime/mod.rsmodule andruntime::external_field_readsregistration inbenches/stdlib.rson both revisions. Add--save-baseline beforeto the command on the baseline revision, then--baseline beforeon the changed revision. Results cover VRL execution, not a full Vector pipeline.How did you test this PR?
make fmt,make fmt-check, andmake check-clippy(workspace, all targets/features).cargo test --lib --no-default-features --features stdlib-base,enable_system_functions compiler::expression(92 passed).cargo run --package vrl-tests --bin vrl-tests -- --pattern expressions/query(15 passed).cargo run --package vrl-tests --bin vrl-tests -- --pattern metadata(6 passed).cargo run --package vrl-tests --bin vrl-tests -- --pattern exists(3 passed).cargo run --package vrl-tests --bin vrl-tests -- --pattern functions/del(9 passed).cargo test --lib --no-default-features --features stdlib-base,enable_system_functions stdlib::del(1 passed).cargo run -p release -- check-changelog.cargo bench --bench stdlib --features default,test -- external_field_reads --test(8 cases passed after moving them into the shared runner).One new VRL fixture checks event and metadata collection copy independence after mutations, plus root, indexed and missing reads.
Does this PR include user-facing changes?
Checklist
dd-rust-license-tool writeand commit the changes. No dependencies changed.