From 5b2e909b9d7f7a9817e923dbb0f9395e71a382c5 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sun, 20 Sep 2026 03:00:09 +0200 Subject: [PATCH] util: bound what equation() renders, and make rendering it in full cheap (fixes #582, fixes #602) A derivation has no bound -- a data-driven `total = total + row` loop records one step per iteration -- and equation() rendered 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); a caller printing it emitted a half-megabyte line. #574 called that out ("an explanation 200,000 steps deep is not an explanation") and deferred it, because capping changes a documented output contract rather than fixing a defect. So this changes the contract, deliberately and in the spec first. equation() takes `maxSteps`, defaulted to kDefaultEquationSteps (100). Past it a sub-derivation is elided: it renders as `eK` in the formula, as its value in the substitution, and takes a legend line of its own, `e1 = 99900 (elided at the 100-step limit)` -- self-describing, and it names the limit, so a caller meeting an `eK` can tell it from an ordinary value without reading the spec. The result line is untouched: eliding changes the account of how a value was reached, never the value. Same input, same build: 502 characters in 0.057 s. The limit is a parameter rather than a constant because how much of a derivation is worth reading belongs to the layer doing the reading, not to the value type -- an audit log and a tooltip do not want the same answer. Its default sits at the human end of the range rather than at the affordable end, because the two errors are not symmetric: a default too low costs one argument, a default too high costs everyone a line nobody can read and never knew was unbounded. kEquationStepsUnlimited restores the old rendering; 0 gives the formatted value alone. The second half of #582 is the quadratic cost, and it wanted its own answer. `combine` now appends to its left operand instead of concatenating both sides into a fresh string, so the left-leaning chain an accumulate loop records is linear in its depth: rendering 70,000 steps in full went from 27.7 s to 0.11 s under ASan+UBSan, and from past ctest's 120 s timeout to 1.3 s at -O0 under TSan. That is what lets the #574 depth regression test keep its evidence -- it now asks for kEquationStepsUnlimited, because under the default limit the renderer stops 100 steps in and would pass against the recursive code it exists to catch. It also retires #590's `[slow]` tag and 600 s timeout exception: 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 bounds those, and the spec says so. #602, found on the way and fixed here because the limit depends on it: assignLabels was the one walk without a visited set, so a node reachable by k displayed paths was walked k times. On a DAG that is exponential -- 31 nodes built by repeated `q = q + q` have 2^30 paths and took 10.3 s to render 33 short lines. Without the set the step budget would have been spent on repeat visits and the limit would fire on derivations far smaller than 100 distinct steps. Measured, clang 22.1.8, -O1 under ASan+UBSan, `total = total + one` n times then equation(); median of three runs: n before after (default) after (unlimited) 40,000 7.544 s/200,001c 0.020 s/502c 0.074 s/200,001c 70,000 27.720 s/350,001c 0.024 s/502c 0.112 s/350,001c 100,000 58.837 s/500,001c 0.057 s/502c 0.157 s/500,001c The new tests were checked against a mutant whose budget never exhausts: four assertions fail, all of them the size of the output. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GS5K2vqZtC4xbRiGJHT7jH --- CHANGELOG.md | 43 ++++++ docs/spec/util/quantity_type.md | 112 ++++++++++++++- include/morph/detail/quantity_equation.hpp | 158 ++++++++++++++++++--- include/morph/util/quantity.hpp | 31 +++- tests/CMakeLists.txt | 28 ++-- tests/test_quantity.cpp | 152 +++++++++++++++++++- 6 files changed, 477 insertions(+), 47 deletions(-) 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)"); +}