Skip to content

Short-circuit And/Or in generated code #1514

Description

@alexzautke

Problem

Cql.Compiler flattens every boolean expression into straight-line SSA locals, so both operands
of and/or are always evaluated
before the operator is called. From a generated HEDIS AIS-E
library:

private bool? Denominator_1_Compute(CqlContext context)
{
    bool? a_ = this.Initial_population_1(context);
    bool? b_ = this.Exclusion(context);              // evaluated even when a_ is already false
    bool? c_ = context.Operators.Not(b_);
    bool? d_ = context.Operators.And(a_, c_);
    return d_;
}

When a_ is false the result is false regardless of c_, so evaluating Exclusion — a
definition costing ~10% of this measure's evaluation time — contributed nothing.

How often this happens

Census over the NCQA AIS-E measure, 240 patient bundles, memoization enabled:

per patient
And calls 69.2
...with left == false (right operand redundant) 57.8 (83.5%)
redundant right operands across And + Or ~80

Why skipping is value-preserving

CQL's three-valued logic collapses these cases regardless of the right operand:

left right and left right or
false true / false / null false true true / false / null true
null false false null true true
null true / null null null false / null null

The condition is exactly left == false for and and left == true for or. left == null
does not permit skipping, because null and false = false still needs the right operand. An
implementation shortcut such as if (left != true) return left; would be a correctness bug — worth
calling out explicitly for whoever picks this up.

Why the existing Lazy overloads are not the fix

ICqlOperators already declares short-circuiting overloads, and they are correct
(Cql/Cql.Runtime/Operators/CqlOperators.LogicalOperators.cs):

public bool? And(bool? left, Lazy<bool?> right)
{
    if (left == false || right.Value == false)   // right.Value untouched when left is false
        return false;
    ...
}

The code generator simply never emits them. But wrapping an operand as Lazy<bool?> costs a Lazy
object plus a capturing closure plus a delegate — roughly 150 bytes per operand, or ~10 KB/patient
at the call volume above. That trades evaluation time for allocation, and allocation is the
constraint that matters most here: at ~0.84 MB/patient, gen0 collections are the serial fraction
that caps parallel evaluation throughput (one collection per ~14 patients; pause cost invariant with
thread count). A Func<bool?> overload is cheaper than Lazy but still allocates a delegate and a
closure.

Proposed change

Have the code generator emit branching control flow instead of flat SSA for and/or, so the
short-circuit needs no runtime API and allocates nothing:

bool? a_ = this.Initial_population_1(context);
bool? d_;
if (a_ == false)
{
    d_ = false;
}
else
{
    bool? b_ = this.Exclusion(context);
    bool? c_ = context.Operators.Not(b_);
    d_ = context.Operators.And(a_, c_);
}
return d_;

Applies symmetrically to or with left == true. Only the right operand's evaluation moves; the
operator call itself is unchanged, so three-valued semantics stay in one place.

Trade-off: the generated code becomes less uniform to read, and Cql.Compiler has to emit
statements rather than a flat expression chain for these nodes.

Measured value

Interleaved A/B over the AIS-E deck (200+ patients, 20 discarded as warmup, memoization on,
evaluation timed only):

  • Short-circuit alone: 0.909 evaluate time (band 0.693–1.053)

Note the saving is smaller than the 83.5% census implies, because a skipped definition still gets
computed if another definition references it — it moves rather than disappears. The ratios above are
end-to-end and already account for that.

The decision that gates this

CQL expressions are pure except for Message() and runtime errors, so skipping the right
operand is observable in three ways:

  1. An error thrown by the skipped operand no longer surfaces.
  2. Message() in the skipped operand no longer fires, so MessageReceived subscribers see fewer
    events.
  3. Which definitions end up memoized shifts (benign for results).

The value of every expression is unchanged — that is guaranteed by the table above. But (1) and (2)
are real behaviour changes that need a Changed release note, and arguably a decision before the
work is scoped: the CQL specification permits short-circuiting and does not mandate evaluation order,
and this repository already ships the Lazy overloads, so the semantics have been accepted in
principle — but they have never actually been reachable from generated code.

If short-circuiting is not acceptable, this issue should be closed rather than implemented — the
implementation choice only matters once that question is settled. This applies equally to the
existing Lazy overloads, which have the identical observable difference.

Acceptance criteria

  • Generated code short-circuits and on left == false and or on left == true, and not
    on left == null.
  • A regression test pins the full three-valued truth table for both operators, including every
    null combination, through generated code rather than through ICqlOperators directly.
  • Output-equivalence check over an existing library set: results byte-identical before and after
    for the same inputs.
  • No new allocation on the short-circuited path (assert with an allocation probe, not by
    inspection).
  • Release note under Changed, disclosing error/Message() suppression.
  • Decide whether the now-unreachable Lazy<bool?> overloads stay (they remain public API).

Provenance

Found while profiling HEDIS AIS-E evaluation against a 15,168-bundle NCQA patient deck. All figures
above are from interleaved in-process A/B measurement on a 4-physical-core machine; the wall-clock
bands are wide because that box is noisy, while the census and allocation figures are deterministic.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions