diff --git a/CHANGELOG.md b/CHANGELOG.md index f8f6ced1..e55f0485 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,41 @@ API surface). ### Changed +- **`Quantity::equation()` writes out at most 100 derivation steps by default, + and takes the limit as an argument.** A derivation has no bound — a + data-driven `total = total + row` loop records one step per iteration — and + `equation()` used to render every one of them: 100,000 steps produced a + **500,001-character** first line in **58.8 s** (clang 22.1.8, `-O1` under + ASan+UBSan). It now produces **502 characters in 0.057 s**, with the deep end + of the derivation collapsed to an `e1` whose value carries a legend line: + `e1 = 99900 (elided at the 100-step limit)`. The result line is unaffected — + eliding changes the account of how a value was reached, never the value. + + **Public signature change**: + `equation(std::size_t maxSteps = kDefaultEquationSteps)`. Source-compatible + (every existing call site compiles and keeps working), but the *output* of an + existing call changes once a derivation passes 100 steps. Callers that want + the old rendering pass `kEquationStepsUnlimited`; `0` returns the formatted + value alone. The limit is a parameter rather than a constant because how much + of a derivation is worth reading is the reading layer's decision, not the + value type's — an audit log and a tooltip do not want the same answer. The + default sits at the human end of the range because the two errors are not + symmetric: too low costs a caller one argument, too high costs everyone a + half-megabyte line they never knew was unbounded. Pre-1.0, per + `docs/spec/VERSIONING.md`. See `docs/spec/util/quantity_type.md`, "The + rendering is bounded; the derivation is not", and morph#582. + + Rendering a derivation **in full** also got cheap, which is a separate half of + the same issue: `combine` appends to its left operand instead of copying it + into a fresh string, so the left-leaning chain an accumulate loop records is + linear rather than quadratic in its depth — 70,000 steps rendered whole went + from 27.7 s to 0.11 s under ASan+UBSan, and from over ctest's 120 s timeout to + 1.3 s at `-O0` under TSan. The `[slow]` tag and 600 s timeout exception added + for that test in morph#590 are gone again; every test is back under one 120 s + cap. A right-leaning chain and a chain of unary negations still copy the big + operand per level and are still quadratic — the step limit is what bounds + those. + - **`Completion` has a stated value-handling contract, and `T` no longer has to be copyable.** `std::move_constructible` is now the whole type requirement; copyability became a *per-handler* obligation, diagnosed where @@ -206,6 +241,14 @@ API surface). ### Fixed +- **`equation()` no longer walks a shared derivation once per path.** + `EquationRenderer::assignLabels` was the one traversal without a visited set, + so a node reachable by *k* displayed paths was walked *k* times. Since the + derivation is a DAG, that is exponential in the node count: 31 nodes built by + repeated `q = q + q` have 2³⁰ root-to-leaf paths and took **10.3 s** to render + 33 short lines. With the set, the same call is instant and its output is + byte-identical, and 61 nodes (2⁶⁰ paths) render instantly too. morph#602. + - **A model registered privately over `morph::net` was not journalled at all.** `morph::net::SocketBackend` left `IBackend::registerModelWithContext` unoverridden, so its default dropped the `contextKey`, and the native diff --git a/docs/spec/util/quantity_type.md b/docs/spec/util/quantity_type.md index d0db6bae..9e5e587b 100644 --- a/docs/spec/util/quantity_type.md +++ b/docs/spec/util/quantity_type.md @@ -461,7 +461,16 @@ the stack, so none of the walks is recursive: stack. The two renderings share one stack machine (`EquationRenderer::render`, selected by `RenderMode`) whose frames resume at the same three points a recursive call would: render-or-descend-left, take-left-descend-right, - combine. + combine. The step limit below does **not** replace this: it bounds what gets + rendered, and the reference count still walks every node at any limit, so the + walks must survive a depth no limit constrains. A caller can also ask for the + whole derivation (`kEquationStepsUnlimited`), which walks all of it. + + The labelling walk carries a **visited set**, like the reference count. Both + are walks of a DAG rather than a tree, and without one a node reachable by + several paths is walked once per *path*: a derivation of 31 nodes built by + repeated `q = q + q` has 2³⁰ root-to-leaf paths and took **10.3 s** to render + 33 short lines before the set was added, 0.000 s after (morph#602). Measured against the recursive code (morph#574, 8 MiB stack, clang 22.1.8 and gcc 16.2.1): @@ -519,12 +528,108 @@ so a caller emits them verbatim), in this fixed order: value used exactly once never reaches the legend — it is inlined at its one point of use instead. +### The rendering is bounded; the derivation is not + +Depth is unbounded (see *Provenance*), and for a while `equation()` rendered +whatever depth it was handed: a 100,000-iteration running total produced four +lines whose first was **500,001 characters** of `0 + c1 + c1 + …`, built in +58.8 s (morph#582, clang 22.1.8, `-O1` under ASan+UBSan). That is not an +explanation of anything, and a caller printing it emits a single half-megabyte +line. So `equation()` takes a **step limit**: + +```cpp +std::vector equation(std::size_t maxSteps = kDefaultEquationSteps) const; +``` + +**A step is one operation node written out.** Atoms — leaves, conversions — and +named nodes render as a single token and cost no steps; only a node whose +operation is spelled out does. The limit is spent **once per `equation()` +call**, across the formula and the legend together, so a derivation that reaches +the limit inside a `where` line has reached it for the whole call. + +**Past the limit a sub-derivation is *elided*.** It renders as `eK` — `e1`, +`e2`, …, numbered in order of first appearance, exactly as `cN` placeholders +are — and it earns a legend line of its own: + +``` +e1 + c1 + c1 + c1 + … + c1 + = 69900 + 1 + 1 + 1 + … + 1 + = 70000 +where c1 = 1 + e1 = 69900 (elided at the 100-step limit) +``` + +The pieces of that contract, exactly: + +- In the **formula** (`[0]`) an elided node is the token `eK`, an atom for + parenthesisation purposes. +- In the **substitution** (`[1]`) it is its **value**, like any other label. +- In the **legend** it is `eK = (elided at the -step limit)`, + after the `cN` lines and sharing the same `where ` / indent alignment. The + parenthetical is on every such line, and names the limit, so that a caller + meeting an `eK` for the first time can tell it from an ordinary value without + consulting this document. +- The **result** (`[2]`) is unaffected. Eliding changes the account of how a + value was reached, never the value. +- **Elision beats a placeholder.** A node that is both reused and past the limit + gets an `eK`, not a `cN`: a `cN`'s legend line expands the very subtree the + limit just declined to render. +- The walk is **not** what the limit bounds. Reference counting still visits + every node in the DAG, because reuse is a property of the derivation and not + of how much of it gets printed — a value shown once must not be given a + placeholder just because the limit hid its other uses. That walk is linear and + iterative; the limit bounds the *rendering*. + +**Where the cut falls is a choice.** The walk is a left-before-right pre-order +from the root, so the steps that survive are the ones **nearest the result** and +what collapses is the deep end. On the running-total shape that is the right +way round: the last *N* additions stay legible and the accumulated history +folds into one number. + +**The default is 100 steps, and it is a readability bound, not a cost bound.** +Two things decide it: + +- *A rendered formula stops being an explanation long before it stops being + affordable.* morph#574's phrasing — "an explanation 200,000 steps deep is not + an explanation" — is the defect; 100 written-out steps is already more than a + person reads, and it is two orders of magnitude above any derivation this + repository's own examples build. Setting the default where the *cost* becomes + intolerable instead (thousands of steps) would keep producing output nobody + can use. +- *The two errors are not symmetric.* Because the limit is a parameter, a + default set too low costs a caller one argument. A default set too high costs + everyone the half-megabyte line, and they cannot opt out of what they never + knew was unbounded. So the default sits at the human end of the range. + +**The limit is the caller's, not the type's.** `Quantity` reports its +derivation; how much of one is worth reading is a decision of the layer doing +the reading — an audit log and a tooltip do not want the same answer, and this +type cannot know which it is talking to. Hence a parameter with a default rather +than a fixed constant. Two named values sit at the ends of its range: + +| Argument | Meaning | +|---|---| +| `kDefaultEquationSteps` (100) | The default. | +| `kEquationStepsUnlimited` | Write the derivation out in full — the pre-morph#582 behaviour, unbounded in output size, with the cost taken deliberately. The depth regression tests use it, since they exist to walk deeper than any limit would render. | +| `0` | Write no step out: the one-element formatted value, the same answer a build with tracing compiled out gives. | + +**Rendering a derivation in full is affordable but not linear in every shape.** +`combine` appends to its left operand instead of concatenating both sides into a +fresh string, which makes a **left**-leaning chain — the shape +`total = total + row` records — linear in the depth: rendering 70,000 steps in +full costs 0.11 s rather than 27.7 s (same build as the measurement above). +A **right**-leaning chain (`a + (b + (c + …))`) and a chain of unary negations +still copy the big operand once per level and are still quadratic; the step +limit is what bounds those, not the append. + **Degenerate roots return a single element.** When the value has no expandable derivation, `equation()` returns a one-element vector (formula only, no substitution/result/legend lines): - an **empty** quantity, or any quantity with tracing compiled out → the formatted value alone (the `N/A` text when empty); +- any quantity rendered with **`maxSteps == 0`** → the formatted value alone, + the same answer tracing-off gives, since no step may be shown; - a **bare unnamed leaf** (a raw input never operated on) → its formatted value; - an **engaged value with no recorded derivation node** — one materialised directly (the wire codec writing `payload`, or direct assignment to the public @@ -565,6 +670,7 @@ placeholder**: | **Unnamed conversion**, reused | placeholder `cN` | its value | `cN = ` | | **Unnamed computed**, used once | its inlined expression (`"a" - "b"`) | the substituted expression | — (inlined) | | **Unnamed computed**, reused | placeholder `cN` | its value | `cN = = = ` | +| **Past the step limit** (any unnamed computed node) | elision `eN` | its value | `eN = (elided at the -step limit)` | A value used exactly once is inlined at its one point of use and never reaches the legend; only a *reused* value earns a `cN` placeholder, so shared work is @@ -935,7 +1041,7 @@ readability). | `roundedToDecimalPlaces(p, mode)` | `Quantity roundedToDecimalPlaces(DecimalPlaces, math::RoundingMode = HalfAwayFromZero) const` | **Rounds the exact value** to `p` decimals and tags it there (`math::roundToDecimalPlaces`); no-op on empty. Precision silently clamped; saturates and logs if the scale-up leaves `int64`. | | `atDeclaredPrecision()` | `Quantity atDeclaredPrecision() const` | `roundedToDecimalPlaces(declaredPrecision())` — **rounds** the value to the field's declared precision, the one the schema advertises as `x-decimalPlaces`; no-op on empty. | | `named(label)` | `Quantity named(std::string label) const` | Returns a same-unit quantity marked as the symbol `label`; builds a fresh history node (no-op returning empty on empty, or with tracing off). | -| `equation()` | `std::vector equation() const` | The worked formula as print-ready lines (see *Provenance*). Single-element (the formatted value) when empty or tracing off. | +| `equation(maxSteps)` | `std::vector equation(std::size_t = kDefaultEquationSteps) const` | The worked formula as print-ready lines (see *Provenance*). Writes at most `maxSteps` derivation steps; past that a sub-derivation renders as `eK` with its value in the legend. Single-element (the formatted value) when empty, tracing off, or `maxSteps == 0`. | | `operator Quantity()` | `operator Quantity() const` | Implicit same-dimension conversion; delegates to `convert`, propagates empty, records a provenance step. | ### Free functions and operators (namespace scope) @@ -949,6 +1055,8 @@ readability). | `operator*`, `operator/` (scalar) | `Quantity op(Quantity, Rational)` | Scale/divide by a dimensionless `Rational`; unit and declared precision unchanged. | | `operator==` | `bool operator==(Quantity, Quantity)` | **Total**; empty==empty is `true`. | | `operator<=>` (`<`,`<=`,`>`,`>=`) | `std::strong_ordering operator<=>(Quantity, Quantity)` | Ordering; **throws `std::logic_error`** if either operand is empty. | +| `kDefaultEquationSteps` | `inline constexpr std::size_t = 100` | `equation()`'s default step limit — see *The rendering is bounded*. | +| `kEquationStepsUnlimited` | `inline constexpr std::size_t = SIZE_MAX` | Pass to `equation()` to write the derivation out in full. | All arithmetic and conversion **propagate empty**; division by a non-empty zero yields empty. diff --git a/include/morph/detail/quantity_equation.hpp b/include/morph/detail/quantity_equation.hpp index 7d6445bd..7806a072 100644 --- a/include/morph/detail/quantity_equation.hpp +++ b/include/morph/detail/quantity_equation.hpp @@ -111,6 +111,16 @@ struct LabelFrame { /// @brief Stateful renderer for one `equation()` call. struct EquationRenderer { + /// @brief Builds a renderer with a step limit. + /// @param steps How many derivation steps may be written out in total. + explicit EquationRenderer(std::size_t steps) : maxSteps(steps), stepBudget(steps) {} + + /// @brief The limit this render was asked for (quoted in the legend). + std::size_t maxSteps; + + /// @brief How many steps `assignLabels` may still spend expanding. + std::size_t stepBudget; + /// @brief Placeholder number per reused, unnamed node (1-based). std::unordered_map labelIndex; @@ -123,6 +133,17 @@ struct EquationRenderer { /// @brief Reused-node placeholders, in first-appearance order. std::vector placeholderOrder; + /// @brief Nodes `assignLabels` has already settled (label, elision, or + /// neither). A node's second visit can only repeat the first one's + /// verdict, so it is skipped. + std::unordered_set settled; + + /// @brief Elision number per node the step limit cut (1-based). + std::unordered_map elisionIndex; + + /// @brief Elided nodes, in first-appearance order. + std::vector elisionOrder; + /// @brief Whether a node earns a placeholder (reused). Only ever called on /// unnamed nodes recorded by `countRefs` (callers return early on /// named nodes), so the lookup is total. @@ -165,12 +186,27 @@ struct EquationRenderer { } } - /// @brief Assigns placeholder labels in first-appearance order. + /// @brief Assigns placeholder labels, and decides where the step limit cuts. /// /// Iterative for the same reason as `countRefs`. First appearance is a /// left-before-right pre-order, so the right child is pushed first and the /// left one popped first — the order a recursive walk would have visited /// them in. + /// + /// This pass is also where `maxSteps` is spent, and it is spent **once** + /// for the whole `equation()` call rather than per rendering. The two + /// renderings and every legend line then consult the one `elisionIndex` + /// they all share, so the formula, the substitution and the legend agree + /// on which sub-derivations were written out — which a per-rendering + /// budget could not guarantee, since the legend renders subtrees the + /// formula stops short of. + /// + /// A step is one **operation node expanded**: atoms (leaves, conversions) + /// and named nodes render as a single token and cost nothing. Past the + /// budget a node is *elided* — it takes an `eK` label and its children are + /// not walked — and elision wins over a `cK` placeholder, because a + /// placeholder's legend line would expand the very subtree the limit just + /// declined to render. /// @param root The node to start from. /// @param expandRoot Whether to expand @p root itself (rather than label it). void assignLabels(const ASTNode* root, bool expandRoot) { @@ -183,32 +219,76 @@ struct EquationRenderer { if (node == nullptr || node->name.has_value()) { continue; } - if (!frame.expandThis && isPlaceholder(node) && !labelIndex.contains(node)) { - labelIndex.emplace(node, placeholderOrder.size() + 1); - placeholderOrder.push_back(node); + // A node reachable by several displayed paths is settled by its + // first visit; re-walking it would assign nothing new and, on a + // DAG, costs one walk per path rather than per node (morph#602: + // 31 nodes built by repeated `q = q + q` have 2^30 paths and took + // 10.3 s to render 33 short lines). + if (!settled.insert(node).second) { + continue; } - // Both the labelled and the unlabelled arm descend exactly when the - // node is not an atom, so the two cases share one exit. + bool const labelIt = !frame.expandThis && isPlaceholder(node); + // An atom is a token in every rendering, so it is not a step and + // the budget does not apply to it — only its placeholder does. if (isAtomNode(*node)) { + if (labelIt) { + labelIndex.emplace(node, placeholderOrder.size() + 1); + placeholderOrder.push_back(node); + } continue; } + if (stepBudget == 0) { + elisionIndex.emplace(node, elisionOrder.size() + 1); + elisionOrder.push_back(node); + continue; + } + --stepBudget; + if (labelIt) { + labelIndex.emplace(node, placeholderOrder.size() + 1); + placeholderOrder.push_back(node); + } pending.push_back(LabelFrame{.node = node->right.get(), .expandThis = false}); pending.push_back(LabelFrame{.node = node->left.get(), .expandThis = false}); } } /// @brief Parenthesises and joins a binary subexpression. + /// + /// Takes its left operand **by value** and appends to it rather than + /// concatenating both sides into a fresh string. The left operand of a + /// left-leaning chain — the shape `total = total + row` records — is the + /// whole expression rendered so far, so copying it once per level made + /// rendering quadratic in the depth (morph#582: 27.7 s and a + /// 350,001-character line at 70,000 steps). Appending makes that shape + /// linear, amortised. A **right**-leaning chain (`a + (b + (c + …))`) is + /// still quadratic — the big operand is on the copied side — and so is a + /// chain of unary negations, which has to prepend; `equation()`'s step + /// limit is what bounds those, not this. /// @param op The operator token. - /// @param left Rendered left operand. + /// @param left Rendered left operand; moved from, so callers pass an + /// operand they are done with. /// @param right Rendered right operand. /// @return The combined rendering. - [[nodiscard]] static Rendered combine(const std::string& op, const Rendered& left, const Rendered& right) { + [[nodiscard]] static Rendered combine(const std::string& op, Rendered left, const Rendered& right) { int const precedence = (op == "*" || op == "/") ? 2 : 1; - std::string const leftText = (left.precedence < precedence) ? "(" + left.text + ")" : left.text; + std::string text = std::move(left.text); + if (left.precedence < precedence) { + text.insert(0, 1, '('); + text += ')'; + } + text += ' '; + text += op; + text += ' '; bool const rightNeedsParens = (right.precedence < precedence) || (right.precedence == precedence && (op == "-" || op == "/")); - std::string const rightText = rightNeedsParens ? "(" + right.text + ")" : right.text; - return Rendered{.text = leftText + " " + op + " " + rightText, .precedence = precedence}; + if (rightNeedsParens) { + text += '('; + text += right.text; + text += ')'; + } else { + text += right.text; + } + return Rendered{.text = std::move(text), .precedence = precedence}; } /// @brief Renders a unary-negation subexpression. @@ -227,6 +307,17 @@ struct EquationRenderer { /// @return The atom's rendering, or `std::nullopt` when @p node has /// operands that must be rendered first. [[nodiscard]] std::optional atomRendering(const ASTNode* node, bool expandThis, RenderMode mode) const { + // Checked first, and before `isPlaceholder`: a node can be both reused + // and past the limit, and `assignLabels` gives such a node an `eK` and + // no `cK` — so asking about the placeholder first would look up a + // label that was deliberately never assigned. An elided node is never + // named and never the root, so `expandThis` cannot be set on it. + if (auto const elided = elisionIndex.find(node); elided != elisionIndex.end()) { + if (mode == RenderMode::symbolic) { + return Rendered{.text = "e" + std::to_string(elided->second), .precedence = 100}; + } + return Rendered{.text = formatOptional(nodeValue(*node)), .precedence = 100}; + } if (mode == RenderMode::symbolic) { if (node->name.has_value()) { return Rendered{.text = "\"" + *node->name + "\"", .precedence = 100}; @@ -308,7 +399,7 @@ struct EquationRenderer { finished = Rendered{.text = formatOptional(top.node->current.rhs), .precedence = 100}; continue; } - finished = combine(top.node->current.operation, top.left, finished); + finished = combine(top.node->current.operation, std::move(top.left), finished); stack.pop_back(); } return finished; @@ -341,6 +432,19 @@ struct EquationRenderer { return label + " = " + renderSymbolic(node, true).text + " = " + renderSubstituted(node, true).text + " = " + formatOptional(node->current.result); } + + /// @brief Builds one `where`-legend line for an elided sub-derivation. + /// + /// Value only, and self-describing: an `eK` stands for work that was *not* + /// written out, so the line says so and names the limit that cut it rather + /// than leaving a caller to wonder why a number appeared where a formula + /// was expected. + /// @param node The elided node. + /// @return The legend body (`eK = (elided at the -step limit)`). + [[nodiscard]] std::string elisionLine(const ASTNode* node) const { + return "e" + std::to_string(elisionIndex.at(node)) + " = " + formatOptional(nodeValue(*node)) + + " (elided at the " + std::to_string(maxSteps) + "-step limit)"; + } }; } // namespace morph::units::detail @@ -349,10 +453,15 @@ namespace morph::units { template requires UnitEnum -std::vector Quantity::equation() const { +std::vector Quantity::equation(std::size_t maxSteps) const { if (!payload) { return {detail::formatOptional(payload)}; } + // Asked for no steps at all: the same one-element answer a build with + // tracing compiled out gives, since nothing of the derivation may be shown. + if (maxSteps == 0) { + return {detail::formatOptional(payload)}; + } const detail::ASTNode* root = _ctx.node.get(); if (root == nullptr) { return {detail::formatOptional(payload)}; @@ -367,7 +476,12 @@ std::vector Quantity::equation() const { return {detail::formatOptional(root->current.result)}; } - detail::EquationRenderer renderer; + // The reference count still walks the whole DAG: reuse is a property of + // the derivation, not of how much of it gets printed, and a value shown + // once must not be given a placeholder just because the limit hid its + // other uses. That walk is linear and iterative; only the *rendering* is + // what `maxSteps` bounds. + detail::EquationRenderer renderer{maxSteps}; renderer.countRefs(root); renderer.assignLabels(root, true); @@ -375,9 +489,19 @@ std::vector Quantity::equation() const { lines.push_back(renderer.renderSymbolic(root, true).text); lines.push_back(" = " + renderer.renderSubstituted(root, true).text); lines.push_back(" = " + detail::formatOptional(root->current.result)); - for (std::size_t i = 0; i < renderer.placeholderOrder.size(); ++i) { - std::string const body = renderer.legendLine(renderer.placeholderOrder[i]); - lines.push_back((i == 0 ? "where " : " ") + body); + // Placeholders first, then elisions: both are things the formula referred + // to by label, in the order the formula introduced them. The first legend + // line of either kind carries the `where `, the rest align under it. + bool firstLegendLine = true; + auto const appendLegend = [&lines, &firstLegendLine](const std::string& body) { + lines.push_back((firstLegendLine ? "where " : " ") + body); + firstLegendLine = false; + }; + for (const detail::ASTNode* node : renderer.placeholderOrder) { + appendLegend(renderer.legendLine(node)); + } + for (const detail::ASTNode* node : renderer.elisionOrder) { + appendLegend(renderer.elisionLine(node)); } return lines; } diff --git a/include/morph/util/quantity.hpp b/include/morph/util/quantity.hpp index a4585a87..de0da57f 100644 --- a/include/morph/util/quantity.hpp +++ b/include/morph/util/quantity.hpp @@ -53,6 +53,7 @@ #include #include #include +#include #include #include #include @@ -470,6 +471,24 @@ concept SameEnumDistinct = std::same_as && (A != B); } // namespace detail +/// @brief How many derivation steps `equation()` writes out by default. +/// +/// A derivation has no bound — `total = total + row` over a data-driven loop +/// records one step per iteration — so without a limit `equation()` renders +/// every one of them into a single line (morph#582: 100,000 steps produced a +/// 500,001-character line in 58.8 s). This is where a *rendered* formula stops +/// being something a person reads, not where the cost stops being tolerable: +/// a caller that wants more passes its own limit, so erring low costs one +/// argument while erring high costs an unreadable line nobody asked for. +inline constexpr std::size_t kDefaultEquationSteps = 100; + +/// @brief Pass as `equation()`'s limit to write the derivation out in full. +/// +/// Restores the pre-morph#582 behaviour: no step is elided, and the cost is +/// the caller's, taken deliberately. Used by the depth regression tests, which +/// exist precisely to walk a derivation deeper than any limit would render. +inline constexpr std::size_t kEquationStepsUnlimited = std::numeric_limits::max(); + // Forward declaration (carrying the default arg) so `convert` and the // conversion operator can name `Quantity` before the full definition. template ::meta(U).defaultDecimals> @@ -862,10 +881,18 @@ struct Quantity { } /// @brief The worked derivation as print-ready lines (see `docs/spec/util/quantity_type.md`). + /// @param maxSteps How many derivation steps to write out, across the + /// formula and the legend together. Past it a sub-derivation is + /// elided: it renders as `eK` and its value appears in the `where` + /// legend. `kEquationStepsUnlimited` writes the whole derivation + /// out (unbounded, quadratic on a right-leaning chain); `0` returns + /// the formatted value alone. Ignored with tracing off, which has + /// no derivation to render either way. /// @return `[0]` formula, `[1]` substitution, `[2]` result, `[3..]` `where` /// legend; a single formatted-value element for a degenerate root - /// (empty, named root, bare leaf, or tracing off). - [[nodiscard]] std::vector equation() const + /// (empty, named root, bare leaf, tracing off, or `maxSteps == 0`). + [[nodiscard]] std::vector equation( + [[maybe_unused]] std::size_t maxSteps = kDefaultEquationSteps) const #if MORPH_QUANTITY_PROVENANCE ; #else diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index be68863f..aa5f8eea 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -211,25 +211,15 @@ endif() include(Catch) # Every test is sub-second in practice; cap each at 120s so a hung/deadlocked # test fails fast instead of stalling the whole CI job (observed: a single test -# hanging blocked a Windows runner for over half an hour). One named exception -# below, registered separately: DISCOVERY_MODE PRE_TEST defers test discovery -# to ctest invocation time, so a per-test TIMEOUT can't be set with -# set_tests_properties() here (nothing named that test yet at configure time) -# -- excluding it by tag and giving the excluded tag its own -# catch_discover_tests() call is the mechanism that's actually available. -catch_discover_tests(morph_tests DISCOVERY_MODE PRE_TEST TEST_SPEC "~[slow]" PROPERTIES TIMEOUT 120) - -# `[slow]` (issue #589): `equation()` walks a 70,000-node provenance chain -# purely to prove the morph#574 iterative rewrite doesn't overflow the stack -# -- it has no performance budget of its own, but the O(n^2) string-building -# cost #582 measured (32.3s at this size under ASan+UBSan) is close enough to -# 120s that TSan's heavier per-access instrumentation pushes it over: observed -# at ">120.07s" on master's own CI (the sanitizer killed it at the deadline, -# so the true completion time is unknown, just past it). Widened generously -# rather than precisely measured, to leave headroom for sanitizer variance, -# without raising the cap for every other test and weakening the "hang fails -# fast" property the 120s default exists for. -catch_discover_tests(morph_tests DISCOVERY_MODE PRE_TEST TEST_SPEC "[slow]" PROPERTIES TIMEOUT 600) +# hanging blocked a Windows runner for over half an hour). +# +# One test used to need an exception, registered separately by tag: the +# 70,000-node `equation()` depth test, whose O(n^2) string building cost 32.3s +# under ASan+UBSan and blew past 120s under TSan (#589, widened to 600s by +# #590). #582 made that rendering linear -- the same test now takes 0.11s under +# ASan+UBSan and 1.3s at -O0 under TSan -- so the exception, and the `[slow]` +# tag it keyed on, are gone again and every test is back under one cap. +catch_discover_tests(morph_tests DISCOVERY_MODE PRE_TEST PROPERTIES TIMEOUT 120) # ── Two-binary journal-path skew test (issue #246) ─────────────────────────── # The executable form of the journal's data-at-rest contract, which diff --git a/tests/test_quantity.cpp b/tests/test_quantity.cpp index 73a937ba..352d5c2d 100644 --- a/tests/test_quantity.cpp +++ b/tests/test_quantity.cpp @@ -702,11 +702,13 @@ namespace { // nothing to run. constexpr int kDeepChainNodes = 100000; -// equation() renders the whole chain into one string by repeated -// concatenation, which is quadratic in the depth, so this one is priced: 70,000 -// nodes cost 32 s under ASan where 100,000 cost 83 s. It stays above the -// highest measured survival depth (50,000, clang -O2) with margin, which is -// what keeps it from passing vacuously in an optimised build. +// Stays above the highest measured survival depth (50,000, clang -O2) with +// margin, which is what keeps it from passing vacuously in an optimised build. +// It used to be priced as well -- rendering the chain in full was quadratic in +// the depth, 27.7 s at this size under ASan+UBSan and over ctest's 120 s +// timeout under TSan (morph#589) -- but since morph#582 `combine` appends to +// its left operand instead of copying it, and the same render costs 0.11 s +// under ASan+UBSan and 1.3 s at -O0 under TSan. constexpr int kDeepEquationNodes = 70000; // Builds `0 + one + one + ...`, one retained ASTNode per term. @@ -734,10 +736,15 @@ TEST_CASE("A 100000-node provenance chain is destroyed without overflowing the s } TEST_CASE("equation() walks a 70000-node provenance chain without overflowing the stack", - "[quantity][provenance][equation][morph574][slow]") { + "[quantity][provenance][equation][morph574]") { Euro const total = runningTotal(kDeepEquationNodes); - auto const lines = total.equation(); + // `kEquationStepsUnlimited`, not the default limit, and that is what keeps + // this test load-bearing: under the default the renderer stops 100 steps + // in and never walks deep enough to have overflowed anything, so it would + // pass against the recursive code this test exists to catch (morph#582). + // The chain is still rendered in full here -- 350,001 characters of it. + auto const lines = total.equation(morph::units::kEquationStepsUnlimited); // Formula, substitution, result, and one `where` line: the single `one` // leaf is referenced 70,000 times, so it earns exactly one placeholder. REQUIRE(lines.size() == 4); @@ -746,3 +753,134 @@ TEST_CASE("equation() walks a 70000-node provenance chain without overflowing th CHECK(lines[2] == " = 70000"); CHECK(lines[3] == "where c1 = 1"); } + +// ── morph#582: the rendered derivation is bounded, the walk is not ── +// +// Two separate things, and the tests below hold them apart. The **step limit** +// bounds what `equation()` writes out (the first two cases); the **append in +// `combine`** is what keeps writing it out in full affordable when a caller +// asks for that (the case above, which renders all 70,000 steps and cost 27.7 s +// before the change, 0.11 s after, measured at -O1 under ASan+UBSan). +namespace { +// Spells the expected formula out rather than matching a prefix of it: a +// `starts_with` check would pass just as well against an uncapped 350,001-char +// line, which is the very thing these cases are about. +[[nodiscard]] std::string repeated(const std::string& unit, std::size_t times) { + std::string out; + for (std::size_t i = 0; i < times; ++i) { + out += unit; + } + return out; +} +} // namespace + +TEST_CASE("equation() renders a deep derivation within the default step limit", + "[quantity][provenance][equation][morph582]") { + Euro const total = runningTotal(kDeepEquationNodes); + + auto const lines = total.equation(); + + // The whole assertion is that this output is *small*: unlimited, the same + // value renders as 4 lines whose first is 350,001 characters long. + REQUIRE(lines.size() == 5); + CHECK(lines[0] == "e1" + repeated(" + c1", morph::units::kDefaultEquationSteps)); + CHECK(lines[0].size() == 502); + CHECK(lines[1] == " = 69900" + repeated(" + 1", morph::units::kDefaultEquationSteps)); + // The result is the real one: eliding the deep end of the derivation does + // not touch the value, only the account of how it was reached. + CHECK(lines[2] == " = 70000"); + CHECK(lines[3] == "where c1 = 1"); + // Self-describing, and it names the limit that cut it: a caller meeting an + // `e1` for the first time can tell it apart from an ordinary value. + CHECK(lines[4] == " e1 = 69900 (elided at the 100-step limit)"); +} + +TEST_CASE("equation()'s step limit is the caller's to set", "[quantity][provenance][equation][morph582]") { + SECTION("a derivation at exactly the limit is written out whole") { + // 100 steps, 100 allowed: nothing is elided, and the output is the + // same one the uncapped renderer produced. + auto const lines = runningTotal(100).equation(); + REQUIRE(lines.size() == 4); + CHECK(lines[0] == "0" + repeated(" + c1", 100)); + CHECK(lines[3] == "where c1 = 1"); + } + + SECTION("one step past it elides exactly one sub-derivation") { + auto const lines = runningTotal(101).equation(); + REQUIRE(lines.size() == 5); + CHECK(lines[0] == "e1" + repeated(" + c1", 100)); + CHECK(lines[2] == " = 101"); + CHECK(lines[4] == " e1 = 1 (elided at the 100-step limit)"); + } + + SECTION("a caller that wants less says so") { + auto const lines = runningTotal(10).equation(3); + REQUIRE(lines.size() == 5); + CHECK(lines[0] == "e1 + c1 + c1 + c1"); + CHECK(lines[1] == " = 7 + 1 + 1 + 1"); + CHECK(lines[2] == " = 10"); + CHECK(lines[4] == " e1 = 7 (elided at the 3-step limit)"); + } + + SECTION("a caller that wants the whole derivation says so") { + auto const lines = runningTotal(150).equation(morph::units::kEquationStepsUnlimited); + REQUIRE(lines.size() == 4); + CHECK(lines[0] == "0" + repeated(" + c1", 150)); + CHECK(lines[2] == " = 150"); + } + + SECTION("a limit of zero is the tracing-off answer: the value alone") { + CHECK(runningTotal(10).equation(0) == std::vector{"10"}); + } + + SECTION("a derivation shorter than the limit is untouched") { + // The limit must not change what a domain-sized explanation looks + // like; this is the same output the pre-limit renderer produced. + auto const a = KilowattHour::fromDouble(6.0).named("a"); + auto const b = KilowattHour::fromDouble(1.8).named("b"); + auto const lines = (a - b).equation(); + REQUIRE(lines.size() == 3); + CHECK(lines[0] == R"("a" - "b")"); + } +} + +// ── morph#602: the label walk must count nodes, not root-to-leaf paths ── +TEST_CASE("equation() renders a derivation shared along many paths in node time", + "[quantity][provenance][equation][morph602]") { + // `q = q + q` forty times: 41 distinct nodes, 2^40 root-to-leaf paths. + // Before `assignLabels` carried a visited set the walk was per-path, and + // the measured curve (0.099 s at 25 nodes, x4 per node, morph#602) puts + // this run at hours -- so the failure signal here is ctest's 120 s timeout, + // as the morph#574 cases' is a segfault. The assertions below cannot tell + // the two implementations apart; the clock is what does. + Euro q{Rational{Numerator{1}, Denominator{1}, DecimalPlaces{2}}}; + for (int i = 0; i < 40; ++i) { + q = q + q; + } + + auto const lines = q.equation(morph::units::kEquationStepsUnlimited); + // One `c` per level plus formula, substitution and result: linear in the + // node count, which is the point. + REQUIRE(lines.size() == 43); + CHECK(lines[0] == "c1 + c1"); + CHECK(lines[2] == " = 1099511627776"); + CHECK(lines[3] == "where c1 = c2 + c2 = 274877906944 + 274877906944 = 549755813888"); +} + +TEST_CASE("equation() numbers several elisions in first-appearance order", + "[quantity][provenance][equation][morph582]") { + // A bushy derivation rather than a chain: ((1+2) + (3+4)) + ((5+6) + (7+8)), + // seven steps, each leaf distinct so nothing earns a `c` placeholder. With + // three steps allowed, the walk expands the root and the left spine and + // cuts everything the fourth step would have reached. + auto const leaf = [](double v) { return Euro::fromDouble(v); }; + auto const total = ((leaf(1) + leaf(2)) + (leaf(3) + leaf(4))) + ((leaf(5) + leaf(6)) + (leaf(7) + leaf(8))); + + auto const lines = total.equation(3); + REQUIRE(lines.size() == 5); + CHECK(lines[0] == "1 + 2 + e1 + e2"); + CHECK(lines[1] == " = 1 + 2 + 7 + 26"); + CHECK(lines[2] == " = 36"); + CHECK(lines[3] == "where e1 = 7 (elided at the 3-step limit)"); + CHECK(lines[4] == " e2 = 26 (elided at the 3-step limit)"); +}