Make for evaluation and list parsing linear, and fix several silent-null defects - #112
Open
toxik wants to merge 27 commits into
Open
Make for evaluation and list parsing linear, and fix several silent-null defects#112toxik wants to merge 27 commits into
toxik wants to merge 27 commits into
Conversation
… speed
The existing benchmarks measure single operations on tiny inputs, so none of
them can show whether a cost grows with the size of the input. These do.
Every step holds four times the data of the one before it. Read the ratio
between the steps rather than any single time. A linear cost answers in about
four times the time. A cost that grows with the square of the input answers in
about sixteen.
`list_scaling` evaluates a `for` expression. `for` binds the implicit variable
`partial` to the results of all previous iterations, so an evaluator that copies
that list on every iteration pays for every result collected so far. It pays
whether or not the body reads `partial`, and neither body here reads it.
`list_literal` parses a list literal. A parser that collects items by inserting
each one at the front of a vector moves every item already collected.
cargo +nightly bench -p dsntk-feel-evaluator --bench list_scaling
cargo +nightly bench -p dsntk-feel-parser --bench list_literal
The benchmarks stop at 6400 items, because `cargo bench` runs many iterations
and a quadratic cost makes larger sizes take minutes each. The example runs the
same shapes exactly once, so the large sizes stay usable, and the system timer
reports peak memory without adding a dependency:
/usr/bin/time -l cargo run --release --example scaling -- for 100000
Both build their input in code, so neither needs a data file.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RncmREziecjpii3CQNsyh2
A `for` expression copied the accumulated `partial` list into the iteration context on every iteration. The copy grew with the result, so the expression cost O(n squared). Move the list in and back out instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The list reduce action inserted each item at index 0. Every insert moved the whole accumulated vector, so parsing a list of n items cost O(n squared). Append, then reverse once. With the previous commit: 13.67 s to 0.36 s at 1,000 elements. Growth exponent 1.99 to 0.98. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ntext `a.b` evaluated the base through `get_value`, which clones the whole context just to read one entry. Read the entry directly when the base is a plain name, and fall through to the general path otherwise. 1.51x: 27.25 s to 18.06 s at 100,000 elements. Marginal cost of one field read drops from 19.89 allocations to 0.89. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Its 128-byte payload was more than twice any other variant, so it alone set `size_of::<Value>()` for every value in the engine. `size_of::<Value>()` 128 to 64, and the 11-entry context node 1,688 B to 984 B. 1.32x footprint (27.1 to 20.5 KB per element), 1.05x time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`is_match` called `Regex::new` on every call, so a `matches` call inside a loop recompiled the same pattern once per element. Profiling put 25.7% of runtime there. The pattern arrives already evaluated, so this is a bounded cache rather than a static. 1.34x: 8.49 s to 6.35 s at 50,000 elements. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Cloning a context deep-copied its whole entry map. Contexts are cloned once per element on several hot paths. Put the map behind an `Arc`, and make every mutation copy-on-write through `Arc::make_mut`. 2.46x time and 1.38x footprint: 12.67 s to 5.15 s, 19.5 to 14.2 KB per element, at 100,000. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A `for` body that provably cannot read `partial` has no dependency between iterations. Analyze the body once, when the evaluator is built. Split a single-list iteration across workers when it is safe and large enough. 2.18x at 100,000 elements. Output is byte-identical to the serial path, which is kept as the default and reachable with `DSNTK_PARALLEL=0`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
About half of evaluation time goes to `malloc` and `free`, and the system allocator on macOS does not return freed small blocks, so peak memory held every page the parse phase touched long after that data died. 1.29x time and 1.60x footprint: 2.32 s to 1.80 s, 13.6 to 8.5 KB per element, at 100,000. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ey are dead `evaluate_context` held the parsed syntax tree alive while the evaluator built the value tree. The command line also held the whole input file until the process exited. `Evaluator` has no lifetime parameter, so an evaluator can never borrow the tree, and the text is dead as soon as the context exists. 1.14x footprint (8.17 to 7.18 KB per element) at 500,000. Time is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`Name` held a `String`. `FeelContext` keys a `BTreeMap` by `Name`, so every context lookup ran a binary search whose every step was a `memcmp` over two heap strings. A profile of the benchmark model attributes 27.5% of main-thread work to `memcmp`, and 97.9% of that is key comparison rather than value comparison. Real key sets share long prefixes, so those comparisons run deep before they decide. `Name` is now a `u64` handle into a process-wide table. Comparison is an integer compare, and `Name::clone` no longer allocates. The key array of a `BTreeMap` node falls from 24 bytes per key to 8, which takes the leaf node from 984 bytes to 808. Ordering is preserved exactly, which is the part that needed care. `FeelContext` derefs to `BTreeMap<Name, Value>` and `jsonify` walks entries in key order, so `Ord` on `Name` is visible in program output. A handle therefore packs a rank in its high 40 bits and an identity in its low 24. Ranks are assigned so that rank order equals lexicographic order, and a rank once assigned never changes, which keeps already-built maps valid. `Ord` is one unsigned compare of the whole handle and is still lexicographic, so no consumer observes a different order. `as_str` resolves through a lock-free slot array rather than the table's lock. This is not a micro-optimization. Reading through an `RwLock` measured 26 times slower than no interning at all, on twelve threads. The same binary won by 4% on one thread. A read lock still writes the lock word, so every reader stole that cache line from every other core. Measured, minimum wall time of three and two repetitions, maximum peak RSS: model A, n=500,000 8.90 s / 3,592 MB -> 7.83 s / 3,200 MB -12.0% / -10.9% model D, n=10,000 40.75 s / 171 MB -> 29.02 s / 190 MB -28.8% / +11.3% The asymmetry is the evidence. The model D record carries seventeen keys with deep shared prefixes and gains 28.8%. The model A record carries eleven shorter keys and gains 12.0%. Model D peak RSS rises because the fixed cost of the table is visible against a 171 MB footprint and invisible against 3.2 GB. Limitations are deliberate. The table never shrinks, because leaking the text is what lets a handle be copied and `as_str` return `&'static str`. Name text comes from identifiers and context keys, so the set is bounded by input shape, not input size. If a rank gap is exhausted the name is left unranked and compares by text, which is slower and still correct. Past 2^24-1 distinct names interning panics rather than return a handle that compares equal to a different name. Gates, all against the previous build: `cargo test` 8510/37/25 with the same 37 environmental failures, in-repo TCK 3538/18/20, four `tdm` suites 27/27, 140 adversarial cases byte-identical, deterministic `bbt` 94/3 with all 97 verdicts identical by name, oracle equivalence 1000/1000 and 24/24, and full output byte- identical at n=10,000 on all four models. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RncmREziecjpii3CQNsyh2
…ure tree
`evaluate_context` parsed the input text into a syntax tree, built a tree of boxed
closures from that tree, then ran the closures to produce a value. An input data file is
not an expression. It is data: nested contexts and lists whose leaves are strings,
numbers, booleans and nulls. For such a tree the closure tree is a whole intermediate
representation built and thrown away for nothing.
`literal_value` reads an accepted tree straight into a `Value`. The test that accepts a
tree is purely syntactic: any `Name`, `Path`, `FunctionInvocation`, `At`, `For`, `Filter`
or `If` node anywhere in the tree rejects it, and the tree then takes the normal path
unchanged. Nothing in the module can panic on an unexpected node, because every function
returns `None` for a node it does not accept, so a gap in the accepted set costs a fast
path and never a wrong answer.
Four places had to stay identical and each one copies the matching builder rather than
reimplementing it: number text is reassembled and parsed by the same code as
`build_number`, a duplicated key yields the same null and the same message as
`build_context`, negation reproduces every arm of `build_neg`, and `At` is rejected
because it needs a temporal parse. `build_context` pushes a context onto the scope so
that a later entry can read an earlier one by name; skipping that push is safe only
because the test rejects `Name` and `Path`.
The tree is read and not consumed, and that is a measured choice rather than an
oversight. A consuming walk frees each record as soon as its value exists, which does
lower the live byte count. It also raised peak RSS by 413 MB, 12.9%, at n=500,000:
freeing the tree a record at a time while the value tree grows spreads mimalloc's
committed arena slices instead of releasing them in one block. mimalloc reported 7,400
fewer pages in use at the peak and 0.4 GiB more committed. Reading the tree and freeing
it whole keeps the allocation pattern the previous code had.
Measured, minimum wall time of three repetitions, maximum peak RSS:
model A, n=100,000 1.59 s / 732 MB -> 1.51 s / 724 MB -5.0% / -1.1%
model A, n=500,000 7.77 s / 3,200 MB -> 7.40 s / 3,200 MB -4.8% / 0.0%
model D, n=1,000 2.38 s / 35.0 MB -> 2.41 s / 34.7 MB +1.3% / -0.9%
model D, n=10,000 26.91 s / 190.5 MB -> 27.01 s / 190.1 MB +0.4% / -0.2%
The model D input holds `date and time("...")` in every record, so it is rejected and
gains nothing; the rejection costs nothing measurable because the test aborts inside the
first record.
Peak RSS does not move, and the reason is worth recording. Counting requested bytes, the
peak live total is 5,109 B per transaction on both paths, and both reach it inside
`parse_context`, where the lexer buffer, the parser stacks and the finished syntax tree
are live together. Everything the closure tree costs is spent below that mark. The lever
is an allocation-traffic lever, not a footprint lever: it removes 62 of 201 allocations
per transaction and 23% of allocated bytes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`Value` is paid by every value slot in the engine, and a `BTreeMap<Name, Value>` node holds eleven of them, so it is the dominant term in the footprint of every context. `FeelNumber` wraps a `BID128`, which is `#[repr(C, align(16))]`, so `Value` has align 16 and its size can only be 16, 32, 48 or 64. An align-8 payload starts at offset 8, so every such payload must be 40 bytes or less to keep the total at 48. After name interning took `(Name, FeelType)` to 40 bytes, exactly three variants were still above that line: `ExternalJavaFunction` and `ExternalPmmlFunction` at 48, two `String`s each, and `DateTime(FeelDateTime)` at 56. The two external function variants now hold `Box<str>` rather than `String`, which is 32 bytes for the pair. `Box<str>` forwards `Display` and `Debug` to `str`, so the text of every message that prints one is unchanged, and both consumers take `&str`, so deref coercion leaves the call sites alone. `DateTime` is boxed, which costs one allocation per date and time value. `size_of::<Value>()` is 48, verified against the real crates rather than a replica, and the eleven-entry context node falls from 808 bytes to 632. Measured, three repetitions interleaved so drift cancels, minimum wall time and maximum peak RSS: model A, n=100,000 1.47 s / 732 MB -> 1.45 s / 731 MB -1.4% / -0.2% model A, n=500,000 7.27 s / 3,200 MB -> 7.24 s / 3,170 MB -0.4% / -0.9% model D, n=1,000 2.41 s / 35.0 MB -> 2.45 s / 33.8 MB +1.7% / -3.4% model D, n=10,000 27.23 s / 190.3 MB -> 27.44 s / 159.9 MB +0.8% / -16.0% The asymmetry between the two models is the whole result and it is worth understanding. The model A input is 171 MB at n=500,000, and its peak RSS is reached inside the parser, where the lexer buffer, the parser stacks and the finished syntax tree are live together and no `Value` exists yet. Shrinking `Value` cannot move a watermark set before the first value is built, so that model gains 0.9%. The model D input is 5 MB, so its parse phase is negligible and its whole footprint is the contexts that rule evaluation derives. The node shrinks 21.8% and its peak RSS falls 16.0%. The +1.7% at n=1,000 on the model D is the cost of the boxed `DateTime`, and it is reproducible: six repetitions gave 2.38 to 2.43 before and 2.45 to 2.49 after. At n=10,000 the same cost is inside the run-to-run spread. That model holds one date and time per record and clones it repeatedly inside a quadratic rule, so it is the worst case for this trade, and it still pays 16% of its footprint for it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…are by `memcmp` `Ord for Name` compares two handles in one integer compare only when both carry a rank, and otherwise falls back to comparing the name text. Ranks are handed out between lexicographic neighbours and never renumbered, so a gap can be used up, and a name that finds no room is left unranked on purpose: it is still ordered exactly lexicographically, only more slowly. The gap was far too small. `RANK_STRIDE` was 2^16 and every insertion between two neighbours halved what was left, so fourteen names arriving in ascending order used a gap up. Rules named `Kb01`, `Kb02`, ... `Kb24` do exactly that. Ascending numbered identifiers are the ordinary case in a generated model, so this fired on every model to hand: model-a.dmn 7 unranked of 50 names Ka18..Ka24 model-b.dmn 7 unranked of 50 names model-c.dmn 22 unranked of 75 names model-d.dmn 27 unranked of 175 names Kb15..Kb24, s08..s24 Those names are rule and state identifiers, and they are context keys, so the fallback ran inside the hot `BTreeMap` search. Profiling `bin/dsntk-val48` on `model-d.dmn` at n=10,000 with `sample` at 1 ms put 1,249 samples on it, 6.9% of main-thread work: 594 in `memcmp` and 655 in `Name::as_str`, all reached through `btree::search::search_tree`, which calls nothing else. Three changes. `ID_BITS` goes from 24 to 20, trading a ceiling of 16,777,215 distinct names for 1,048,575 — the models here intern 50 to 175 — and buying four more bits of rank space. `RANK_STRIDE` goes from 2^16 to 2^26. And a name that continues a family of names now takes a fixed `RANK_STEP` away from its family neighbour rather than halving the gap, so a run of numbered names costs one step per name instead of using the gap up in `log2(gap)` names. A family is a shared prefix of at least two characters. Both halves of that rule were found by measurement, because two earlier versions starved a family from opposite directions: stepping away from whichever neighbour shares the longer prefix put the whole `Kb` family inside one step, because `Kb01` shares `P` with `Model D Assessment` and nothing with `InputFlags`; and always stepping up from the lower neighbour put `Model D Assessment` itself one step above `InputFlags`, with the same result. An unrelated name is halved into the middle, because that is what keeps both sides of a gap roomy for whatever arrives next. Misjudging a family costs later capacity and never an ordering, since every branch returns a rank strictly inside the gap. `RANK_STEP` was chosen by sweeping it against all four models and reading off the narrowest gap any name was inserted into, which is what says how much margin is left. The response is not monotone, because the constant changes which names become neighbours: 2^12 0 unranked narrowest gap 512 9 halvings left 2^14 0 unranked narrowest gap 2,048 11 halvings left <- chosen 2^16 0 unranked narrowest gap 2 1 halving left 2^18 6 unranked narrowest gap 1 0 2^20 7 unranked narrowest gap 1 0 All four models now intern every name with a rank. The residual cliff is not removed, only moved, and the module documents where it now sits: at least 131,072 appends, at least 4,096 names of one family per appended gap, and at least 15 halvings of a gap already too narrow to step through. No fixed rank space can remove the last one, because that needs renumbering, and the module now records why renumbering is unsound here — the rank lives inside the handle and handles have already been copied into live `BTreeMap`s that are still being searched, so renumbering the interner would leave every stored key holding a stale rank. Past every bound the behaviour is what it was: unranked, compares by text, still exactly lexicographic, so no output and no answer changes. Four new tests sort a name set through `Name` and through the raw text and require the two orders to be identical, over an ascending run, a descending run, repeated splitting of one gap, and a 1,203-name scrambled set that includes the empty name, a space and a non-ASCII name. Measured on a quiet machine, three repetitions with the binaries interleaved so drift cancels, minimum wall time and maximum peak RSS: model D, n=1,000 2.49 s / 34.3 MB -> 2.23 s / 33.7 MB -10.4% / -1.7% model D, n=10,000 30.86 s / 164.8 MB -> 27.98 s / 160.5 MB -9.3% / -2.6% model A, n=100,000 1.55 s / 713.1 MB -> 1.51 s / 732.1 MB -2.6% / +2.7% model A, n=500,000 7.59 s / 3,169.8 MB -> 7.61 s / 3,169.7 MB neutral The asymmetry is the evidence that the change did what was intended. The model D had 27 unranked names of 175, its records are seventeen keys wide, and its cost is dominated by context lookups, so it gains 9.3%. The model A had 7 of 50, its records are eleven keys wide, and its peak and much of its time are inside the parser, so it gains nothing outside the run-to-run spread. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RncmREziecjpii3CQNsyh2
`FeelContext::entries_mut` was 8.99% of main-thread work on the model D benchmark model, all of it reached through `set_entry`. Attributing those samples to the caller that is not itself a context method puts **97.5% of them on one line**. That line is the filter evaluator, which builds a context holding the single entry `item` for every element of every filtered list. The recorded diagnosis was that `Arc::make_mut` copies a seventeen-entry `BTreeMap`. It does not. The map it copies is **empty**, and copying an empty `BTreeMap` allocates no nodes. The cost is somewhere else: `FeelContext::default` clones the shared `EMPTY_ENTRIES` static, so the refcount is never one and `Arc::make_mut` therefore always takes its slow path. That path runs three atomic operations on **one** cache line that every thread in the process shares: an increment to clone the static, a compare-exchange inside `make_mut`, and a decrement to release the static again. It also allocates for the new `Arc`. With twelve Rayon workers all doing this per filtered element, the line is contended, which is why a copy that moves no bytes cost 8.99%. `FeelContext::from_entry` fills the map first and shares it afterwards, so it allocates once and touches nothing another thread can see. The filter evaluator and the parallel `for` worker, the two sites that build a one-entry context per element, now use it. Measured, four binaries interleaved **one repetition of each per round** rather than in blocks, so a load ramp cannot fall on whichever binary runs last. Minimum wall time and maximum peak RSS: model A, n=100,000 1.56 s -> 1.40 s -10.3% 731 MB, unchanged model A, n=500,000 7.75 s -> 7.00 s -9.7% 3,170 MB, unchanged model D, n=1,000 2.27 s -> 2.19 s -3.5% 34 MB, unchanged model D, n=10,000 31.37 s -> 29.86 s -4.8% 160 MB, unchanged Both models gain, which is what a contended cache line predicts. That distinguishes this lever from every one before it: the model A's peak is set inside the parser, so nothing that shrinks a value has ever moved it. This change removes work rather than bytes, and the model A filters as heavily as the other. Eighteen cases in `adversarial/` cover both filter paths — an element that already binds `item` and one that does not. They also cover a filter whose predicate is an index, a filter of a filter, `item` shadowed from an enclosing scope, a list that alternates the two paths, and two `for` expressions above the 512-element threshold that puts the body on the parallel path, which is the second call site. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RncmREziecjpii3CQNsyh2
`8d39fc0` boxed `Value::DateTime` to hold `size_of::<Value>()` at 48
bytes. Its own message recorded the price: "+1.7% at n=1,000 on the
model D is the cost of the boxed `DateTime`". A `Box` makes every
clone of the value allocate. An `Arc` holds the same 48 bytes and
makes the clone a refcount pair, so this keeps the footprint and
drops the allocation.
The reason it matters is that this payload is read far more often
than it is built. Instrumenting `FeelScope::get_value` to histogram
the variant it returns, on the model D model at n=1,000:
16,173,415 bare-name reads
60.75% Value::DateTime `orderDate`, 9,824,862 reads
29.61% Value::String `type` and `order_direction`
5.00% Value::Number
4.18% Value::List
0.19% Value::Context
That mix answers a question that was open: whether the clone
`get_value` performs is already cheap because `FeelContext` shares
its entries behind an `Arc`. On this model it is not, because a
context is 0.19% of what a bare name resolves to. On the model A it
is — 83.71% of bare-name reads there return a context — and that is
exactly where this change measures no gain.
Per-variant cost of `Value::clone` and the matching drop, 20,000,000 iterations each:
Boolean 4.23 ns
Number 3.91 ns
Context, 17 keys, Arc 4.51 ns
DateTime, Box 16.41 ns
DateTime, Arc 4.28 ns
String, 12 bytes 18.61 ns
`size_of::<Value>()` stays 48, verified against the real crates.
Ten arithmetic sites needed a change, all of the same shape. They
moved the payload out of the pointer with `*lh`, which a `Box`
allows and an `Arc` does not. Each now uses `Arc::unwrap_or_clone`,
which takes the payload when nothing else holds it and copies the 56
bytes when something does. Nothing mutates a `FeelDateTime` in
place, so sharing one between two values is unobservable.
Measured on top of `0e74274`, four binaries interleaved one
repetition of each per round. Minimum wall time and maximum peak
RSS:
model A, n=100,000 1.40 s -> 1.40 s 0.0% 732 MB, unchanged
model A, n=500,000 7.00 s -> 6.78 s -3.1% 3,170 MB, unchanged
model D, n=1,000 2.19 s -> 2.09 s -4.6% 34 MB, unchanged
model D, n=10,000 29.86 s -> 27.33 s -8.5% 164 MB, unchanged
Applied to `f0f3d27` **without** `0e74274` the same change measures
-1.1% at n=10,000, inside the run-to-run spread. The two are not
additive because both remove pressure from the same place. While
`Arc::make_mut` was serializing twelve workers on one cache line, a
cheaper clone on those workers bought nothing. That is the second
time in this effort that a lever's value depended on another lever
landing first. It is a reason to measure the combination rather than
only the increment.
Sixteen cases in `adversarial/` cover a shared payload: one binding
read twice and compared, a self-subtraction, both directions of date
minus date and time, each `unwrap_or_clone` site, a payload carrying
an IANA zone name, a shadowed date and time, a sort, and a list
filtered and then read back.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RncmREziecjpii3CQNsyh2
…ation `"item".into()`, `"partial".into()` and `"?".into()` intern their text on every call. Interning takes a read acquisition on the one process-wide `RwLock` that guards the name table. All three sit on paths that run from every Rayon worker. The filter evaluator and the `for` evaluator are both constructed inside the closure the builder returns, so they are built once per **evaluation**. The allowed-values test runs once per checked value. A `LazyLock` is an acquire load on a line that is read-only after first use. **Measured under 1%, which is inside the run-to-run spread, so this is hygiene and not a win.** Three repetitions, minimum wall time and total CPU: model A, n=100,000 1.30 s / 4.88 CPU-s -> 1.30 s / 4.85 CPU-s model D, n=10,000 22.65 s / 50.86 CPU-s -> 22.44 s / 51.23 CPU-s The reason it is small is that these are per-evaluation costs, not per-element: one acquisition amortizes over every element of the filtered list. It is also **not** the explanation for the Rayon join wait that `f0f3d27` left at 23.63% of the main thread. `0e74274` was: that wait is now 0.33%. Recorded so the hypothesis is not tried again. Output is byte-identical on all four models at n=10,000. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RncmREziecjpii3CQNsyh2
…able name
A `FEEL` name may contain spaces, so the lexer builds the longest run
of name parts it can. It then decides how much of that run is
really the name. It walks the candidates from longest to shortest
against the parse-time scope and commits to the first one the scope
knows. **When the scope knows none of them it committed to the
longest**, which absorbs a following operator into the name:
{c: {x: 1}, r: (c.noAddresses or true)}.r
-> null(build_path: no entry noAddresses or true in context: {x: 1})
The correct answer is `true`. `null or true` is `true` in
three-valued logic, and `noAddresses` is simply absent from the
record. Instead the member name became `noAddresses or true`, the
lookup missed, and the whole expression collapsed to `null`. `and`
behaves the same way, and so do `between`, `then`, `else`,
`satisfies`, `instance` and `return`.
This is a silent wrong answer rather than an error, and it hides in
a specific way. When the field **is** present, the candidate loop
resolves the short name, and the same expression is correct. So it
only shows up where a field is optional.
**In a model it does not even need the field to be absent.** A
parameter declared `typeRef="Any"` gives the parser no member
information at all. No candidate is ever known, so the longest is
always taken, whatever the runtime data holds. Two of the
twenty-four rules of the benchmark model guard a count threshold
this way:
if cust.noAddresses or count(outOfState) < 5 then [] else outOfState
`cust.[noAddresses or count](outOfState) < 5` invokes a null, yields
null, and `if null then [] else outOfState` takes the else branch.
The threshold never runs. Measured on a fixture holding three
out-of-state withdrawals against a threshold of five, where the
correct result is no marks:
noAddresses absent 3 marked -> 0 marked
noAddresses present 3 marked -> 0 marked
The fix stops the name at the first reserved word, and does it **as
the last resort**, after every existing branch has had its chance.
Ordering is the whole difficulty:
- Cutting before the built-in checks reduces `date and time` to `date`, because several
built-in names contain a reserved word themselves. That failed 544 TCK cases.
- `of` cannot be a cut point at all without consulting the built-in table. `day of week`,
`day of year`, `week of year`, `month of year` and `index of` all contain it. The cut
is now guarded by `Bif::from_str`, the same table the evaluator resolves a bare name
against, and `of` is left out of the list. `in` is left out too, because the existing
`till_in` branch already handles a `for` or quantified variable.
Cut points are enumerated rather than excluded: `and or between
instance then else satisfies return`. The cut cannot reach a name
the scope knows, a context key, a named parameter, a `for` or
quantified variable, a built-in type, a built-in date and time
function, or any other built-in function. Every one of those
returns earlier.
Verified individually: `date and time`, `day of week`, `index of`,
`years and months duration`, a key containing `or`, a key containing `and`, a multiword
member, a multiword formal parameter, a multiword `for` variable, `instance of`,
`if/then/else` and `some/satisfies` are all unchanged.
Gates: **in-repo TCK 3538 passing, 18 failing, unchanged to the case.** Full-output byte
comparison at 10,000 records over four models: unchanged. Ten full-chain scenario fixtures
verified against a chain-level oracle: unchanged, and still unchanged when `noAddresses` is
stripped from each. The bulk fixtures cannot see this defect, because they always supply
the field and their counts never fall under the threshold. That is why it survived every
gate in the repository.
`tests::expr::various::_0003` asserted `is_err()` on
Flights[ From = Original Flight.From and To = Original Flight.To and ... ][1]
parsed with an **empty** scope, commented "this test should fail
without properly set scope". That pinned this defect rather than a
requirement. The expression is the same shape as the bug, a path
member followed by `and`. So nothing can fix `c.noAddresses or true`
and still leave this unparseable. Narrowing the cut to a position
after a dot does not separate them either, because the absorption
here is itself after a dot. Grammar rule 25 does not admit a
reserved word inside a name, so refusing to absorb one moves toward
the specification. The test now asserts the parse succeeds, that
the member names stop at the reserved word, and that all four `and`
operators survive. The reasoning sits next to it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RncmREziecjpii3CQNsyh2
…once
Two rules of the benchmark model write their window test as a filter
whose predicate calls the same function several times, and every
call filters and folds the whole transaction list:
wins[inSum(item) >= 6000 and outSum(item) >= 6000 and outSum(item) > 0
and inSum(item) / outSum(item) >= 0.9 and inSum(item) / outSum(item) <= 1.1]
`inSum` three times and `outSum` four, for every window. Two
evaluations answer the same question, which is what the reference
implementation computes. `cse.rs` finds such subtrees when the
evaluator is built, and `build_filter_predicate` binds each distinct
one to a synthetic name once per predicate evaluation, in one extra
scope frame.
**Build-time rather than a runtime memo cache.** A memo has to key
on the argument values, so it has to hash a `Value`. Per-variant
measurements put a `String` clone at 18.61 ns and an eleven-element
`List` at 61.70 ns, with hashing in the same range, so a cache would
spend a large part of what it saves. These repeated calls are
syntactically identical subtrees evaluated in one unchanged scope,
so binding them needs no hashing at all.
Purity decides correctness here and nothing is assumed. Five rules,
each written as a list of what is **allowed** rather than what is
forbidden:
1. The callee is a plain name that is **not** a built-in. That one test excludes `now`,
`today` and everything else clock-derived, instead of relying on a list of things to avoid.
2. Every argument is a plain name or a literal, so evaluating the arguments changes nothing.
3. The callee is bound by a sibling entry of the enclosing context to a function definition
that is not external, so no `Java` or `PMML` implementation is reached.
4. That body calls only built-ins on an enumerated allow list, or other functions that pass
this same test, to a depth of eight.
5. Every occurrence sits on the predicate's non-binding spine. The walk refuses to enter
`for`, `some`, `every`, a context literal, a nested filter or a function definition, so no
occurrence is hoisted out of a construct that binds a name it reads.
`DSNTK_CSE_TRACE=1` prints what was hoisted. Every decision below
was read off that trace rather than asserted. The same trace is what
to reach for when the transform meets a rule nobody has written yet:
hoists two identical calls; two functions; a literal argument; a body reading a
path; one pure local function calling another
refuses `f(item)` beside `f(1)`, not the same subtree
`count(xs)` twice, built-in callee
an occurrence inside a `for`, off the spine
a body calling `now()`, built-in absent from the allow list
a callee that is a formal parameter, not a sibling entry
On the model it hoists exactly two subtrees in each of the two rules
and nothing anywhere else.
Measured round-robin on a quiet machine, one repetition of each binary per round, minimum wall
time and maximum peak RSS. Single-rule runs pin `InputFlags` to one gate key:
rule 12 alone, n=20,000 31.68 s -> 9.48 s -70.1% 3.34x
rule 15 alone, n=20,000 18.73 s -> 5.31 s -71.7% 3.53x
all 24 rules, n=20,000 52.21 s -> 30.16 s -42.2% 248 MB -> 233 MB
model A, n=500,000 6.40 s -> 6.41 s flat 3,170 MB unchanged
Seven invocations becoming two predicts 3.5x, and both per-rule
figures land within 6% of it. That agreement is the check that the
transform did what it claims, rather than that something else got
faster.
The ten full-chain scenario fixtures are the correctness gate that
matters, because these two rules mark nothing on the generated bulk
fixture. Both single-rule runs produce the same digest as each
other, so without a fixture where they fire the transform's effect
would be unobservable. All ten are byte-identical, with the two
target rules verified marking 6 and 10.
Gates: `cargo test --workspace` 8517 passing with the 37 failure
names unchanged. In-repo TCK 3538 / 18 / 20. 214 adversarial cases
byte-compared, 0 differing. The oracle at 1000/1000 and 24/24 per
rule. Deterministic `bbt` 94 / 3 with all 97 verdicts identical.
Full output unchanged at 10,000 records over four models.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RncmREziecjpii3CQNsyh2
…truct that rebinds it
This repairs a silent wrong answer introduced by `b5a59f1` earlier today, and that is the
important half of this commit. The performance work below is the smaller half.
Common subexpression elimination in a filter predicate used two walks that did not agree.
`cse::collect_calls` refuses to enter a construct that binds a name, so it never counts an
occurrence inside one. The replacement in `EvaluatorBuilder::build` matches a hoisted
subtree **structurally**, anywhere in the predicate, so it reached the occurrences the walk
had refused and made them read a value computed outside the construct.
{f: function(x) x * 10, L: [1,2,3],
r: (L[f(item) > 0 and f(item) < 1000 and sum(for item in [5] return f(item)) = 50])}.r
`f(item)` occurs twice on the spine, so it is hoisted. The third occurrence sits inside
`for item in [5]`, which rebinds `item`, and the rewrite turned it into a read of the outer
slot: `sum` returned 10 instead of 50 and the filter returned `[]` where the answer is
`[1, 2, 3]`. Six constructs that bind a name reproduce it, one expression shape each: a
nested `for` body, a `some` body, an `every` body, a context entry, a nested filter, and a
function definition body. All six return the correct answer on the binary built before CSE
landed.
**No model in this repository reaches the shape and no recorded measurement is affected.**
It needs three or more occurrences of one call with at least one of them inside a construct
that rebinds a name the call reads. It was reachable, not reached, so nothing measured
before this commit needs re-verifying.
**The existing gates could not have caught it, and that is why the new cases exist.** The
adversarial suite had sixteen cases for this pass and every one of them had either two
occurrences or a refusal, so none exercised three occurrences with one inside a binding
construct. A gate list is only as strong as the shapes in it. Group 20 adds fourteen cases
that pin the defect: one per binding construct, plus the positive cases below. All fourteen
are byte-identical to `dsntk-noglock`, which has no CSE at all, so the suite now has an
oracle for this pass rather than only a self-consistency check.
`EvaluatorBuilder::build_isolated` takes the slot table out of scope for exactly the
constructs the walk refuses, so finding and replacing refuse the same set. Enumerated
rather than excluded: a construct that is not listed keeps the slots and has to be argued
for.
The same discipline is what makes a repeated call in a `for` body safe to hoist, which is
the second half of this commit. A `for` body is evaluated once per iteration with the
iteration variables and `partial` bound, and `partial` is set once per iteration and does
not move while the body runs, so two occurrences inside one iteration share a value.
`build_for` now builds its body through the same path as a filter predicate. The
parallel-safety analysis still reads the body as written, so no `for` expression becomes
parallel that was not parallel before.
Two positions were added to the walk, and both are places the engine already evaluates in
the enclosing scope:
1. The positional argument list of a call.
`build_function_invocation_with_positional_parameters` evaluates every argument in the
caller's scope, eagerly, before the call happens. This is what reaches `dayGroup(d)`
inside `count(dayGroup(d))`.
2. The iterated expressions of a nested `for`, never its body. `build_for` calls every
iterated-list evaluator with the **enclosing** scope, once per `for` evaluation and
before any iteration variable exists. This is what reaches `dayGroup(d)` inside
`for g in dayGroup(d) return g.location`.
Named parameters are not entered, and neither are the iterated lists of `some` and `every`.
Nothing measured needs them, so they stay outside the enumerated set.
Verified per case with `DSNTK_CSE_TRACE=1` rather than assumed: three calls in a `for` body
hoist one subtree; the `MULTIPLE_LOCATIONS_SAME_DAY` shape hoists one; an occurrence in a
nested `for`'s iterated list hoists one; a callee whose body reads `partial` hoists one and
the answer does not change; a built-in callee refuses; and a callee whose body calls `now()`
refuses. On the model it hoists exactly one subtree, `dayGroup(d)` in `Kb10`, and nothing
anywhere else.
Measured at n=20,000, round-robin against the tree before this change, rule time net of the
fixed 0.40 s: `MULTIPLE_LOCATIONS_SAME_DAY` falls from 2.243 s to 1.139 s at an 1825-day
calendar and from 0.472 s to 0.254 s at 365 days. That is 1.97x where a hand rewrite of the
same rule measured 4.45x; the difference is not hoisting but parallelism, because removing
the invocation from the body is what lets `parallel::analyze` admit it onto twelve workers.
Gates, against the tree before this change: `cargo test --workspace --no-fail-fast`
8517/37/25 with the 37 failure names identical, in-repo TCK 3538/18/20, `tdm` 27/27 on four
models, the adversarial suite byte-identical on all 214 pre-existing cases,
`equivalence.py 1000` at 1000/1000 and 24/24, deterministic `bbt` 94/3 with all 97 verdicts
identical, all ten chain scenario fixtures byte-identical, and full-output md5 unchanged at
n=5,000 and n=20,000 across 30-, 365- and 1825-day calendars on both models.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… visiting every element
`L[i]` is an index, but a filter is the only machinery `FEEL` has
for one. `FilterExpressionEvaluator::evaluate` ran the predicate for
**every** element, pushed a one-entry `item` context for each, and
discarded the number it got back. Only afterwards did it evaluate
the predicate once more, to discover the result was an index.
Measured at 43 ns per element per index, with the constant confirmed
in two dimensions. The cost is linear in list length at fixed
operation count (exponent 0.94 over a 16x sweep), and linear in
operation count at fixed length (exponent 0.90). Both sweeps agree
on 35 to 47 ns. The consequence is that a prefix pass which indexes
inside its own loop performs D**2/2 indexes of a D-element list, and
is therefore **cubic** in the list length. A two-stage prefix sum
over 1,825 days measures 125.7 s, where the one-pass form measures
0.4 s. The control that isolates it is the identical loop with the
index removed, which costs 0.072 s where the indexing version costs
11.5 s at D=800.
The loop's final evaluation already decides index against filter,
and it does so with **no** element context pushed. When it yields a
number, the list the element loop built is discarded. So when every
per-element evaluation returns that same number, the loop
contributes nothing at all. The index taken is the same, and the
list built is empty and unused. That is the whole argument, and it
needs one premise -- that no per-element evaluation can differ from
the final one.
What can make a per-element evaluation differ is only what the loop
**binds**: `item`, bound for every element, and the element's own
entries. The element's entries are pushed when the element is a
`Value::Context`, which lets a bare name in the predicate resolve to
a field of the element. Both are name resolution. So `IndexShape`
classifies a predicate by **which names it reads**, not by what it
computes:
Closed reads no name, so neither `item` nor an element's fields can reach it. Needs
no check of the elements at all, so `fired[1]` is O(1) over a list of contexts.
PlainNames reads only plain names, none of them `item`. Sound once no element is a
context, which is one discriminant test per element, no allocation and no
scope push.
Unknown everything else, including any read of `item`. Unchanged behaviour.
Classifying by names read rather than by result type is the correct
invariant. The first version of this change got it wrong in a way
worth recording. It argued that an arithmetic root can only yield a
number or a null, so the loop's error arm was unreachable. **That is
false: `+` returns a string, a date, a date and time and both
durations as well as a number.** `[10,20,30][i + j]` with `i` a date
and `j` a duration yields a date, and the loop path answers that
with an error null. Cases `21k`, `21l`, `21n` and `21o` are the four
that would have failed. They now cover string concatenation, a date
result, and both of those arriving by an element's fields shadowing
the names. The rewritten gate says nothing about what the predicate
computes, so what `+` returns no longer matters.
**Nothing is skipped unless the evaluation returns a number.** Every
other result falls through to the element loop, which then behaves
exactly as it does today. That includes returning `only number or
boolean indexes are allowed in filters` when a predicate yields a
value a filter refuses. `index_shape` admits numeric literals, plain
names other than `item`, and the arithmetic operators only. So
nothing in an admitted predicate can invoke a function, evaluate a
nested filter, build a context, or write through the scope. A node
kind absent from the enumeration answers `Unknown`.
`index_into` is lifted out of `evaluate` unchanged, so the fast path
and the loop path share one copy of the index arithmetic. That
includes every message and the bare null an empty list returns. An
empty list is left to the loop path, which costs nothing there.
A boxed DMN `Filter` element carries an expression instance rather
than a `FEEL` syntax tree, so `build_filter_evaluator` passes
`IndexShape::Unknown` and is unchanged.
Measured at n=20,000, round-robin against the tree before this
change, rule time net of the fixed 0.40 s. `INCOMING_OR_OUTGOING_BALANCE_ANALYSIS`
falls 1.44x and `ATM_CASH_FUNNEL_PATTERN` 1.34x at an 1825-day
calendar, 1.15x and 1.13x at 365 days. The two-stage prefix sum above
falls 10.3x. It removes about a third of the calendar-length growth
those two rules retain, and not all of it. Their factor from a
30-day to an 1825-day calendar goes 7.8x to 5.4x and 6.8x to 5.6x,
where the rule that was never rewritten sits at 3.5x.
`UNEMPLOYMENT_OFFSHORE_EXIT`, rewritten by the same model commit,
does not move at all, so the rest of that residual is a different
cause.
The remaining cost is that a bare-name index still walks the list
once, at 3.9 ns per element instead of 43. So a pass built on
`pre[i]` stays cubic, with a ten times smaller constant. Only
`Closed` reaches O(1). Making `PlainNames` O(1) needs a way to know
that no element can shadow the name without looking at the elements,
and there is none.
Gates, against the tree before this change: `cargo test --workspace --no-fail-fast`
8517/37/25 with the 37 failure names identical, in-repo TCK 3538/18/20, `tdm` 27/27 on four
models, the adversarial suite byte-identical on all 214 pre-existing cases and on all 26 new
index cases, `equivalence.py 1000` at 1000/1000 and 24/24, deterministic `bbt` 94/3 with all
97 verdicts identical, all ten chain scenario fixtures byte-identical, and full-output md5
unchanged at n=5,000 and n=20,000 across 30-, 365- and 1825-day calendars on both models.
Model A at n=500,000 is flat with peak RSS unchanged at 3,022.9 MB.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…name
`0483067` stopped a name at the first reserved word but left `in` out of the list, on the
grounds that the existing `till_in` branch already handles it. That branch only trims the
**variable** name of a `for` or quantified expression, so it never sees the `in` of grammar
rule 49.c or 49.d, `expression in ( positive unary tests )`. The remainder of the defect was
live:
{f: function(t) t.type in ("a","b"), r: f({type:"a"})}.r
-> null(expected built-in function name or function definition,
actual is null(build_path: no entry type in in context: {type: "a"}))
The correct answer is `true`. The member name became `type in`, the following `(` then read
that absent member as the callee of a function invocation, and invoking a null yields null.
The same shape inside a filter is worse, because it has no diagnostic at all:
`count(X[type in ("a","b")])` answers **0**, silently.
Like the keyword case, it hides. When the parse-time scope knows the base name the candidate
loop resolves the short form and the expression is already correct, so `"a" in ("a","b")` and
a bare in-scope `y in ("a","b")` both work today. It appears where the scope cannot know the
member: a `typeRef="Any"` parameter, a path member of any `Any`-typed value, or a bare name
that does not resolve at parse time.
`docs/prebuilt-spec.md` in the harness repository recorded this as "the `in` operator is
unusable in literal expressions, use `list contains`". Same defect, same mechanism, retired.
Two hazards, both checked rather than argued.
- **`instance` is not cut.** The comparison is against a whole name part, not a substring.
`instance` is one part and the two strings differ, so `1 instance of number` and
`t.n instance of number` are byte-identical before and after.
- **No built-in name carries `in` as a part.** Enumerated from `bif.rs` and `bip.rs`. The
nearest are `index of`, `insert before`, `list contains`, `started by`, `finished by`, and
no built-in type name contains `in` either. So `in` has the hazard shape of `and`, not of
`of`: `of` must stay out of the list because `day of week` and `index of` carry it as a
part of their own name, which is why the cut is guarded by `Bif::from_str`.
Verified individually: `instance of` bare and after a path member, all three iteration forms
with `in` also used as a comparison in the body, a multi-word `for` variable, two iteration
contexts, a range iteration, `index of`, `insert before`, `list contains`, `date and time`,
`day of week`, `years and months duration`, and a name, a member and a context key that each
carry `in` as a real part.
One behavior change beyond the target, and it is the class `0483067` already accepted: a
multi-word formal parameter that contains a cut word no longer parses. `function(total in USD)`
now behaves as `function(a or b)`, `function(a and b)` and `function(a return b)` have since
`0483067`. A formal parameter with no type annotation misses the colon branch and its own name
is not yet in scope while it is being read, so it reaches the fallback. `function(a b)` and
`function(x: number)` are unaffected.
**`0483067`'s stated grounds were wrong. Its behavior stands.** That commit justified the cut
with "grammar rule 25 does not admit a reserved word inside a name". Clause 10.3.1.6 gives
`Profit and loss` as its own example of a name containing a keyword, next to `a-b` and
`what if?`, and resolves the ambiguity by matching name tokens against **the names in-scope**,
longest match preferred. So a reserved word inside a name is legal and the cut is not derived
from the grammar.
The premise came from the message authorizing that fix, not from the analysis in it, and this
correction is recorded here because a commit message in history cannot be edited. `STATE.md`
33.5 and 46.3 carry it forward, and the comment next to `_0003` is corrected in this commit.
What the cut is actually derived from: when no candidate matches the scope there is no longest
match to prefer, so the specification says nothing at all about that case. Taking the longest
lexical run was as much an invention as cutting is. The cut is the reading that keeps the
operator the author wrote, and it can only reach a name the scope does not know.
The same clause settles the related family that is **not** fixed here, and one part of it is
new. Rule 30 lists `. / - ’ + *` as additional name symbols, so `t.amt * 2` reads the member
as `amt*2` and **`t.a.b` reads it as `a.b`**: path nesting beyond one level is ambiguous in
this parser whenever the base's member set is unknown at parse time. No model here uses a
depth-three path, so it is latent.
Cutting at those symbols was built and measured, one at a time. `/` costs **nine TCK cases** --
`dmn_2_0110`, `dmn_2_0118`, `dmn_2_0119`, all three declaring an `itemComponent` named
`Approved/Declined`. `.`, `-`, `+` and `*` each cost nothing. That is not a licence to ship
them: it is a fact about the TCK's name corpus, `Approved-Declined` is exactly as legal, and no
gate here would notice. Two things settle it. **The affordable subset does not intersect the
risk** -- both at-risk sites in the benchmark model use `/`. And **a partial cut removes the
author's reason to be careful**: right for three operators and silently wrong for the fourth is
worse than none, because `t.amt * 2` working teaches the author that `t.amt / 2` works.
10.3.1.6 also rules out the whitespace heuristic by name and puts the remedy on the author:
parenthesize the name. `(t.amt) * 2`, `(t.a).b` and `wins[(item.inS) / (item.outS) >= 0.9]`
all answer correctly today.
Gates, run against the tree before it. In-repo TCK **3538 / 18 / 20** exactly, all eighteen
names `dmn_3_0076`. `cargo test --workspace --no-fail-fast` **8518 / 37 / 25**, which is the new
gate value: the +1 is `_0004` and nothing else, the lexer change alone measured
8517 / 37 / 25 before the test existed, and the 37 failure names are byte-identical across
both runs. Deterministic `bbt` **94 / 3** with all 97
verdicts diffed identical and no strays. `tdm` **27 / 27** on four models. Oracle
`equivalence.py 1000` at **1000 passed, 0 failed** and **24/24** per-rule. All ten full-chain
scenario fixtures byte-identical with `pb10fire` 3, `pb12fire` 6, `pb15fire` 10, `pb21fire` 1,
`pb23fire` 1 and every miss 0. Bulk output md5 identical in **12 of 12 cells** at n=5,000 and
n=20,000 across 30-, 365- and 1825-day calendars on both models, with model D n=20,000
span 365 reproducing `7aabd3b5…`. Adversarial suite **302 cases**: the 254 that predate this
change are byte-identical, and the 48 new ones differ from a binary without the cut in exactly
the seven cases that are the fix's targets.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RncmREziecjpii3CQNsyh2
…lement `string join` used the list index to decide where to put the delimiter. A list that starts with a null therefore got a leading delimiter, so `string join([null,"b"], "X")` returned "Xb". A null in the middle of the list was already ignored. The function contradicted itself. It now tracks whether it has joined a string yet. `context merge` skipped a null entry and reported nothing. A number in the same position returns a diagnostic null. A null is also the entry that a failed expression produces, so it is the case that most needs a diagnostic. A null entry is now out of domain, like any other non-context. This adds 8 tests. The existing tests cover a null in the middle and at the end of a join. Both already passed. No test covered a leading null. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RncmREziecjpii3CQNsyh2
The gate decides whether a `for` body can read the implicit
`partial`, and it did so by reading the body as written plus one
check of the scope at loop entry. Neither can see a binding that
arrives later, from data. Two ways in:
**A filter predicate that invokes.** Evaluating a filter pushes
the filtered element's own entries onto the live scope, so when
an element is a context its keys become names the predicate can
call. An element such as `{sum. Function(x) count(partial), v:
1}` binds `sum` to a function reading `partial`, and a predicate
calling `sum(item)` passes the built-in check.
**A loop variable that spells a built-in.** The loop variable
binds a data value to a name the model author chose. `analyze`
only ever sees the body, so the loop variables never reach the
bound-name check, and at loop entry the variable is not bound
yet.
{ data: [{sum: function(x) count(partial), v: 1}], big: (for k in 1..600 return k),
r: (sum(for i in big return count(data[sum(item) > 0]))) }.r
parallel -> 600 serial -> 599
{f: function(z) count(partial), fs: for i in 1..600 return f,
r: for sum in fs return sum(1), out: {a: r[1], b: r[600]}}.out
parallel -> {a: 1, b: 1} serial -> {a: 0, b: 599}
Serial is correct in both. Divergence starts at exactly
PARALLEL_MIN_ELEMENTS — below it the two paths agree and the bug
is invisible.
Rule 5 refuses a body containing a filter whose predicate invokes
anything. Rule 6 refuses a body whose loop variable is also an
invoked callee, and it has to live in `build_for`, because that
is the only place the loop variables are known.
Rule 6's intersection is exact rather than conservative. Rule 3
already forces every callee to be a plain name, so a loop-bound
value can only be invoked through a callee name equal to a
loop-variable name.
Adds three tests, including a control with the loop variable
renamed so it stays serial through rule 3. The control matters —
it fails a fix that works by disabling parallelism everywhere.
Neither model has such a body, and neither names a loop variable
after a built-in, so the measured wins survive. Model A at
n=100,000 is 1.29 s either way.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RncmREziecjpii3CQNsyh2
Names intern to an integer handle, and `fn context` interns every string key — including keys built from data. The models' `idmap` construct spends one name per marked transaction, per marking rule, and never gets it back. At n=500,000 the shipped workload sat at 48% of the 1,048,575 budget. **The table was a hard `panic!` at that limit.** It is also per-process and never shrinks, so `dsntk srv` died after roughly 86 requests at the 4 MB body limit. Names past the rankable space now take a top-bit-flagged 63-bit identity and compare by text. That is the same fallback an exhausted rank gap already used, so ordering stays exact. The rank append budget halves and still leaves about 500x margin. Verified at 1.1M and 2.2M ids by a test that dies before and passes after. **`get value` fed the table on every probe, including misses.** It built its key with `Name::from`, so looking up an id that was absent still added a name. The models probe `get value(idmap, string(t.order_id))` once per transaction per rule, so a miss was the common case. Fixing the insert path alone would not have stopped the growth. `Name::existing` returns a handle only when the text is already interned. Reading `None` as "no context holds this key" is safe by construction, not by test. The tuple field of `Name` is private to this module, and every constructor goes through `intern` or `intern_new`. So a `Name` cannot exist without sitting in `handles`. Text absent from `handles` names no key in any context. A caller that interned it would have received a fresh handle that is by construction in no map. The hot path does not change shape — `existing` performs the same read-lock probe that `intern` already does first. Measured: 400,000 probes for absent keys give byte-identical output and take peak RSS from 141 MB to 115 MB. This does not close the leak. Keys that are actually **stored** are still interned, by `context()` and `context put`, so a long-lived process can still grow on distinct stored keys. That needs a two-variant key type in `FeelContext` and is separate work. An error there returns a wrong answer where today's behaviour returns a diagnostic. Adds 7 tests across the overflow path, the `existing` contract and `get value`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RncmREziecjpii3CQNsyh2
… it as a name DMN 1.2 onward defines `?` as the placeholder for the tested value in a generalized unary test. Grammar rule 28 makes `?` a legal name start character, so it parsed as a name, resolved to nothing, and the test was simply not true. `contains(?, "VENMO")` never matched. Inside a decision table that reads as an unmatched rule with no diagnostic anywhere. Two corrections to the analysis that prompted this. Binding `?` to the tested value is not sufficient. Per DMN 1.3 Table 55 a boolean test that uses `?` **is** the result, rather than a value to compare against the input. And `item_definition.rs` did not already handle this correctly — an `allowedValues` of `contains(?, "VENMO")` rejected matching and non-matching values alike. So this adds a shared unary-test builder, used by both the decision table and `allowedValues`. An entry that does not mention `?` builds the byte-identical node it built before, so no existing model changes behaviour. A scan of the corpus finds no `?` in any input entry or `allowedValues`. Rule 28 governs the start of a name, not the continuation list of rule 30, and the lexer separately asserts `?` is not a continuation character. So `?` cannot swallow a following token and the lexing risk is zero, unlike the arithmetic absorption in defect 20. Adds 5 tests. Three fail before and pass after, plus a negative control that passes throughout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RncmREziecjpii3CQNsyh2
The cache took a `RwLock::read` on every call. Under the parallel `for` path, that contention made parallel evaluation slower than serial. A body using `matches` measured 0.94 to 0.95x of serial, where an identical body using `contains` measured 1.21 to 1.28x. Upstream marked the lock with its own TODO. The cache is now thread local, so the hit path touches no shared state and each worker owns its compiled `Regex`. That also avoids contention inside the regex crate's own pool. Measured interleaved, 2 by 6 reps: the parallel `matches` body goes to 1.26 to 1.27x, matching the `contains` control. About 25% of wall time comes back on the probe, well clear of the 1 to 6% spread seen on this machine. Serial is unchanged. A unit test cannot observe lock contention, so the evidence here is the measurement. The added case guards parallel correctness instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RncmREziecjpii3CQNsyh2
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
1 participant
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.
Tested with real sizes
Every number below can be reproduced from this branch with no data files and no new dependency.
cargo benchruns many iterations, so a quadratic cost makes large sizes unusable. The same commit adds an example that runs one pass at any size, and the system timer reports peak memory:mainGrowth for 4x items:
main17.4x, this branch 1.3x. This branch reaches 1,000,000 items in 0.162 s and 56 MB.List parsing gains time and not memory. At 100,000 items it goes from 6.299 s to 0.023 s, with peak RSS 55 MB against 52 MB.
Two causes, both found by profiling.
ForExpressionEvaluator::evaluatecopied the implicitpartiallist twice per iteration, and it did so whether or not the body ever readpartial.action_list_tailcollected list items withVec::insert(0, ..)on a right-recursive production, so every item moved all the items already collected.Reproducing this
The first commit adds the benchmarks and nothing else.
Correctness fixes, independent of performance
Each one returned a wrong value with no diagnostic, so only a differential test finds them.
string joinput a delimiter in front of a leading null.string join([null,"b"], "X")returned"Xb". The code counted the list index, so it ignored a null in the middle and honored one at the front. It contradicted itself.context mergedropped a null entry and reported nothing, where a number in the same position already returned a diagnostic null.?in a unary test did not bind. Grammar rule 28 makes?a name start character, so it parsed as a name and the test was never true. Per DMN 1.3 Table 55 a boolean test that uses?is the result.fn contextinterns every string key, so a process that builds contexts from data reached the limit and panicked.get valueinterned its key on every probe, including probes that missed, so a lookup fed a table that never releases a name.memcmp.in.Optional parallel
forA body that cannot read
partialmay run in parallel. Deciding that safely needs six rules. Adversarial review found two holes after the first version shipped. A filter predicate can call a name pushed from the filtered element's own entries. A loop variable can spell a built-in. Both made the parallel path disagree with the serial path above 512 elements, andthe parallel path was the wrong one.
parallel.rsdocuments all six rules and why each one exists.Existing test failures
The 36 failure names are identical. Every failure already exists on
main. This branch adds 33 passing tests and breaks none.What this does not fix
. - + * /legal name characters. Cutting on/loses nine TCK cases, so a partial cut would teach an author that the unsafe form is safe.dsntkexits 0 when a model file or an input file is missing. This branch does not change that, but it is a silent failure and deserves its own issue.