Repository navigation
perf: parser, metric, output, and CLI batch (#1098, #1100, #1107-#1111, #1115, #1118) - #1134
Merged
Merged
Conversation
`guess_language` ran the Emacs/Vim modeline scan on every file — up to 20 regex executions plus a lowercased `String` — even though every arm of its extension branch returned the extension's language, including the "modeline agrees" arm where the two are equal by construction. The extension lookup also allocated a lowercased copy unconditionally. Resolve through a lazy `or_else` chain instead, so the modeline runs only when the extension resolves to nothing and the shebang only when both do. The extension is now borrowed when it is already ASCII lowercase; the `is_ascii` guard is load-bearing because `str::to_lowercase` folds U+212A onto ASCII `k`. Precedence and the public `(Option<LANG>, &'static str)` result are unchanged. Measured over a 53 KB buffer with a resolving extension: 824.4 ns/call before, 16.4 ns/call after. End to end this is below the noise floor — `bca metrics -p src` is 524 ms +/- 3 ms for ~360 files. Fixes #1111
`Tree::new` built a fresh `tree_sitter::Parser` for every file. It now
borrows one from a thread-local slot, rebinds the grammar, parses, and
puts it back.
The issue's stated cause was wrong, and measuring first changed the
design. `Parser::new()` + `set_language()` costs 0.27-0.75 us against a
200-350 us parse of a 4 KiB file — 0.1-0.2%, not "a measurable per-file
constant". What reuse actually saves is re-growing the parser's lexer,
GLR stack, stack-node pool, subtree pool and scratch arrays, which
`set_language` and `parse` reset without releasing.
Two consequences for the plan the issue proposed:
* Keying the cache on `LANG` to skip `set_language` buys nothing —
rebinding on every parse measures identically to binding once, and
`ts_language_copy` is a no-op for the static grammars we ship. So the
slot caches the parser only, and the stale-grammar failure mode never
becomes representable.
* `Parser::reset` is redundant. `ts_parser_set_language` opens with
`ts_parser_reset`, and `Tree::new` always parses with `None` as the
old tree.
The parser is moved out of the slot for the duration of the parse, so a
re-entrant call builds its own rather than observing one mid-parse. Both
accesses use `try_with`: a parse issued from another thread-local's
destructor can run after this slot is destroyed, where `LocalKey::with`
aborts the process ("thread local panicked on drop") rather than
failing. That path is reached in practice — swapping `try_with` for
`with` turns the teardown test into a SIGABRT.
Caching a parser across files is sound because the tree does not depend
on it: `ts_tree_delete` releases subtrees through a pool of its own and
holds its own grammar handle, so a tree outlives both the parser and the
thread that produced it.
Measured, honestly: -2.5% of in-situ parse time (median of 9 paired
runs, faster in 8 of 9) over 8,792 files averaging 3.3 KiB; 8-9% in a
parse-only loop where the buffers stay cache-hot. End to end it is under
the noise floor — a `nocache` control build, semantically identical to
the old code, differed from it by 2.2% on wall clock, which is larger
than the effect. Retention is bounded by tree-sitter's own pool caps
(32 subtrees, 50 stack nodes), so a worker holds a few KiB whatever it
parses. `make bench-scaling`: 18/18 probes green, no class change.
The baseline refresh records `src/node.rs` `loc.sloc = 800` for the soft
headroom tier. It also corrects pre-existing line-number drift for two
`src/metrics/cognitive.rs` entries left stale by daccb6f.
Fixes #1118
Review of the #1111 / #1118 wave found two problems worth fixing before the batch continues. The parser-cache machinery pushed `src/node.rs` to `loc.sloc = 800` against a cap of exactly 800, so the next production line added anywhere in the file would have turned `make self-scan` red for everyone, and the baseline entry recording it could not absorb that. Moving the cache to `src/node/parser_cache.rs` drops the file to 745, retires the baseline entry, and gives the cache a home for its own unit tests. Nothing observed the reuse itself. Deleting the write-back in `parse_on_scratch_parser` reduces the cache to a no-op, and all five tests in `tests/parser_reuse.rs` still pass, because every one of them compares parse *output* against a fresh-parser reference. Verified by perturbation: with the write-back removed, those five stay green and only the two new construction-count tests fail. The counter sits on the slot-miss path, which runs once per thread, so it costs nothing per file. Also from review: * `lang_from_ext` was byte-identical to `get_language_for_file`'s body; `guess_language` now calls the public function directly. * `lowercase_ext` folds with `to_ascii_lowercase` past the `is_ascii` guard, where the input is ASCII by construction. * `get_regex` uses `captures` rather than `captures_iter().next()`. * The `parse` invariant now records that it holds only because nothing sets a timeout, cancellation flag, or included ranges — `set_language` resets parse state but not those, and the parser is no longer freshly built. * `ParseOnDrop` records into an `AtomicBool` checked after `join`; an assertion inside a thread-local destructor aborts the process instead of reporting a named failure. * `assert_parsed_cleanly` checks for a function node rather than a magic-number s-expression length.
The reference tree and the tree under test were selected by two separate `if lang == LANG::Rust` expressions, so a third language would have had to be added in both places; getting one wrong compares a Rust tree to a TypeScript reference, which fails for a confusing reason. Both now go through `fixture`. `threads_are_isolated_and_trees_outlive_their_thread` was also the one test computing references without running `assert_parsed_cleanly` over them, leaving its comparisons able to pass against two equally broken trees. `assert_parsed_cleanly` takes the `LANG` rather than a hand-written label, which `Display` already provides.
The test audit found that both wave-1 optimizations were unobserved. The parser-cache tests called `parse_on_scratch_parser` directly, so nothing checked that production still reaches it: reverting `Tree::new` to its pre-#1118 body left all 3,143 lib tests and all 5 integration tests passing. A test now drives the public `Ast::parse` seam and counts constructions, and is the only failure under that revert. `guess_language` had the same gap one level over. Every assertion pins what it returns, and all of them hold if the modeline scan runs eagerly and is discarded — which is the work #1111 removed. A thread-local scan counter closes it; re-eagering the chain fails that test alone, 1 of 3,145. The counter is unconditional so tests observe the production path, and the prose around it was tightened to keep `src/tools.rs` inside the soft headroom tier rather than spend a baseline entry on it. Also from the audit: * `guess_language_unrecognised_modeline_keeps_extension` used `foo.c`, whose extension short-circuits, so it never reached the branch it named. Renamed, and given the two inputs that do reach it. * The parser-cache tests are gated on the `rust` feature, as their integration sibling already was; without it they failed at `expect` under `--no-default-features` rather than being skipped. * `each_thread_builds_its_own_parser` relied on iterator laziness to serialize its threads. That property is load-bearing — a global cache is only deterministically observable across consecutive threads — so the loop is now spelled out and the reason recorded.
`HalsteadMaps::operators` is keyed by a tree-sitter `kind_id` — a `u16` grammar symbol this crate generated, drawn from an alphabet of at most 639 values (mozcpp, the largest grammar here). Hashing it with keyed SipHash-1-3 buys nothing and costs a full keyed round on a path that runs once per non-primitive operator node. That is the case `src/int_hash.rs` was introduced for in #1069; Halstead's map was missed at the time. Move it onto `IntKeyHashMap` and add the `write_u16` override the map's key width needs, so the probe does not fall into `Hasher::write`'s byte-at-a-time loop. The issue's other two proposals are declined, with the reasoning recorded in the module docs: - A dense `Box<[u64]>` indexed by `kind_id` would be slower here, not faster. Measured over the three corpus repositories, the median space holds 8 distinct operators and the 99th percentile 40, against grammars of 16-639 symbols — so a per-space dense array would zero and rescan several hundred slots per space, and again per `merge`, to replace ~10 map entries. - `primitive_operators` and `operands` stay on SipHash. Their keys are identifiers and literals lifted verbatim out of the file being analysed, which for this crate is untrusted input: `bca` runs over whatever a repository contains and `bca-web` accepts source in a request body. Hash-flooding resistance is load-bearing there. Metric values are unchanged — `merge` and `finalize` are both order-independent, and the one place operator iteration order escapes into output (`bca ops`) has sorted since #1091. No submodule snapshot moved. `make bench-scaling` holds: `halstead/nested-paren` 1.00, `halstead/nested-not` 0.99, all eighteen probes ok. Fixes #1108
`Ploc::lines` and `Cloc`'s two comment-line sets were `IntKeyHashSet<usize>`, and merging a child space into its parent re-inserted every element one at a time. A row inside D nested spaces therefore cost D hash probes on the way up, and the sets stay resident in the returned `FuncSpace` tree. Replace them with `LineSet`, a word array offset by the first row inserted. Merge is a word-wise `|=`, cardinality a popcount sum, and `cloc()`'s two-set union is inclusion-exclusion over the shared words. Semantics are unchanged: the fold is still a set union, so a line shared by sibling spaces counts once in the parent. The offset is derived from the data rather than from the space's `[start_row, end_row]` span. Instrumenting `compute_minmax` showed no parsed space records a row outside its span across pdf.js, DeepSpeech and serde, but the span is not yet known when the first row arrives and `Stats::with_cloc_sloc` records rows past it deliberately — a span-anchored offset would drop such a row silently instead of failing. `IntKeyHashSet` had no other user and is removed. Measured: the new `loc/nested-fn-rows` scaling probe fits 2.11 before and 1.10 after (bound 1.50); at depth 4000 its cost relative to the `nom/nested-fn-rows` control falls from 39.4x to 1.36x. Over the three corpus repositories (14 438 files, 4.03M rows, 234 605 spaces) the merge path sheds 7 547 304 hash inserts, and the retained set payload drops from ~170 MB to ~5.8 MB. On `corpus/walk/loc` the effect is below this host's noise floor: 11 interleaved rounds gave a median after/before of 1.003 with 5/11 rounds faster, while the `corpus/walk/nom` control moved just as far (0.990, 6/11) — real code nests spaces at most nine deep, so the merge is a small share of that walk. Fixes #1109
`serde_json::to_string(&ops)` built a full owned `wire::Ops` clone first, and `Ops` carries a `Vec<String>` vocabulary per space. `finalize` merges each child space's Halstead maps into its parent, so a parent's vocabulary is a superset of every descendant's and the clone re-copies an entry once per enclosing space. On a hundred-level nest of functions with distinct identifiers that clone was 80% of the call: 0.445 ms -> 0.082 ms. On a tree past `MAX_SPACE_SERIALIZE_DEPTH` it was *all* of it, because the whole tree was cloned and then dropped unserialized: 109 ms -> 0.007 ms at 2 000 levels. `Ops` now serializes through a borrowed mirror in the new private `wire::ops_view`; `to_wire()` still returns the owned projection. `compute_operators_and_operands` sorted a freshly built `Vec<String>`. Sorting the borrowed keys and rendering afterwards keeps each swap to a fat pointer and lays the `String`s out in output order, which every later pass reads sequentially: `Ast::ops` on the same shape is ~22% faster at depth 2 000 (760 ms -> ~595 ms) and `to_wire` ~52% (108 ms -> ~51 ms). The two orders differ only when a key is not valid UTF-8, since the lossy rendering is not order-preserving, so a vocabulary that actually lost bytes is re-sorted on the rendered text. `ops_inner` also classified a space kind for every node and used it only when a space opened, where `spaces::compute::open_func_space` has classified after the decision since #522. Hoisted. The effect is below this host's noise floor, so a thread-local counter is what pins it. Output is unchanged: `bca ops` over `src` and `tests` is byte-identical to the pre-change binary across 59 120 files in JSON, YAML, TOML and CBOR, and the human-readable tree agrees as a multiset (its file order has always been non-deterministic under the parallel walk). Adds an `ops/nested-fn` probe to the scaling gate, the first to run `ops_inner` at all: 1.06 before, 1.03 after, both linear. `Probe` now carries a `Workload` so a probe can drive `Ast::ops` as well as `Ast::metrics`. The probe covers the walk, not the vocabulary — `Ops` publishes a `Vec<String>` per space, so a shape with distinct identifiers per level has quadratic *output* and no implementation fits under a linear bound. That is why the issue's interning proposal is not here: it needs a shared representation in the public type, i.e. a 3.0 break. `docs/development/benchmarking.md` records the reasoning. Fixes #1110
`serializing_ops_builds_no_owned_projection` is gated on the `rust` feature but its three imports sat at module scope, so a `--no-default-features` build warned on all three. `make pre-commit` does not run that leg; CI does.
Review of the wave-2 fixes (#1108, #1109, #1110) found one performance regression, two vacuous tests, and four independently-grown copies of one idiom. `Ploc`/`Cloc`/`Stats::compute_minmax` called their accessors twice. Those were `HashSet::len()` — O(1) — before #1109; `ploc()` is now a popcount over the word array and `cloc()` is three array scans, so `Cloc::compute_minmax` did six scans per space where it used to do two O(1) reads. Bound once, it does three. `insert_range_ignores_an_inverted_span` did not cover its guard: both endpoints sat in one word, where the mask arithmetic already yields zero, so deleting `if end < start { return; }` left all 13 tests green. The guard only bites across a word boundary, where the interior fill slices `words[2..0]`. With a cross-word case the deleted guard is the only failure in the suite. Nothing called `insert_range` on a non-empty set either, leaving both growth branches — the ones the "reserve both ends first" comment defends — unreached; a seeded case now covers them. Four thread-local counters existed so a test could observe an optimization that changes no output, written independently in three dialects. Their shared invariant — the counter and its bump are unconditional, only the accessor is `cfg(test)` — was enforced by nobody, and gating the counter would leave the test measuring a build production never ships. `observation::counter!` emits the three items together so that cannot happen. Per-site rationale stays above each invocation. Also: the `insert_range` guard comment named the wrong justification, `LineSet`'s density claim was too absolute for a blank-line-heavy file, `len()`'s new O(words) cliff and `reserve`'s asymmetric growth cost were undocumented, `classify_space_kind` read as claiming parity with `open_func_space` (they diverge — #1130), `STABILITY.md` did not say that ops ordering is over the rendered strings, and `lib.rs` still listed `metrics::loc` as an `int_hash` consumer.
`Checker::should_skip_subtree` reached `Node::previous_sibling` to walk backward across the run of `#[...]` siblings before an item. `ts_node__prev_sibling` opens with `ts_node_parent`, which tree-sitter resolves by descending from the root, so the scan was `O(attributes x depth)` per item and quadratic over a nested file. The hook now takes the `Ancestors` chain the walker already carries and, under a parent narrow enough for it to be the cheaper bound, reads the attribute run forward from that parent instead. Measured here, a cursor sibling step costs ~68 ns against ~420 ns for a whole `previous_sibling` on a shallow node, and that 420 ns did not move with the sibling index -- so reading forward unconditionally would have traded the depth quadratic for a width one (a generated file of 2_000 top-level attributed items went 6.0 ms -> 569 ms). Above the measured break-even the backward walk stays. No metric value moves: `rust_outer_attr_scans_agree` compares the two readings node by node across every attribute shape, and an `observation::counter!` on `Node::previous_sibling` is what makes a revert fail rather than merely run slower. `Workload::Metrics` in the bench harness gains an `exclude_tests` flag, which unblocks two probes that had no way to reach their hot path: `nom/nested-attributed-fn` (2.00 -> 1.04 fitted, 2.05 s -> 8.1 ms at depth 4_000) and `nom/nested-cfg-predicate`, the guard #1105 deferred here. Fixes #1100
`record_indirect_includes` ran a fresh petgraph `Dfs` per file over the already-collapsed include graph, so every shared header was re-visited once per file that reached it. It now computes each node's closure once, in reverse topological order over the cycle-free graph, merging its successors' finished index sets; the per-file walk is kept as the fallback for a graph `toposort` cannot order, so a violated assumption costs time rather than correctness. `get_macros` deep-cloned a file's whole transitive macro set on every C/C++ parse and allocated a `PathBuf` per indirect include just to probe a `PathBuf`-keyed map. `Parser::new` now goes through a crate-internal `visible_macros`, which borrows the names and keys the map with `Path::new`; the published `get_macros` shape is unchanged. Measured on the 10_918-file DeepSpeech corpus (V=10_039, E=46_773, 732_585 closure entries), both paths compiled into one binary: `record_indirect_includes` 126.6 ms -> 83.0 ms (median of 3), and the macro set per full pass 292 ms -> 195 ms with 2.37 M fewer allocations. Closures are byte-for-byte the same across all 10_024 files. Not shipped, deliberately: storing closures as interned ids, bitsets, or `Arc`s. `PreprocFile::indirect_includes` is a published, serialized `HashSet<String>`, so materializing one owned string per reachable pair is fixed until 3.0 — and it is two thirds of what this pass costs. Fixes #1107
Review of the wave-3 fixes found a cliff left by #1100, a latent bug in #1107, and a test that could not fail. #1100 dispatched on parent width alone, so one child past the threshold returned the full O(depth^2) walk — a module body holding five items instead of four, nested a few hundred deep, kept the quadratic. The budget now grows with the depth the walker already knows. The factor is 3, measured rather than assumed: the marginal cost of one level inside `previous_sibling` is ~120 ns against ~35 ns per cursor step, a ratio near 3.4. At depth 3200 a width-7 parent goes 2.671 s to 0.056 s, and nothing measured is slower — a 20 000-module shallow file and a 400-child parent are both flat. Reverting to width-only fails the two new tests and nothing else. `merge_sorted_ids` never cleared its output buffer; correctness rested on its one caller doing it. A second caller would have produced a corrupt union silently. It clears its own buffer now, and the new test seeds a non-empty one — the existing test seeded from empty, so it could not have caught this. The preproc fixture asserted `cyclomatic_sum() == 3` to prove the C-family macro masking ran. It proves nothing: `$` is an identifier byte in tree-sitter-cpp, so `DBG` and `$$$` give identical metrics. Deleting the whole `LANG::C | Cpp | Mozcpp` arm from `get_fake_code` left the test green. Equal-length macro names now collapse to one masked operand, so `unique_operands()` reads 3 masked against 4 unmasked, and deleting that arm fails the test. Also from review: the threshold boundary had no fixture (widths ran {1,2,4,5,8,9,65,400}, so neither 6 nor 7 was covered, and no assertion recorded which arm ran); `src/preproc.rs` sat at exactly its 760-sloc soft cap, where one added line turns the gate red for everyone, and is now at 757; the scaling report told the reader `nom/nested-fn` was the only probe leaving `exclude_tests` off, when 21 do; and `benchmarking.md` claimed the width axis was pinned by unit tests that only pin agreement between the two readings, never cost.
`#1060` wired the read-failure counter into `bca check` only. Every other walking subcommand left `Config.read_failures` at `None`, so an input file that could not be read printed `error processing <path>: …` on stderr and the process still exited 0 — a metrics document, report, node count, or exemptions audit silently missing files. Move the guard into the shared walk layer instead of repeating it per runner: `read_failures` becomes unconditional, the single `ConcurrentRunner` seam returns the tally, and `run_walk` / `run_walk_resolved` / `run_walk_collecting` enforce it. That covers `metrics`, `ops`, `report`, `functions`, `find`, `count`, `dump`, `exemptions`, `preproc`, and `strip-comments` — which the issue's list missed — and cannot be forgotten by a future subcommand. The contract is "any read failure", not "produced nothing at all", matching `check` and the documented meaning of `EXIT_TOOL_ERROR`. The narrower rule would miss exactly the cases that matter: a partial `--output` aggregate looks complete, and a `diff --since` side that lost a file reports it as added or removed rather than as I/O failure. `diff --since` returns `DiffError::UnreadableInputs` rather than exiting, so its two temp trees still unwind, and names the side. Fixes #1098
Every file destination handed the serializer a raw `std::fs::File`, and the terminal dumps wrote through a `StandardStream` (a `LineWriter`), so the incremental serializers issued one `write(2)` per structural token and the tree renderers one per line. A 165-file `--output-dir` run cost 4,757,028 writes; the 12 MB `--output` aggregate 4,757,194. One seam in `formats.rs` now wraps both destinations — file and stdout — in a 64 KiB `BufWriter` and flushes it explicitly. The flush is the correctness half: a `BufWriter` drained only by `Drop` discards the error it hit, turning a full disk into a truncated document and exit 0. `check_format.rs`, the three `vcs` CBOR arms, and the `vcs` CSV writer route through it; `emit_check_results` buffers its stderr lock and flushes before the step-summary and `process::exit` paths. `open_file`'s eager `create_dir_all` before every `File::create` becomes a retry on `ErrorKind::NotFound`, and the four terminal dumps render into a `termcolor::Buffer` printed under the stdout lock in one `write_all`, which keeps a parallel walk's per-file trees atomic as `StandardStream` did. `bca dump`/`find` hold the stdout lock across their banner so the wider render window cannot detach it from its tree. Measured (pdf.js/src): --output-dir 4,757,028 -> 303 writes (1.55s -> 0.13s), --output aggregate 4,757,194 -> 197 (3.36s -> 0.20s), metrics text tree 1,524,444 -> 165 (0.47s -> 0.14s), ops text tree 170,560 -> 169, check sarif 5,622 -> 16, check stderr 741 -> 4. Output is byte-identical in every format and on every destination. Fixes #1115
Review of the wave-4 fixes found one regression this batch introduced, one gap it left open, and an untested half. #1115 rendered each terminal dump into one in-memory buffer, and the rendered document is O(nodes x depth), not proportional to the source as its comment claimed. A 16 KB file of nested parens went from 12.7 MB resident, streamed, to 545 MB. `print_to_stdout` now emits every 64 KiB through an injectable sink, taking the stdout lock at the first emission and holding it to the last so a multi-chunk document stays contiguous. The same file now peaks at 21.3 MB, and a 16-worker DeepSpeech dump falls from 430 MB to 269 MB. Output is byte-identical. The library half of #1115 was also untested: reverting all five files to the unbuffered form failed nothing, because `print_to_stdout` hard-coded its destination where `write_flushed` is generic over its sink. It now takes a `ColorSink`, and write-count tests fail under both a write-through revert and a dropped chunk flush. `write_flushed`'s doc claimed the explicit flush folds a full disk back into the returned `Result`. It does — and the walk consumer then printed the error and dropped it, so `metrics -O json --output-dir` onto a read-only directory wrote nothing and exited 0. That is the mirror of the read-failure gap #1098 just closed, so it uses the same seam: write failures are tallied and `enforce_complete_walk` covers both. `BrokenPipe` still exits 0. `check`'s "not optional" flush was decoration — `BufWriter::drop` flushes, and the adjacent `drop(stderr)` is what orders the streams. Perturbation confirms neither line alone is load-bearing. The redundant flush is gone and the comment now names the real cost: a panic before the drop loses the report, because the drop skips its flush when panicking. Holding the stdout lock across banner and tree fixed a real interleaving under `--jobs N` (5 interleavings in 3 runs before, 0 after) but shipped with no test; it has one now. `vcs_command.rs` sat at exactly its baselined 815 sloc, where the next comment line would go red for everyone. The CBOR-to-file arm was triplicated verbatim across the three vcs files while its error message already lived in `formats.rs`; hoisting that and its three siblings takes the file to 787, under the hard limit with real margin.
Record the nine issues fixed in this batch. The two exit-code entries are called out as CI-affecting: a run that previously tolerated an unreadable input, or an unwritable output, now fails.
`ColorSink::exclusive` hands back a `StdoutLock`, which is the right guard only for a writer pointed at stdout. The impl sat on `BufferWriter`, and `BufferWriter::stderr` has the identical type, so a future stderr caller would have been handed stdout's lock — excluding the wrong writers while leaving two stderr renders free to interleave. A `StdoutSink` newtype makes that pairing unrepresentable rather than merely unused today.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1134 +/- ##
==========================================
- Coverage 98.18% 98.15% -0.03%
==========================================
Files 272 276 +4
Lines 68345 69223 +878
Branches 67915 68793 +878
==========================================
+ Hits 67107 67949 +842
- Misses 848 873 +25
- Partials 390 401 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Patch coverage on #1134 trailed project coverage (96.19% against 98.15%), so the added-but-uncovered lines were enumerated and triaged rather than chased as a number. Of 85 such lines, most are not worth covering and some cannot be: ten in `preproc.rs` are a fallback the code itself documents as unreachable, four more are defensive early returns, and nine sit inside `#[cfg(test)]` as `assert_eq!` message arguments, which Rust evaluates only when the assertion fails — they are uncovered precisely because the tests pass. What was worth covering is the output path, where a defect reaches users as a corrupt document. `dump_aggregate` now round-trips through all four formats in both pretty and compact spellings, with the TOML `files` wrapper asserted by deserializing through a struct that names the key, so dropping the wrapper fails where a bare validity check would not. `dump_csv_aggregate` is pinned to emit the shared header exactly once for many files, which is the corruption the function exists to prevent. The pretty flag gets its own test because both spellings parse back to the same value: the discriminating check is that exactly one of them is multi-line. Also cover `ChunkedSink`'s `WriteColor` delegation, added here for bounded terminal output, and `LineSet`'s hand-written `Debug`. Every assertion was perturbation-verified. One did not survive that: asserting the color test with `contains("\u{1b}[")` still passed when `set_color` dropped its spec, because `reset` emits an escape of its own. The assertion is now positional — an escape before the styled word and another between the words — so all three delegated methods fail independently. Uncovered added lines drop from 85 to 55.
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.
Second batch off the #1105–#1127 performance audit: nine issues fixed
across the parser, metric, output, and CLI layers, on one integration
branch.
Parserper thread instead of rebuilding and rebinding it per fileOpsthrough a borrowed projection rather than rebuilding and re-sorting per-space vocabulary strings for every ancestorlocline sets as a bitset instead of merging element-wise up the space stackkind_idrather than through SipHash on the per-node pathexclude_testsattribute scan and scale the forward scan by depth, retiring the O(attributes × depth) climbcheckSupporting refactors and test work ride along: the parser cache is
extracted with its reuse guarded, the observation counters are shared,
the stdout lock is tied to a stdout sink, the terminal dump buffer is
bounded,
wire's Rust-only test imports are scoped to their test, andthe tools/parser fixtures are routed through one selector.
CHANGELOG.mdis updated in-branch (75 lines under## [Unreleased]).Scope
60 files, +6,549 / −1,094. New test files include
tests/parser_reuse.rsandsrc/wire_ops_tests.rs.Note on validation
I opened this PR at the request of the repository owner; the commits
are from a separate working session, so I have not personally run
make pre-commitover all 18 of them. What I did verify beforepublishing: the working tree is clean, the branch is 18 commits ahead
of
mainwith no merge conflicts, andcargo check --workspace --all-targets --all-featurescompletes with zero warnings and zeroerrors. CI's full 42-check matrix is the real gate here and runs on
this PR.
Two incidental observations from that check, neither affecting this
branch: an untracked
lossy_probe.rsexists in the working directorybut is not tracked and so cannot reach CI, and editor diagnostics
reporting dead code in
src/ops.rswere stale — no such warningreproduces against the committed tree.
Merge note
Each fix carries its own single
Fixes #Ntrailer rather than onemulti-issue trailer, so all nine issues auto-close on merge. A single
Fixes #a #b #cline would close only the first.