chore(compiler): speed up typecheck for deep else-if ladders (remove quadratic complexity) - #1865
Conversation
![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: 76838b148a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Update: fixed so that the semantics are now exactly the same as beforeCodex flagged that flattening While that was an intentional choice in the original PR to make the behavior of else if predicates more like if predicates, because of how we were peeling, it would lead to an incorrect behavior for a block that did Fix: after merging arm states, re-apply Tests: expanded Running tests on main branch vrl: copied these new
So these goldens match pre-flatten nested semantics; this commit is not introducing new local-escape behavior and not changing semantics. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abe4e9fc7c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Hi @klondikedragon, thank you for your contribution! I will take a look. |
8825c44 to
7aaf471
Compare
|
Thanks @pront for taking a look! We did a bunch for testing/profiling on our workloads. Vector was still taking 15-20 seconds to fully startup even after our latest changes, so we dug in again. The O(N^2) problem with depth of nested if-else arms was not recurring, it was now due to cost of merging more complex type states: Profiling showed compile time still dominated by per-operator
Real-world effect (release build, single thread, median):
Overall, combined with this PR's flattening fix: ~16s -> 0.27s VRL compilation time for one of our larger VRL programs. On 1-2 core hosts compiling ~24 programs at startup this makes a huge difference. Reproduce with the included example driver: Correctness:
I know this PR is pretty large now so it could be split up. On the other hand, each individual commit is fairly narrow and the concepts are all related and one depends on the other, so conceptually it still feels like this is one logical unit that could merge together. I'm happy to take any feedback. thanks!! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7aaf471cdd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e73c7912b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3653707f86
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| out.push_str(" ._itl.class = \"NOTABLE\"\n"); | ||
| out.push_str(&format!(" ._itl.arm = {i}\n")); | ||
| } | ||
| out.push_str("} else {\n"); |
There was a problem hiding this comment.
When --arms 0 is accepted, the loop above emits no opening if, but this unconditional } else { still gets appended, so the generated VRL starts with an unmatched brace and compile_once exits before any timing data is collected. Either reject zero in parse_args or emit a valid else-only/no-op baseline for this case.
Useful? React with 👍 / 👎.
| for _ in 0..coalesce_chain.max(1) { | ||
| expr.push_str(&format!("to_string(.src{c}) ?? ")); |
There was a problem hiding this comment.
When a benchmark is run with --coalesce-chain 0, this silently promotes the generated expression to one to_string(.srcN) ?? ... step, while the summary still prints chain=0. That makes zero-chain coalesce experiments measure fallible-call/coalesce work under a mislabeled configuration, so either reject 0 in argument parsing or store/report the effective value used here.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| println!( | ||
| "else-if ladder compile (fields/arm={}, locals/arm={}, coalesce/arm={} chain={} shared={}, {env}, warmup={}, repeat={})", |
There was a problem hiding this comment.
When comparing synthetic runs with and without --fields-disjoint, the generated program changes from repeatedly updating a fixed event shape to growing distinct fields per arm, but the printed configuration only reports shared for locals and never records fields_disjoint. That makes saved benchmark output for these two modes indistinguishable except for timings/source size, so include this flag in the run label.
Useful? React with 👍 / 👎.
@klondikedragon I highly recommend splitting this up as much possible otherwise it will be a while before it is merged. |
|
@pront I could split this so that all of the July 23rd commits are on this PR, and all of the Aug 6+ commits are in a follow-on. The first batch stands on its own and is a great speedup by itself. the second follow-on is another big 5x speedup but also naturally stands on its own. how does that sound? |
@klondikedragon that's a good start. However, I would try to split pre-July 23rd commits as well if possible. |
3653707 to
7bb8ee8
Compare
|
@pront understood. I'm splitting the first batch in half again as they are independently composable (different files, different optimizations). will update shortly and open the second PR shortly. thanks! |
Locals assigned in if and else-if predicates remain visible after the chain; locks flatten semantics vs nested else-scope strip. Co-authored-by: Cursor Grok 4.5 Signed-off-by: Klondike Dragon <klondikedragon@gmail.com>
Peel parser else { if ... } into multi-arm IfStatement so
typecheck visits each arm once instead of re-walking nested
suffixes. Keeps predicate side-effect order and join semantics.
Co-authored-by: Cursor Grok 4.5
Signed-off-by: Klondike Dragon <klondikedragon@gmail.com>
Wall-clock harness for synthetic ladders and --program files, plus --env-width/--env-depth/--env-seed-fields to seed a spine+stubs ExternalEnv Kind for more realistic compile timing. Co-authored-by: Cursor Grok 4.5 Signed-off-by: Klondike Dragon <klondikedragon@gmail.com>
Signed-off-by: Klondike Dragon <klondikedragon@gmail.com>
Flattened typecheck treated later-predicate bindings as definite after the chain. Re-apply apply_child_scope from post-arm0 locals and expand if/else-if suite coverage. Co-authored-by: Cursor Grok 4.5 Signed-off-by: Klondike Dragon <klondikedragon@gmail.com>
7bb8ee8 to
c01beae
Compare
|
@pront - this split into 3 pieces:
Happy to take more feedback. Thanks! |
|
P.S. this isn't just a speedup. If you've got a VRL program with a large number of else-if arms, it will currently stack-overflow, because the unflattened representation recurses per arm. (e.g., the test driver with 160 arms stack overflows attempting to use 1 GB of stack). It's not a good idea to have 160 straight else-if in prod programs anyways LOL, which is why this was never hit, but it's removing something could cause vrl/vector to crash or use an unexpected amount of memory, etc. |
2 participants
Summary
Hi! I 💖 vector and have been using it for many years. thank you!
This PR speeds up VRL compile / typecheck for programs with long nested
else ifchains by removing O(n^2) behavior for this case.How we discovered it: We're using VRL to classify events in complex ways, which means some of our VRL has long else-if chains (40-60+ arms in several places). Our test suite (which runs validate over each VRL file + exercises data flow) went from a few seconds total to minutes as we've developed more VRL files. We found vector taking a long time (8+ seconds) to validate/compile some of our VRL files. Profiling pointed at
Expression::apply_type_info/IfStatement/Kind/TypeState/LocalEnvclone and merge.After these changes, these same VRL files validate/compile an order of magnitude faster (~8s to ~0.8s), taking our overall test suite run back from minutes to seconds.
While VRL compile-time is a one-time cost at startup, it's single-threaded and with our VRL, machines were taking 30+ seconds from vector startup before processing events. This removes that long delay.
I've contributed a smaller PR in the past, but this is my first time digging in to the compiler in VRL... I'm happy to adjust and discuss!
Change Type
Is this a breaking change?
Semantics note
After some updates to the PR, there are no changes to semantics. Additional tests were added to increase coverage of semantics for edge cases around if-else-if chains and related.
Problem
else ifto nestedelse { if ... }.IfStatement::type_infoclonesTypeState, typechecks both sides, then merges.Kindtrees and local bindings, so cost grows O(n^2) rather than a single linear pass over arms (O(n)).Approach
Ordered commits:
else { if ... }into a multi-armIfStatementat compile so each arm is typechecked once. Main benefit that solves quadratic behavior.Kindinserts and assignment type updates (avoid deep-clone on every nested write).Arc+ COW forCollectionknown maps.examples/else_if_ladder_compile.rs) with ladder +--programand--env-width/--env-depthknobs.Arc+ COW forLocalEnvbindings (this commit further speeds up ~2.7× on a large--programworkload in that example driver, on top of the earlier commits).apply_child_scopefrom the post-arm-0 local env so bindings first introduced in later predicates are not definite after the chain (restoring nestedelse { if }Block semantics).Benchmarks
Timings below are from my local release builds of
examples/else_if_ladder_compileSynthetic benchmark
Performance after primary fix of removing quadratic complexity:
After doing in-place mutations of
Kindplus cheap clones ofCollectionandLocalEnv(viaArc), the 60 arm test drops to med_ms=49.4:So approximately a 40x speedup for the 60-arm case before to final after. The performance is also the same after the 8th commit.
Synthentic benchmarks are interesting, but how about a real-world case...
End-to-end Vector validate (
bench-vrl-runtime.py)Our CI harness in one of its steps uses vector validate over all of VRL scripts. We exposed this piece of it to benchmark this piece (with our large VRL it was causing big slowdowns). We built vector 0.57 with our branch cherry-picked to the version of VRL paired with 0.57, and compared it to stock vector on my Linux system (was version 0.56, sorry it's not quite the same, I could re-run if you want; other systems where I'm running 0.57 show almost identical behavior though, and the profiling data showed the hotspot was the type checking and VRL synthetic benchmarks confirm that):
Our bench-vrl-runtime.py isn't part of the PR, it's something we wrote to interface with our VRL CI.
Same harness flags; PATH selects the Vector binary.
Stock (
/usr/bin/vector0.56.0 release on this machine):PATH=/usr/bin:$PATH python3 tools/bench-vrl-runtime.py \ --scenario mix --events 1 --arms full --work /tmp/sl-vrl-runtime-benchPatched (workspace Vector 0.57.0 release build pinned to this VRL):
So
cfg-fullvalidate ~8.07s → ~0.79s on that A/B.How did you test this PR?
make allgreenexpressions/if_statementfiles: first-ifpredicate escape, an assert-heavy success file for cross-arm / type-merge / short-circuit cases, and E701 goldens for else-if pred escape, bracedelse { if }, else-if body escape, body-->later-pred isolation, multi-stmt else, and third-arm pred + finalelseelse_if_ladder_compileDoes this PR include user facing changes?
our guidelines.
Checklist
run
dd-rust-license-tool writeand commit the changes. More details here.AI disclosure
I used Cursor Grok 4.5 to help develop this PR (understanding the compiler code base, developing a strategy, writing code and tests). I've personally reviewed the code and believe it's correct. This is my first time in the VRL compiler codebase though, so I greatly appreciate your expertise in reviewing!
References
If you like this PR, consider reviewing my unrelated vector PRs I'm writing up for vectordotdev/vector#25810 and vectordotdev/vector#25815