fix(perf): consolidate extractor walks and scope native post-passes (v3.12.0 publish gate)#1436
Conversation
Fixes the two regressions that failed the v3.12.0 publish benchmark gate (native 1-file rebuild 86ms -> 1657ms +1827%; native full build +98%; wasm full build +95% vs 3.11.2): 1. JS/TS extractor had grown from 5 to 15 full-tree walks per file as each Phase 8.x feature added its own DFS. On WASM trees every node access marshals across the JS<->WASM boundary, so traversal count dominated extraction cost (+135%/file). Nine stateless walks are now one dispatch pass (runCollectorWalk) and the three class-context walks (typeMap, object-rest params, spread/for-of) are one context-tracking pass (runContextCollectorWalk) with per-concern context registers preserving each walk's distinct reset rules. extractReturnTypeMapWalk stays separate: the declarator handler reads the complete per-file map for inter-procedural propagation. 2. runPostNativePrototypeMethods ran unscoped on every native build: it read every JS/TS file in the repo, regex-filtered, and re-parsed all matches (18 files here) through the WASM worker pool -- a ~1s flat tax on every build including 1-file rebuilds. The definition scan is now scoped to changedFiles on incremental builds (mirroring runPostNativeThisDispatch); the caller-relink second pass still sweeps all files but only when a changed file actually added new func-prop definitions. Backfill parses for this pass and the this-dispatch pass also skip the worker-side AST/complexity/CFG/ dataflow visitors via the new symbolsOnly option (they only consume definitions/calls/typeMap). Local before/after (M-series, codegraph corpus, vs 3.11.2 baseline): native 1-file rebuild 1134-1232ms -> 99-104ms (baseline 58-59ms) native full build 3045-3238ms -> 2354-2576ms (baseline 1309-1474ms) wasm full build 14.4-15.9s -> 8.2-9.2s (baseline 7.0-7.8s) per-file extraction 17.7ms -> ~12ms (baseline 7.5ms) Remaining deltas vs 3.11.2 are the feature cost of the Phase 8.x resolution passes plus corpus growth, not bugs. Follow-ups: #1432 (native func-prop extraction removes the post-pass entirely), #1433 (per-PR perf canary), #1434 (post-pass phase timings), #1435 (worker-pool per-file overhead). Impact: 42 functions changed, 25 affected
|
Claude finished @carlos-alm's task —— View job PR Review: fix(perf): consolidate extractor walks and scope native post-passesTasks:
Root Cause AnalysisProblem Being Solved: Two independent performance regressions accumulated during Phase 8.x resolver PRs, invisible until v3.12.0's publish benchmark gate fired:
Why This Approach: Correct. The fix consolidates walks into Risk Assessment: Medium. The walk consolidation is a significant refactor of the hottest code path in the extractor. Correctness depends on precisely preserving the semantics of 12 individual walks that were separately authored and tested. The test suite passing (3042 tests, 176 resolution precision/recall fixtures) is strong evidence, but any missed edge case in the combined walk would silently regress extraction quality. Phase 2: Configuration Change DetectionNo lint rules relaxed, no test timeouts increased, no TypeScript strict mode changes. No config changes at all. Clean. Phase 3: Test IntegrityNo test files modified. No assertions weakened. The PR description cites 3042 passed / 0 failed with resolution fixture gate (176 passed). No red flags. Phase 4: Code Quality
Concern 1 — // parser.ts:1248-1251
if (filePaths.length <= INLINE_BACKFILL_THRESHOLD) {
return parseFilesWasmInline(filePaths, rootDir); // EXTRACT_ONLY never passed!
}
return parseFilesWasm(filePaths, rootDir, opts.symbolsOnly ? EXTRACT_ONLY : FULL_ANALYSIS);When Concern 2 — // javascript.ts:392-395 (query path)
runCollectorWalk(tree.rootNode, {
...
classMemberDefs: definitions, // same array reference as `definitions`
});In // javascript.ts:702-711 (walk path)
runCollectorWalk(tree.rootNode, {
definitions: ctx.definitions,
...
funcPropDefs: ctx.definitions,
// classMemberDefs: NOT passed
});The walk path's Concern 3 — case 'call_expression': {
if (targets.imports && !inDynamicImport && collectDynamicImport(node, targets.imports)) {
childInDynamicImport = true;
}
if (targets.calls && targets.thisCallBindings) {
collectThisCallAndBindings(node, targets.calls, targets.thisCallBindings);
}
collectParamBindings(node, targets.paramBindings); // runs even inside import() subtree
collectDefinePropertyReceiver(node, targets.definePropertyReceivers);The
Concern 4 — Caller-relink second pass ( const callerScanFiles = isFullBuild || !changedFiles ? jsFiles : allJsFilesFromDb();On an incremental build where a changed file added func-prop definitions, the caller-relink sweep calls Concern 5 — Minor: On the query path, Phase 5: Backlog Compliance
Tier: 1 (zero-dep ✓ + foundation-aligned ✓) Critical Concerns
Final Recommendation
|
Codegraph Impact Analysis42 functions changed → 25 callers affected across 11 files
|
Greptile SummaryThis PR addresses a v3.12.0 publish-gate benchmark regression by consolidating 15 independent AST walks into two unified passes (
Confidence Score: 5/5Safe to merge — the walk consolidation faithfully preserves the semantics of all 12 prior independent traversals, the scoped post-pass correctly handles both full and incremental paths, and the 3042-test suite passed at 0 failures. The refactoring is purely mechanical consolidation of stateless per-node handlers into shared DFS loops. Each collector's context-dependency (class stack resets, funcStack push/pop, dynamic-import subtree suppression) is carried over exactly, and the walk-path vs. query-path asymmetries (classMemberDefs, funcPropDefs, calls) are correctly represented in the new CollectorWalkTargets interface. The incremental-build scoping in the post-pass has the right fallback (caller-relink still sweeps all files when new method nodes are inserted), and the lazy allJsFilesFromDb() is called after the insert transaction commits. The known symbolsOnly limitation on the inline path predates this PR and is tracked separately. No files require special attention beyond the acknowledged symbolsOnly inline-path limitation in parser.ts (tracked as #1437). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[extractSymbolsQuery / extractSymbolsWalk] --> B[extractReturnTypeMapWalk\ncomplete per-file map for forward refs]
B --> C[runContextCollectorWalk\ntypeMap · objectRestParams · spread/forOf\nthree context registers with different reset rules]
B --> D[extractDestructuredBindingsWalk\nscope-pruning walk]
C --> E[runCollectorWalk\ndynamicImports · protoMethods · funcProps\nparamBindings · arrayElems · objectProps\nnewExprs · definePropertyReceivers\nthisCall/classMember on query path only]
F[tryNativeOrchestrator] --> G{isFullBuild?}
G -- yes --> H[allJsFilesFromDb\nscans ALL JS/TS files]
G -- no --> I[changedFiles.filter\nscans only changed files]
H --> J[regex pre-filter\nfunc-prop pattern match]
I --> J
J --> K[parseFilesWasmForBackfill\nsymbolsOnly:true on pool path]
K --> L{new method nodes\ninserted?}
L -- no --> M[return]
L -- yes --> N[build newMethodSuffixes]
N --> O[callerScanFiles\nfull build: jsFiles\nincremental: allJsFilesFromDb]
O --> P[text-search callers\nregex suffix match]
P --> Q[parseFilesWasmForBackfill\nsymbolsOnly:true]
Q --> R[resolve call edges\nto new method nodes]
Reviews (4): Last reviewed commit: "Merge branch 'main' into fix/postpass-sc..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export async function parseFilesWasmForBackfill( | ||
| filePaths: string[], | ||
| rootDir: string, | ||
| opts: { symbolsOnly?: boolean } = {}, | ||
| ): Promise<Map<string, ExtractorOutput>> { | ||
| if (filePaths.length <= INLINE_BACKFILL_THRESHOLD) { | ||
| return parseFilesWasmInline(filePaths, rootDir); | ||
| } | ||
| return parseFilesWasm(filePaths, rootDir); | ||
| return parseFilesWasm(filePaths, rootDir, opts.symbolsOnly ? EXTRACT_ONLY : FULL_ANALYSIS); | ||
| } |
There was a problem hiding this comment.
symbolsOnly is silently ignored on the inline path
When filePaths.length <= INLINE_BACKFILL_THRESHOLD (16), parseFilesWasmInline is called with no analysis argument and always performs a full extraction — AST/complexity/CFG/dataflow visitors run regardless of opts.symbolsOnly. For a 1-file rebuild, protoFiles will typically be 1 file, so symbolsOnly: true is a no-op on the exact path the PR targets. The benchmark gains are real (they come from file scoping), but the symbolsOnly optimization contributes nothing to incremental builds in practice. Callers passing { symbolsOnly: true } with small batches get no benefit, and there is no diagnostic to indicate this.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Acknowledged. The symbolsOnly flag is indeed silently ignored on the inline path (parseFilesWasmInline) for batches of ≤16 files. Fixing this properly requires adding an analysis-mode parameter all the way down to the per-language extractor functions, which is a meaningful separate change.
Filed as #1437 to track this follow-up. The benchmark wins from this PR are real — they come from restricting the proto-methods post-pass definition scan to changedFiles, not from symbolsOnly.
…dows CI The embedding-regression test skips when the HuggingFace model download fails due to network issues. However, on Windows CI the failure surfaces as a ConnectTimeoutError (code: UND_ERR_CONNECT_TIMEOUT) wrapped inside EngineError as 'fetch failed' — neither 'timeout' in the message nor the UND_ERR_ code was checked. Walk the full cause chain so nested timeouts and undici socket errors are treated as network failures and silently skipped rather than failing the test run.
|
Addressed review feedback:
|
|
Windows CI failures — transient infrastructure issue The three failing checks (Test Node 22 windows-latest, Engine parity windows-latest, CI Testing Pipeline) are unrelated to this PR's code changes. Root cause: The CI run was scheduled on a Evidence that this is infrastructure-only:
|
1 participant
Why
The v3.12.0 publish failed its benchmark regression gate (run 27260875859):
Not CI variance — two real regressions accumulated across the Phase 8.x resolver PRs since 3.11.2 (invisible until release because benchmarks only run at publish — #1433):
Root cause 1 — extractor walk pile-up (both engines' parse cost)
The JS/TS extractor grew from 5 to 15 full-tree walks per file; each feature PR added its own DFS. On WASM trees every node access (
.type,child(i),childForFieldName) marshals across the JS↔WASM boundary, so traversal count dominates extraction cost. Measured +135%/file (7.5 → 17.7ms on this corpus); CPU profile self-time was dominated bymarshalNode/setValue.Root cause 2 — unscoped native post-pass (native 1-file rebuild 20×)
runPostNativePrototypeMethods(#1331) ran on every native build: read all ~590 JS/TS files, regex-filtered, WASM-re-parsed all 18 matches through the worker pool — ~1s flat tax per build. On a 1-file rebuild that's 46ms of real work + ~1.1s of post-pass (result.phasessummed 48ms while wall was 1232ms — #1434).What
runCollectorWalk— one dispatch pass replacing 9 stateless walks (dynamic imports, this/call/apply bindings, param/array-elem/object-prop bindings,new X()names, defineProperty receivers, class members, prototype + func-prop assignments). Per-walk semantics preserved exactly (incl. the import()-subtree suppression and the TStype_identifierclassStack quirk — see comments).runContextCollectorWalk— one context-tracking pass replacing the 3 class-context walks (typeMap, object-rest params, spread/for-of), each keeping its own context register because their reset rules deliberately differ.extractReturnTypeMapWalkstays separate and runs first: the declarator handler reads the complete per-file map for inter-procedural propagation (forward references).changedFileson incremental builds (mirrorsrunPostNativeThisDispatch); the caller-relink sweep still covers all files but only fires when a changed file actually added func-prop definitions.symbolsOnlybackfill parses — the proto and this-dispatch passes skip worker-side AST/complexity/CFG/dataflow visitors + their serialization (they only consume definitions/calls/typeMap). Dropped-language backfill keeps full analysis.Numbers (local M-series, codegraph corpus, vs 3.11.2 baseline)
Remaining deltas vs 3.11.2 are the genuine cost of the new Phase 8.x resolution features plus corpus growth (628 → 670 files), not bugs. If the publish gate still trips on native full build, the residual is root-caused above and the next lever is #1432 (native func-prop extraction deletes the post-pass entirely).
Verification
buildGraphfull/no-op/1-file for both engines on this repo vs a v3.11.2 worktree, same machineFollow-ups filed
#1432 (Rust func-prop extraction), #1433 (per-PR perf canary + benchmark methodology unification), #1434 (post-pass phase timings), #1435 (pool per-file overhead)
After merge: re-run the v3.12.0 publish workflow.