Ripwire Optimization Remarks Triage

Triaging clang optimization remarks in this repo

Nearest neighbours: • A measured slow operation, any codebase → ripwire-perf-target. Do that FIRST; remarks are the second question, not the first. • Maintenance risk / complexity in a subsystem → ripwire-fresh-eyes. Different axis entirely.

Full record of the pass this skill came from, with every number: docs/OPTREMARKS.md.

The one rule

A remark is an observation, not a defect. Clang is reporting a decision, usually the right one. The pass that produced this skill read ~1.1 M remarks over src/. Zero of them justified a source edit. Two justified a BUILD change (LTO, then PGO), and the best-looking source-level candidate was implemented, verified to do exactly what the remark asked, measured, and reverted. Budget your attention accordingly: on this codebase, the answer to a remark is far more often a compiler flag than a diff.

Step 0 — profile first, or you will optimize a millisecond

cmake -S . -B build_prof -DRIPWIRE_PROFILE=ON && cmake --build build_prof -j 6
./build_prof/ripwire . --no-cache 2>&1 >/dev/null | sed -n '/hottest scopes/,/PROF_TSV/p'

On this codebase the answer is stable and counterintuitive: PageRank is ~1 ms of a ~2.7 s cold CPU profile, the build-model sorts are ~1.7 ms, and ~29% sits in two phases now living in src/ingest_sidecap.h (captureTagsFacts, captureSideFacts) that spend it calling tree-sitter. So a remark in src/pagerank.cpp, src/infra/sortutil.h or src/infra/radixSort.inl is dismissible on arithmetic before you read it. Re-derive this table if you touch the pipeline; do not trust this paragraph forever.

Step 1 — collect

scripts/optremarks.sh --passes 'inline|loop-vectorize|slp-vectorizer|licm|gvn|.*unswitch|loop-idiom'
python3 scripts/optremarks.py --hot --top 40

--hot narrows to a literal list (HOT_FILES) rather than a heuristic, so what the report calls "hot" stays reviewable. That list once went stale silently — the ingest split moved the hot phases into src/ingest_*.h sections of the same TU, every listed path still existed, and --hot quietly fell to 0.2% coverage of that TU. test/optremarkshotcheck.sh now gates the list against the tree (docs/OPTREMARKS.md §8). If you split or rename a hot file, run that gate: it will tell you which files you now owe a decision on, and COLD_FILES is where a deliberate exclusion goes, with its reason.

Never in build/ — CMake refuses it by name, because build/ripwire is the binary every gate and bench number is measured against. Never with a build type: RIPWIRE_ARCH_FLAGS is already -O2, and Release would define NDEBUG and blind the degrade-path gates. Run wide once to learn which classes fire (src/main.cpp unfiltered exceeds 800 MB of YAML), then narrow.

Step 2 — the remark classes, and what each one is worth here

Real signal

Missed inline NoDefinition, callee in another TU that you own the build of. Meaning: the definition is not visible, full stop — not a cost-model opinion. Confirmed here: 831 of 1,437 distinct NoDefinition sites in the ingest translation unit name a tree-sitter C accessor (ts_node_child_by_field_name, ts_node_is_null, ts_node_type, …), each a two-line function compiled into a separate C object, inside the phases that are ~29% of a cold run. Fix pattern: make the definition available — link-time optimization, not a source edit. Measured: -DRIPWIRE_LTO=ON, four interleaved A/Bs (9/21/21/31 runs per arm) → cold 1–6% faster, warm 0–3%, every cold statistic in every run favouring LTO; output byte-identical, build time +2.6×. Now the default (-DRIPWIRE_LTO=OFF for a fast edit loop). Note the shape of that claim: four runs put the cold median anywhere from −0.8% to −5.9%, so the honest number is the range and the unanimous direction, not the best run. Tell it from noise: if the callee is libc (fopen, fread, stat) the same remark is worthless — inlining a syscall wrapper saves nothing. The class is only interesting when the callee is tiny and yours to build.

Missed gvn LoadClobbered … clobbered by call, in bulk, in one function. Meaning: fires per load per pass; 90,971 of them in this record. Useless individually. Fix pattern: there isn't one for a single site. Its value is aggregate — a dense cluster in one function means that function is call-bound, and that is how the NoDefinition finding above was located. Read it as a heat map, never as a work item.

Restated algorithm — dismiss as a class

loop-vectorize early-exit family: TooManyUncountableEarlyExits, PotentiallyFaultingEarlyExitLoop, CantComputeNumberOfIterations, UnsupportedUncountableLoop, LoopContainsUnsupportedSwitch, WritesInEarlyExitLoop. Confirmed here: hundreds, concentrated in src/docparse.h, src/arch.h, src/lexical.h, src/graph.h, src/infra/radixSort.inl:402. Every one is a scanner that breaks on a delimiter. A loop whose trip count depends on the bytes it is reading is uncountable by construction. There is nothing to fix without changing the algorithm.

Missed inline NeverInline naming __clang_call_terminate. 275 in the hot set. The exception-landing-pad helper; noinline is deliberate and it never runs on a hot path. Filter first, every time.

Missed loop-vectorize MissedDetails. Always paired with an Analysis row that gives the actual reason. Read the Analysis row; the Missed row carries no information of its own.

Missed gvn LoadClobbered … in favor of store … clobbered by store on a histogram. Confirmed here: src/infra/radixSort.inl:411-413, the 4×-unrolled counting loop — the compiler cannot prove digit0 != digit1, so each ++hist[pass][d] reloads. Real, and it has a textbook fix (private per-lane histograms, summed after). Dismissed on the profile, not on the analysis: the sorts are 1.7 ms. Note the shape — "the remark is right and the fix is known and it still is not worth doing" is a legitimate, common verdict.

Real but not worth it — the calibration case

Missed licm LoadWithLoopInvariantAddressInvalidated + gvn LoadClobbered on an address-escaped struct in a call-heavy loop. Confirmed here: src/ingest_sidecap.h:1428, for( uint16_t ci = 0; ci < match.capture_count; ++ci ) over match.captures[ci]. match had its address taken by ts_query_cursor_next_match, so every accessor call in the body clobbers it: the trip count and the capture base were reloaded on every iteration, inside the single hottest own-code loop in the tool. Fix pattern (the textbook one): hoist to locals before the loop — const uint16_t captureCount = match.capture_count; const TSQueryCapture* const captures = match.captures; What happened: the remark moved exactly as predicted (from the inner loop's line to the hoist's line — the load became per-match instead of per-capture). The wall clock did not:

Direction flips between runs ⇒ noise. Reverted. The reason: the reload sits immediately beside an opaque call that costs orders of magnitude more. This is the most useful entry in this file — if you are about to hoist a load out of a loop whose body calls into another translation unit, expect this outcome and measure before you commit to the diff.

Missed inline TooCostly on libc++ (basic_string::push_back, vector::push_back, unordered_dense::table::…). The cost model declining to inline into a large caller. Occasionally worth chasing; every hot-set instance here was in src/docparse.h (document extraction, off the default path) or one-shot setup in buildGraph. Dismissed on location, not on principle — check where yours is before you copy this verdict.