perf: skip ensureWasmTrees when native engine provides complete data#344
perf: skip ensureWasmTrees when native engine provides complete data#344carlos-alm merged 5 commits intomainfrom
Conversation
Greptile SummaryThis PR lifts the native-engine optimisation to the shared pre-parse step: before calling Key findings:
Confidence Score: 3/5
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Exclude constant nodes — the native engine has a known scope bug where it | ||
| // extracts local `const` variables inside functions as top-level constants, | ||
| // while WASM correctly limits constant extraction to program-level declarations. | ||
| const nodes = db | ||
| .prepare('SELECT name, kind, file, line FROM nodes ORDER BY name, kind, file, line') | ||
| .prepare( | ||
| "SELECT name, kind, file, line FROM nodes WHERE kind != 'constant' ORDER BY name, kind, file, line", | ||
| ) | ||
| .all(); | ||
| const edges = db | ||
| .prepare(` | ||
| SELECT n1.name AS source_name, n2.name AS target_name, e.kind | ||
| FROM edges e | ||
| JOIN nodes n1 ON e.source_id = n1.id | ||
| JOIN nodes n2 ON e.target_id = n2.id | ||
| WHERE n1.kind != 'constant' AND n2.kind != 'constant' | ||
| ORDER BY n1.name, n2.name, e.kind | ||
| `) |
There was a problem hiding this comment.
The WHERE kind != 'constant' exclusion handles the native engine scope bug where function-scoped const declarations are wrongly extracted as top-level constants. That bug is fixed in this PR's javascript.rs change. However, the test exclusion has no mechanism to be re-enabled once the 3.0.3 native binary is published — constant-node parity will never be verified even after the fix is live.
Consider adding a TODO comment referencing the binary version at which this exclusion should be removed:
// TODO: Remove this exclusion once the native binary >= 3.0.3 is published.
// The fix is in crates/codegraph-core/src/extractors/javascript.rs (find_parent_of_types guard).There was a problem hiding this comment.
Fixed in bc3b58c — added TODO comment referencing native binary >= 3.0.4 as the threshold for removing the kind != 'constant' exclusion.
CHANGELOG.md
Outdated
| ## [3.0.3](https://github.com/optave/codegraph/compare/v3.0.1...v3.0.3) (2026-03-04) | ||
|
|
||
| ### Performance | ||
|
|
||
| * **ast:** use single transaction for AST node insertion — `astMs` drops from ~3600ms to ~350ms (native) and ~547ms (WASM), reducing overall native build from 24.9 to 8.5 ms/file ([#333](https://github.com/optave/codegraph/pull/333)) | ||
|
|
||
| ## [3.0.1](https://github.com/optave/codegraph/compare/v3.0.0...v3.0.1) (2026-03-03) |
There was a problem hiding this comment.
The 3.0.3 entry only documents the AST single-transaction optimization. Two other significant changes shipped in this release are missing:
src/builder.js: skipensureWasmTreeswhen native engine provides complete CFG + dataflow datacrates/codegraph-core/src/extractors/javascript.rs: fix function-scopedconstdeclarations being incorrectly extracted as top-level constants
Additionally, the version jumps from 3.0.1 directly to 3.0.3 with no 3.0.2 entry. A note explaining why 3.0.2 was skipped would help future maintainers.
Consider updating the entry to include all three changes and clarifying the version gap.
There was a problem hiding this comment.
Fixed in bc3b58c — added the two missing changelog entries (ensureWasmTrees skip and const scope fix) and a note about 3.0.2 being internal/unpublished.
|
Addressed both review comments in bc3b58c:
|
src/builder.js
Outdated
| const fnDefs = symbols.definitions.filter( | ||
| (d) => (d.kind === 'function' || d.kind === 'method') && d.line, | ||
| ); |
There was a problem hiding this comment.
Missing null guard on symbols.definitions
symbols.definitions.filter(...) will throw a TypeError if a symbols object has definitions: undefined or definitions: null. This can happen for binary or unsupported files whose extractor returns a partial object, and for language-specific extractors that may omit definitions under error conditions.
Every other place in the codebase that accesses symbols.definitions uses a || [] guard (e.g. src/ast.js:171 — const defs = symbols.definitions || []). This new code path is the only exception, which means a single bad file can abort the entire build at the pre-parse check rather than silently skipping.
| const fnDefs = symbols.definitions.filter( | |
| (d) => (d.kind === 'function' || d.kind === 'method') && d.line, | |
| ); | |
| const fnDefs = (symbols.definitions || []).filter( | |
| (d) => (d.kind === 'function' || d.kind === 'method') && d.line, | |
| ); |
There was a problem hiding this comment.
Fixed in cb4a6c3 — changed to (symbols.definitions || []).filter(...), matching the guard pattern used everywhere else in the codebase.
|
Claude finished @carlos-alm's task —— View job PR Review AnalysisReviewing PR #344: Performance optimization to skip WASM parsing when native engine provides complete data. Todo List:
Phase 1: Root Cause Analysis ✅Problem Being Solved: Performance optimization to avoid redundant WASM parsing when the native engine already provides complete CFG and dataflow data. This addresses unnecessary work in builds that use the native engine. Why This Approach: The solution adds a pre-check loop that examines Risk Assessment: Low risk - the optimization is conservative (only skips when data is definitely complete) and maintains backward compatibility. Phase 2: Configuration Change Detection ✅No problematic configuration changes detected. The changes are purely algorithmic optimizations in Phase 3: Test Integrity ✅Test integrity is maintained. The author addressed previous review feedback by:
Phase 4: Code Quality Assessment ✅CRITICAL BUG CONFIRMED: The review comment from Greptile is correct. At Evidence of Inconsistent Pattern:
This inconsistency suggests the new code path doesn't follow the established safety pattern used elsewhere in the codebase. Other Code Quality Observations:
Root Cause AnalysisProblem Being Solved: Performance bottleneck where Why This Approach: The pre-check heuristic is logical and conservative - it only skips WASM parsing when it can definitively confirm that native data is complete for all required functionality (CFG blocks for functions/methods and dataflow data). Risk Assessment: Low risk optimization with proper fallback behavior and accurate timing metrics. Critical Concerns❌ NULL SAFETY BUG: Line Required Fix: const fnDefs = (symbols.definitions || []).filter(
(d) => (d.kind === 'function' || d.kind === 'method') && d.line,
);Final Recommendation
The fix is trivial but essential - all other aspects of the PR are acceptable. |
|
Claude finished @carlos-alm's task —— View job Claude is reviewing PR #344 Todo List:
Phase 1: Root Cause Analysis ✅Problem Being Solved: Performance optimization to avoid redundant WASM parsing when the native engine already provides complete CFG and dataflow data. This addresses unnecessary work in builds that use the native engine. Why This Approach: The solution adds a pre-check loop that examines Risk Assessment: Low risk - the optimization is conservative (only skips when data is definitely complete) and maintains backward compatibility. Phase 2: Configuration Change Detection ✅No problematic configuration changes detected. The changes are purely algorithmic optimizations in Phase 3: Test Integrity ✅Test integrity is maintained. The author addressed previous review feedback by:
Phase 4: Code Quality Assessment ✅CRITICAL BUG CONFIRMED: The review comment from Greptile is correct. At Evidence of Inconsistent Pattern:
This inconsistency suggests the new code path doesn't follow the established safety pattern used elsewhere in the codebase. Other Code Quality Observations:
Root Cause AnalysisProblem Being Solved: Performance bottleneck where Why This Approach: The pre-check heuristic is logical and conservative - it only skips WASM parsing when it can definitively confirm that native data is complete for all required functionality (CFG blocks for functions/methods and dataflow data). Risk Assessment: Low risk optimization with proper fallback behavior and accurate timing metrics. Critical Concerns❌ NULL SAFETY BUG: Line Required Fix: const fnDefs = (symbols.definitions || []).filter(
(d) => (d.kind === 'function' || d.kind === 'method') && d.line,
);Final Recommendation
The fix is trivial but essential - all other aspects of the PR are acceptable. |
|
Addressed the null guard comment in cb4a6c3 — added (symbols.definitions || []) guard. |
src/builder.js
Outdated
| const fnDefs = (symbols.definitions || []).filter( | ||
| (d) => (d.kind === 'function' || d.kind === 'method') && d.line, | ||
| ); | ||
| if (fnDefs.length > 0 && !fnDefs.every((d) => d.cfg?.blocks?.length)) { |
There was a problem hiding this comment.
Empty blocks array treated as missing native CFG
d.cfg?.blocks?.length evaluates to 0 (falsy) when the native engine returns cfg: { blocks: [] } for trivial functions (e.g., empty body or single return). This causes !fnDefs.every(d => d.cfg?.blocks?.length) to be true, spuriously triggering needsWasmTrees = true even though native CFG was provided.
In practice, any file containing a trivially simple function prevents the skip from firing, silently negating the optimization. cfg.js line 1151 also skips such functions, but that doesn't help here—the builder check should recognize empty-blocks as "native CFG was computed, just returned empty."
Consider using Array.isArray(d.cfg?.blocks) instead, which correctly treats an explicitly-set empty-blocks array as "native CFG was provided":
| if (fnDefs.length > 0 && !fnDefs.every((d) => d.cfg?.blocks?.length)) { | |
| if (fnDefs.length > 0 && !fnDefs.every((d) => Array.isArray(d.cfg?.blocks))) { |
There was a problem hiding this comment.
Fixed in b6a7699 — changed the check from d.cfg?.blocks?.length (falsy for empty arrays) to d.cfg === null || Array.isArray(d.cfg?.blocks). This correctly treats cfg: { blocks: [] } as valid native CFG while preserving cfg: null for nodes that intentionally have no CFG (e.g. interface members).
carlos-alm
force-pushed
the
perf/skip-wasm-preparse-native
branch
from
cb4a6c3 to
bf8de1f
Compare
Before calling ensureWasmTrees, check whether native engine already supplies CFG and dataflow data for all files. When it does, skip the WASM pre-parse entirely — avoiding a full WASM parse of every file on native builds where the data is already available. Impact: 1 functions changed, 14 affected
…omplete changelog
Impact: 1 functions changed, 0 affected
d.cfg?.blocks?.length evaluates to 0 (falsy) when the native engine
returns cfg: { blocks: [] } for trivial functions, spuriously triggering
needsWasmTrees. Use Array.isArray(d.cfg?.blocks) instead, and preserve
d.cfg === null for nodes that intentionally have no CFG (e.g. interface
members).
Impact: 1 functions changed, 1 affected
Impact: 1 functions changed, 0 affected
carlos-alm
force-pushed
the
perf/skip-wasm-preparse-native
branch
from
b6a7699 to
f3700aa
Compare
|
Claude finished @carlos-alm's task —— View job Claude is reviewing PR #344 Todo List:
Phase 1: Root Cause Analysis ✅Problem Being Solved: Performance optimization to avoid redundant WASM parsing when the native engine already provides complete CFG and dataflow data. This addresses unnecessary work in builds that use the native engine. Why This Approach: The solution adds a pre-check loop that examines Risk Assessment: Low risk - the optimization is conservative (only skips when data is definitely complete) and maintains backward compatibility. Phase 2: Configuration Change Detection ✅No problematic configuration changes detected. The changes are purely algorithmic optimizations in Phase 3: Test Integrity ✅Test integrity is maintained. The author addressed previous review feedback by:
Phase 4: Code Quality Assessment ✅✅ NULL SAFETY FIXED: The critical null guard issue at ✅ EMPTY BLOCKS ARRAY HANDLING FIXED: The empty blocks array issue has been resolved in commit b6a7699. The logic now correctly uses Other Code Quality Observations:
Root Cause AnalysisProblem Being Solved: Performance bottleneck where Why This Approach: The pre-check heuristic is logical and conservative - it only skips WASM parsing when it can definitively confirm that native data is complete for all required functionality (CFG blocks for functions/methods and dataflow data). Risk Assessment: Low risk optimization with proper fallback behavior and accurate timing metrics. Final Recommendation
The PR demonstrates good engineering practices with thorough review response and maintains high code quality standards. Minor deduction for the manual test plan items remaining unchecked, but the optimization logic is solid and all automated tests pass. |
1 participant
Summary
ensureWasmTrees, checks whether native engine already provides CFG and dataflow data for all files_t.wasmPreMs = 0when skipped so timings remain accurateBuilds on PRs #342 (native CFG) and #343 (native dataflow) which add per-module bypass logic. This change lifts the optimization to the shared pre-parse step in
builder.js, eliminating the redundantensureWasmTreescall when native data is already sufficient.Test plan
node src/cli.js build . --engine native --timings— verifywasmPreMsdrops to ~0node src/cli.js build . --engine wasm --timings— verify WASM path still works