feat(pipeline): index-time importance scoring (weighted-degree) - #1883
Conversation
Score every Function/Method/Class node during indexing and persist the
result as a numeric "importance" key inside the node's existing
properties_json:
importance = sqrt(num_refs) * priv * generic * distinct * test_penalty
num_refs is the incoming CALLS + USAGE degree. The multipliers demote
private (leading-underscore) and generic names (defined in >= 5 distinct
files), promote distinctive identifiers (snake_case or camelCase, len >=
8), and demote test scaffolding -- symbols in a test file per the graph's
own cbm_is_test_path() classifier, or targets of an incoming TESTS edge.
Weighted degree only; a transitive (PageRank) refinement is deliberately
not built here until it can be shown to beat this on a fixed judgment
set.
No schema change and no CBM_INDEX_FORMAT_VERSION bump: the score lives in
the existing properties_json TEXT column, so no index is forced to
rebuild and indexes written by older builds simply lack the key.
Three failure modes this shape invites, all of them silent, are addressed
directly:
* Cost. The generic-name multiplier needs |{files a name is defined in}|.
Computing that per NODE with a pairwise file-path scan is O(k^3) in a
same-name group of size k -- ruinous on real corpora, where a single
Java name group reaches k > 4000. Here the count is computed once per
distinct NAME and memoized, and file paths are deduplicated through a
hash set, so total distinct-file work is linear in the graph. Measured
cold full index of a 666 MB Java corpus, same -O2 binary: 85 s with the
pass disabled, 80 s with this implementation, 481 s with the per-node
shape.
* Registration. PREDUMP_PASS_COUNT is now derived from the pass table
with sizeof instead of being hand-written. A hand-written count that
lags an appended entry silently skips the last-registered pass with
every test still green.
* Incremental. The incremental post-pass sequence runs the pass too, over
a graph rehydrated from the store whose nodes already carry the key.
The write-back overwrites an existing key in place instead of appending
a second one, which would otherwise corrupt properties_json into
{"importance":1.0,...,"importance":2.0}.
tests/test_importance.c covers the formula and binds all three failure
modes; the complexity suite gains a GATED counter asserting the pass's
same-name-group work stays linear (~2x, not ~4x) under a corpus doubling.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Co-authored-by: petercoxphoto <20230727+petercoxphoto@users.noreply.github.com>
|
Converting to draft — review turned up a correctness bug in the incremental wiring that I under-described in the PR body above. Recording it here rather than quietly fixing it, because the original framing was wrong. What the body says: that on the closure-delta route importance is computed over a partial graph, and that this matches a limitation Why that framing was too soft. The shared-buffer part is accurate — all those passes do run on the same buffer. But there is an asymmetry that matters: for edge-producing passes a partial graph yields fewer edges, a coverage gap. For importance it yields a wrong scalar that is then persisted over a correct one. A symbol referenced 500 times project-wide, sitting in a file you just edited, drops to near-zero. "Confidently unimportant" is a worse failure mode than "missing". Verified mechanism:
Impact today is nil — nothing reads the score yet; #880 is the consumer. That is exactly why it is worth fixing now rather than letting #880 inherit a signal that is wrong on the primary warm path. Fix in progress: recompute importance at SQL level over the staging store after Landing with it: a test asserting a full index and an incremental index that reach the same final state produce identical importance values (the guard against the two scoring implementations drifting), a closure-route regression test that asserts the route was actually taken so it cannot pass vacuously, the corrected comment, and re-measured Java timings so the added warm-path cost is visible rather than absorbed. Everything else in the PR stands as verified: the O(k³)→O(N) fix (483s → 80s), the structural |
…oute The closure-delta incremental route scored importance off a graph that does not contain the project's edges, and persisted the result. cbm_delta_preseed fills that route's gbuf with PROXY nodes -- id, label, name, qualified name, file path, and nothing else. Only the re-extracted files carry real edges; every inbound edge from an unchanged file is snapshotted and re-linked later, inside cbm_delta_patch. Scoring in run_postpasses therefore read an in-degree of zero for symbols with hundreds of real callers, and because the repaired files' rows are purged and re-inserted, cbm_delta_patch wrote those zeros to disk. Measured on a fixture whose `helper` has 25 callers: full index 5.000000, after one closure-delta re-index 0.000000. closure_try_plan runs first, so this is the preferred warm path, not an edge case. The comment at the call site claimed a rehydrated whole-project buffer; that is true only of the legacy-partial route, and it is now stated correctly. The fix rescores in SQL over the staging store, immediately after cbm_delta_patch has inserted the new nodes and re-linked the inbound edges -- the first moment on this route where the complete graph is queryable. Loading the full graph into RAM instead would have given back the point of the delta route. Rescoring is project-wide, not limited to the changed files. Distinct-files- per-name and in-degree are both GLOBAL inputs: a body-only edit in one file can change the score of a symbol defined in a file that did not change. A scoped rescore would leave those stale. TWO ROUTES, ONE RULE. The scoring rule and the JSON write-back are not reimplemented in SQL. cbm_pipeline_importance_score() and cbm_pipeline_importance_set_prop() are now single definitions that both routes call -- the SQL side reaches them through two SQLite user-defined functions, so the same C code runs either way and the rule cannot drift. SQL contributes only the three aggregates, each an indexed GROUP BY. What remains free is the input GATHERING, and tests bind that directly: a full index and a closure-delta incremental index converging on the same tree must produce identical scores for every symbol. Both new tests assert the route was actually taken, so neither can pass by silently running some other path. Cost, measured on a 666 MB Java corpus: cold full index unchanged at 83 s (the recompute does not run on that path); warm closure-delta re-index 20 s without the recompute, 26 s with it. The delta route keeps its advantage over a full reindex (26 s vs 83 s). Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
The delta-route bug flagged above is fixed, and the warm-path cost is now measured rather than estimated. The fixImportance is recomputed at SQL level over the staging store after Divergence is prevented by construction, not merely tested against. Why a changed-files-only rescore would be wrongThe equivalence test demonstrates it concretely. Editing That is √13 vs √12 — in-degree short by exactly the added caller, on a symbol in a file that did not change. Global inputs move when unrelated files are edited, so the rescore has to be project-wide. Measured cost — kernel, 8,529,900 nodes / 15,986,598 edgesCorpus held fixed for every leg, one
Corroborated three independent ways: wall-clock A/B, the recompute's own internal timing (20.5 / 20.7 s), and a standalone measurement on a separate kernel-scale DB (16.6-17.0 s). It scales sublinearly — Java at 693k nodes costs +6 s, so 12.3× the nodes yields 2.8× the cost, because the work is two indexed Accepted as the price of correct scores. Worth stating plainly: nothing reads this score until #880, so the cost lands before the benefit. Two properties the kernel run proved for free
Tests
Both assert the route was genuinely Known follow-upThe same scoring costs 2.2 s in-memory on the cold path vs ~20.6 s in SQL on the warm path. The cost is write volume, not computation — a 1.13M-row UPDATE where a one-file edit changes very few scores. Updating only rows whose value actually moved would preserve exact equivalence while cutting most of that. Filed separately; not attempted here. |
Distilled in-house from #879, with
Co-authored-by:credit to @petercoxphoto. The design — index-time weighted-degree importance — is theirs and was accepted on 2026-08-20. This is a fresh implementation against current main, because the original no longer applied (3 of 6 files conflicting).The blocker that had to be fixed first
The original
name_distinct_file_countwas cubic in the size of a same-name group. The kernel hides this — its worst C name ismainat k≈1064 — but Java does not. Measured onperf-bench/java(elasticsearch), one-O2binary, cold full index, back to back, node count identical at 693,254 in every leg:≈6.0x. The shipped version is free within run-to-run noise.
The fix computes the distinct-file count once per distinct name (memoized) and deduplicates paths through a hash set instead of pairwise
strcmp, taking the work from Σk³ per group to Σk = O(N).Three traps closed, each proved by revert
PREDUMP_PASS_COUNTis now structural —enum { PREDUMP_PASS_COUNT = (int)(sizeof(passes)/sizeof(passes[0])) }. Main had already grown a 7th pass; a hand-written= 7beside an 8-entry table would have left importance — registered last — silently never running, with every test green. Restoring= 7produces 3 failures."importance", so an append-only write yields duplicate JSON keys. Forcing append-only prints the real corruption:...,"importance":1.000000,"importance":1.000000}. The key is located in key position (preceded by{/,, followed by:) rather than bystrstr, which would also match text inside a string value.g_lsp_tail_*precedent is explicitly "recorded, deliberately NOT gated". Measuredimp_name_visitsratio: 1.92 linear vs 3.96 cubic, with the gate proved to fire. Two non-vacuity floors mean a skipped pass fails rather than passing on zeroed counters. Ratios only, never wall time.No reindex
CBM_INDEX_FORMAT_VERSIONis unchanged —src/store/store.his not in this diff at all.importanceis a JSON key inside the existingproperties_jsoncolumn: no schema change, and legacy indexes simply lack the key, so consumers must tolerate its absence.Honest scope note
Nothing reads this score yet.
compute_search_scoreis deliberately untouched; the consumer is #880. This lands the production of the signal, not its use.Verification (macOS, ASan+UBSan)
importance complexity pipeline→ 271 passed, 0 failed (11 + 5 + 255;pipelineis an exact baseline match).complexityis now 5/5; existing gates unmoved (nodes 1.93, edges 2.06, per-lang 1.98, perfile_defs 2.00).make -f Makefile.cbm lint-cipasses.fopen(in the diff. Every assertion is on state or counters — no timeout decides a verdict.Known limitations, recorded not hidden
similarity,semantic_edges,configlinkanddecorator_tagsalready carry exactly this limitation on that path; consistency was preferred over inventing new behaviour. Making the delta route exact is a separate design question.append_complexity_props()inpass_complexity.chas the identical append-only shape. It is safe today only because that pass is not wired into the incremental post-passes — if it ever is, it inherits the duplicate-key bug. Worth its own issue.cbm_is_test_path()still lacks a_test.c/_test.hcase, so the test-penalty under-fires on C (~640 kernel files unclassified). Pre-existing, deliberately out of scope, deserves its own issue.