tstypes: partition the streamed-apply fact spool by class - #530
Conversation
zzet
left a comment
There was a problem hiding this comment.
@pbednarcik thank you for the performce fix.
A few points work notice (at least for follow up):
-
pageClass(fact_spool.go:341-382) never callsrows.Err(). The pre-PRpage()did, and both sibling functions added in this same PR (pageFiles,attachImports) do. database/sql'sRows.Close()returns the driver's close error, not the iteration error — so a mid-iteration failure is swallowed. An error on the firstNext()yields an empty page, which the driver loop in provider_stream.go reads as "phase complete," silently skipping that phase's remaining facts with no error surfaced. Low probability (local temp SQLite, single connection), but it's a silent-data-loss shape and a one-line fix. CI couldn't catch it: .golangci.yml uses default: standard (errcheck/govet/ineffassign/staticcheck/unused) — rowserrcheck isn't enabled. -
partial progress under the pass deadline.
applyStagedFactsruns under applyCtx, which carries the movable deadline. The new coverage walk is now the first thing under that context and mutates nothing; pre-PR the first work was the supers phase, which flushed edges page by page. So a deadline expiry that previously left some supers edges applied now yieldsEdgesAdded == 0. The comment atprovider.go:100-107records a prior incident in exactly that shape ("a partial with zero coverage"), which is what the movable deadline exists to prevent. The walk is decode-free, pre-warms the hot cache all four phases hit, and the pass got 3.3× faster overall — so hitting the deadline at all is now much less likely. What degrades is only the quality of the partial when it does. -
file_facts's PK is (class, file_path) WITHOUT ROWID, so WHERE file_path = ? has no usable index prefix. I confirmed with EXPLAIN QUERY PLAN:
legacy-arm : SCAN file_facts (full table scan)
partitioned : SEARCH file_facts USING PRIMARY KEY (class=? AND file_path>?)
At 512 files × 4 phases that's ~2048 full table scans. The 296→32 ms headline is dominated by O(N²) scan cost, not decode volume. The PR body discloses only "point-query overhead," which materially understates it. The production A/B (527.7s → 154.1s) is measured on the real path and stands.
-
The byte cap counts only the class payload, not side-fetched imports, so peak page bytes can modestly exceed the stated 4 MiB.
-
appendFilessizes its slices for 4 classes per file when there are 5 — capacity hint only. Bound params per chunk rise 192 → ~1280, still far under SQLite's 32766.
Problem
On a cold index of my production C# repo (≈117k graph nodes), the csharp-types provider dominated the enrichment phase: 527.7 s of a 532.3 s phase. The streamed apply walks its fact spool four times — once per phase (supers, calls, aliases, coverage) — and each walk decoded every file's full fact payload, regardless of which facts the phase actually consumes. Fact classes are heavily skewed (calls dominate every file, inheritance facts exist in a fraction of files, aliases are absent entirely in C#), so most of that decode volume was thrown away.
Change
Partition the per-pass temp spool by fact class:
filesrow per file plus onefile_factsrow per non-empty class — a file with no aliases contributes no aliases row at all, which is what lets a phase skip files wholesale rather than decode-and-discard.buildIndexneeds the import map in every phase).tstypes: apply hot cache), so the cache's behavior is visible in production runs, not just tests.The spool is per-pass and never outlives it; class ids are stable row keys, nothing persisted.
Numbers
Microbench (committed,
fact_spool_bench_test.go, class-skew shaped like the production repo): four-phase walk 296 → 32 ms, allocations 131 → 33 MB. Caveat honored in the bench comment: the legacy model queries per file where the old code scanned 32-file pages, so read the gap as decode volume (~4×) plus point-query overhead, not decode alone.Production A/B (idle machine, from-scratch index, same repo):
Outcome counters digit-identical across runs: confirmed 9798 / added 114 / nodes enriched 41234 / coverage identical to 14 decimal places. Apply hot cache at pass end: 87% node / 86% adjacency hit.
Disclosures
apply_golden_test.gopins the full streamed pipeline (real extractors, partial-type fixture in the Razor-codebehind shape) against a committed snapshot; regenerate only deliberately with-update. It deliberately pins a pre-existing partial-class cross-part resolution gap as-is — that's a separate correctness item, not something this PR touches.Tests
Full tstypes suite green, including the new golden and the streamed-vs-whole parity oracle — re-verified after rebasing onto the sqlite-only backend transition (#473), since the storage substrate changed underneath. Repo-wide failure set vs clean main is identical on my Windows box (pre-existing platform bucket only, zero new). Five consecutive production cold runs with this code produced digit-identical outcome counters.