perf(search): early-exit synthetic verify + pool cap for --ast <pattern> (#394 AC11/#419)#420
Open
dean0x wants to merge 3 commits into
Open
perf(search): early-exit synthetic verify + pool cap for --ast <pattern> (#394 AC11/#419)#420dean0x wants to merge 3 commits into
dean0x wants to merge 3 commits into
Conversation
…euse (#394) Standalone `--ast` returned zero results for all 5 synthetic-marker patterns (god-function, deep-nesting, empty-function, empty-catch, excessive-params). Root cause: vocab_lookup never yields a synthetic ID (>= BUCKET_LABEL_BASE), making the strict real-CST ancestor walk in pattern_occurs_in_file structurally unsatisfiable for them — while the compound path (which skips this gate) returned matches on the same index, a standalone/compound contradiction (avoids PF-006). - pattern_occurs_in_file (AD-394-1/AD-394-2): route synthetic-containing patterns through re-running the index-time pipeline (linearize_source + extract_ast_ngrams_with_metrics) and checking every resolved n-gram key is present in the emitted set; real-node patterns keep the strict walk unchanged, preserving AD-374-6 precision (applies ADR-006: indexer is the single source of truth). - recover_line (AD-394-5, OD-394-1 resolved to recover now): synthetic patterns recover a real representative :line via the same extraction pass instead of degrading to a path-only row. - LinearNode gains transient start_line/start_byte (AD-394-6; PF-004: widen usize->u32 before saturating_add(1); no persisted-format change, FORMAT_VERSION stays 2 — AC12). - is_synthetic_id (AD-394-3) is the single source of truth for the `>= BUCKET_LABEL_BASE` predicate, re-exported crate-wide; a new AC8 test asserts no catalog pattern mixes real + synthetic n-grams. Ground-truth line values and the standalone/compound parity checks (AC1-AC13) were verified via release-binary dogfood before being hardcoded into tests. Filed #419 to track a discovered perf gap: the synthetic verify branch is ~260x slower than the real-node gate for low-selectivity patterns like deep-nesting on a real corpus (6.9s vs 26ms) — correctness is unaffected; deferred since a proper fix needs its own design. Co-Authored-By: Claude <noreply@anthropic.com>
Replace the mutable-accumulator loop with a min_by_key iterator chain — same semantics (first/topmost line wins on tie, matching AC-F3), half the lines, no nested Some(match …).
…ttern> (#394 AC11/#419) Replace full O(n) extract_ast_ngrams_with_metrics in the synthetic-marker verify gate (pattern_occurs_in_file) with synthetic_key_present — an early-exit variant using the same ExtractState emission logic (ADR-006 single source of truth) that returns as soon as the target bigram key is confirmed present. For deep-nesting (DEEP_NODE → bucket_label(0) fires at the first node at CST depth >= 4), this exits on the very first qualifying node instead of traversing the full node list. Companion fix: reduce the candidate pool from max(5*limit, 100) to limit.max(1) for synthetic patterns via ast_query_is_synthetic — synthetic verify has near-zero drop rate so the 100-file floor was pure overhead. Together: measured --ast deep-nesting latency drops from ~6.9s to within AC11's <500ms target and the same order of magnitude as --ast try-catch. Also removes the now-dead all_ngrams_present_in method from AncestorMatchTable (compile-enforced dead-code elimination). Unit test: ac11_394 classifies all 5 synthetic patterns as synthetic, real-node patterns as real, and containment queries as real. Closes AC11 of #394; fix tracked as #419.
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.
Summary
Resolves the single failing acceptance criterion from the #394 Evaluator: AC11 (performance) was unmet because standalone
--ast <synthetic-pattern>(deep-nesting, god-function, empty-function, empty-catch, excessive-params) was measured at ~6.9s on skim's own repo vs. the 500ms target (260x slower than--ast try-catch). Two root causes were fixed in tandem:CANDIDATE_POOL_FLOOR=100was applied uniformly, so even a--limit 10synthetic query fetched and gated 100 candidates instead of 10.extract_ast_ngrams_with_metrics(complete O(n) traversal) for each candidate; fordeep-nesting(DEEP_NODE → bucket_label(0)) the match fires at the first node at depth ≥ 4, so the traversal was doing needless work.Changes
crates/rskim-search/src/ast_index/extract.rs: Newsynthetic_key_present(nodes, lang, target: AstBigram) -> bool— sameExtractStatestate machine asrun_extraction(ADR-006 single-source), exits immediately whentargetis found inbigram_map. Skipsemit_real_ngramsandupdate_branch_count(synthetic IDs cannot appear in real n-gram slots; correct per the BUCKET_LABEL_BASE contract).crates/rskim-search/src/compound/reparse.rs: The synthetic branch (lines 366–393) now callslinearize_source+synthetic_key_presentinstead of the full extraction path, preserving the early-exit property inside the per-file verify gate.crates/rskim/src/cmd/search/ast.rs: Newast_query_is_synthetic(query) -> boolhelper;run_ast_standalonesizes the candidate pool tolimit.max(1)for synthetic queries instead ofmax(K×limit, floor=100).crates/rskim/src/cmd/search/ast_tests.rs: AC11 testast_query_is_synthetic_classifies_correctly_ac11_394asserts all 5 synthetic patterns → true, real-node patterns + containment → false.Breaking Changes
None. FORMAT_VERSION stays 2. The verify gate call site in
run_ast_standaloneis unchanged; routing is internal topattern_occurs_in_file.Reviewer Focus Areas
synthetic_key_present: sameExtractStatehelpers (emit_depth_buckets,close_open_subtrees,fill_depth_gap,update_child_count,record_node) asrun_extraction— drift is structurally impossible because both paths share the same methods.emit_depth_buckets(fires for deep-nesting on first deep node) AND afterclose_open_subtrees(fires for god-function/empty-body/excessive-params when a subtree closes).ast_query_is_syntheticusesis_synthetic_idfrom the crate — same predicate asAncestorMatchTable::contains_synthetic_idinreparse.rs, no duplicate threshold.Closes #419 (the deferred AC11 follow-up filed in the original implementation commit).
Co-Authored-By: Claude noreply@anthropic.com