fix: benchmark scripts handle missing WASM grammars gracefully#311
fix: benchmark scripts handle missing WASM grammars gracefully#311carlos-alm merged 2 commits intomainfrom
Conversation
Add isWasmAvailable() to parser.js that checks if required WASM grammar files exist on disk. All 4 benchmark scripts now check WASM availability before attempting WASM engine benchmarks, mirroring the existing pattern for native engine availability. When neither engine is available, scripts print a helpful error message and exit instead of crashing. Impact: 2 functions changed, 5 affected
|
Claude finished @carlos-alm's task —— View job PR Review: Benchmark WASM Graceful HandlingReview Checklist
Phase 1: Root Cause AnalysisProblem Being Solved: Benchmark scripts crash with unclear errors when WASM grammar files are missing. This happens when users run benchmarks without first running Why This Approach: The PR adds Risk Assessment: LOW - This is defensive error handling that prevents crashes and provides helpful error messages. The change is minimal and follows existing patterns. Phase 2: Configuration Changes✅ No concerning configuration changes detected
Phase 3: Test Integrity
Phase 4: Code Quality✅ Implementation Quality - Good
✅ Error Handling - Excellent
✅ Code Consistency - Good
Critical ConcernsNone - This is solid defensive programming that follows established patterns. Final AssessmentRoot Cause AnalysisProblem Being Solved: Prevents benchmark crashes when WASM grammars are unavailable, providing clear error messages instead of cryptic failures. Critical Concerns (if any)None. The implementation is sound and follows established patterns. Final Recommendation
Condition for approval: Add unit tests for |
Greptile SummaryAdded Key improvements:
The implementation is clean, well-tested, and maintains consistency across all benchmark scripts. Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 1ac5d28 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
scripts/query-benchmark.js
Outdated
| const targets = selectTargets(); | ||
| console.error(`Targets: hub=${targets.hub}, mid=${targets.mid}, leaf=${targets.leaf}`); | ||
| wasm = benchmarkQueries(targets); | ||
| } else { | ||
| console.error('WASM grammars not built — skipping WASM benchmark'); | ||
| } | ||
|
|
||
| let native = null; | ||
| if (isNativeAvailable()) { | ||
| if (hasNative) { | ||
| if (fs.existsSync(dbPath)) fs.unlinkSync(dbPath); | ||
| await buildGraph(root, { engine: 'native', incremental: false }); | ||
|
|
||
| const targets = selectTargets(); | ||
| if (!hasWasm) { | ||
| console.error(`Targets: hub=${targets.hub}, mid=${targets.mid}, leaf=${targets.leaf}`); | ||
| } | ||
| native = benchmarkQueries(targets); |
There was a problem hiding this comment.
targets selected separately for each engine breaks benchmark validity. When both engines are available, WASM benchmarks with one set of targets (hub/mid/leaf from WASM graph), then native benchmarks with different targets (from native graph). This makes the comparison invalid. Should select targets once before the engine blocks and reuse for both.
| const targets = selectTargets(); | |
| console.error(`Targets: hub=${targets.hub}, mid=${targets.mid}, leaf=${targets.leaf}`); | |
| wasm = benchmarkQueries(targets); | |
| } else { | |
| console.error('WASM grammars not built — skipping WASM benchmark'); | |
| } | |
| let native = null; | |
| if (isNativeAvailable()) { | |
| if (hasNative) { | |
| if (fs.existsSync(dbPath)) fs.unlinkSync(dbPath); | |
| await buildGraph(root, { engine: 'native', incremental: false }); | |
| const targets = selectTargets(); | |
| if (!hasWasm) { | |
| console.error(`Targets: hub=${targets.hub}, mid=${targets.mid}, leaf=${targets.leaf}`); | |
| } | |
| native = benchmarkQueries(targets); | |
| const targets = selectTargets(); | |
| console.error(`Targets: hub=${targets.hub}, mid=${targets.mid}, leaf=${targets.leaf}`); | |
| wasm = benchmarkQueries(targets); | |
| } else { | |
| console.error('WASM grammars not built — skipping WASM benchmark'); | |
| } | |
| let native = null; | |
| if (hasNative) { | |
| if (fs.existsSync(dbPath)) fs.unlinkSync(dbPath); | |
| await buildGraph(root, { engine: 'native', incremental: false }); | |
| native = benchmarkQueries(targets); |
| let wasm = null; | ||
| if (hasWasm) { | ||
| if (fs.existsSync(dbPath)) fs.unlinkSync(dbPath); | ||
| await buildGraph(root, { engine: 'wasm', incremental: false }); | ||
|
|
||
| const wasm = benchmarkQueries(targets); | ||
| const targets = selectTargets(); | ||
| console.error(`Targets: hub=${targets.hub}, mid=${targets.mid}, leaf=${targets.leaf}`); | ||
| wasm = benchmarkQueries(targets); | ||
| } else { | ||
| console.error('WASM grammars not built — skipping WASM benchmark'); | ||
| } | ||
|
|
||
| let native = null; | ||
| if (isNativeAvailable()) { | ||
| if (hasNative) { | ||
| if (fs.existsSync(dbPath)) fs.unlinkSync(dbPath); | ||
| await buildGraph(root, { engine: 'native', incremental: false }); | ||
|
|
||
| const targets = selectTargets(); | ||
| if (!hasWasm) { | ||
| console.error(`Targets: hub=${targets.hub}, mid=${targets.mid}, leaf=${targets.leaf}`); | ||
| } | ||
| native = benchmarkQueries(targets); |
There was a problem hiding this comment.
when both engines are available, targets is selected separately inside each block (line 175 and 187), causing WASM and native to benchmark different functions. For a valid comparison, both should use the same targets.
Fix: move let targets = null; outside the blocks, select once in WASM block, reuse in native:
let wasm = null;
let targets = null;
if (hasWasm) {
if (fs.existsSync(dbPath)) fs.unlinkSync(dbPath);
await buildGraph(root, { engine: 'wasm', incremental: false });
targets = selectTargets();
console.error(`Targets: hub=${targets.hub}, mid=${targets.mid}, leaf=${targets.leaf}`);
wasm = benchmarkQueries(targets);
} else {
console.error('WASM grammars not built — skipping WASM benchmark');
}
let native = null;
if (hasNative) {
if (fs.existsSync(dbPath)) fs.unlinkSync(dbPath);
await buildGraph(root, { engine: 'native', incremental: false });
if (!targets) {
targets = selectTargets();
console.error(`Targets: hub=${targets.hub}, mid=${targets.mid}, leaf=${targets.leaf}`);
}
native = benchmarkQueries(targets);
}
…le tests - query-benchmark.js: select targets once from first built graph, reuse for both engines to ensure valid comparisons - Add unit tests for isWasmAvailable() covering all branches: all present, some missing, all missing, and verifying only required grammars are checked
|
Claude finished @carlos-alm's task —— View job PR Review: Benchmark WASM Graceful HandlingReview Checklist
Phase 1: Root Cause AnalysisProblem Being Solved: Benchmark scripts crash with unclear errors when WASM grammar files are missing. This happens when users run benchmarks without first running Why This Approach: The PR adds Risk Assessment: LOW - This is defensive error handling that prevents crashes and provides helpful error messages. The change is minimal and follows existing patterns. Phase 2: Configuration Changes✅ No concerning configuration changes detected
Phase 3: Test Integrity✅ Good test coverage for new functionality
Phase 4: Code Quality✅ Implementation Quality - Good
✅ Error Handling - Excellent
✅ Code Consistency - Good
✅ Logic Bug Fixed
Critical ConcernsNone - This is solid defensive programming that follows established patterns and addresses previous review feedback. Final AssessmentRoot Cause AnalysisProblem Being Solved: Prevents benchmark crashes when WASM grammars are unavailable, providing clear error messages instead of cryptic failures. Critical Concerns (if any)None. The implementation is sound, follows existing patterns, and addresses all previous review concerns. Final Recommendation
|
|
Addressed review feedback in 1ac5d28: Greptile — targets selected separately for each engine (query-benchmark.js): Claude — missing test coverage for isWasmAvailable(): |
1 participant
Summary
isWasmAvailable()toparser.js— checks if required WASM grammar files (JS, TS, TSX) exist on diskbenchmark.js,query-benchmark.js,incremental-benchmark.js,token-benchmark.js) now check WASM availability before attempting WASM engine benchmarksisNativeAvailable()pattern for symmetric engine availability handlingTest plan
isWasmAvailable()returnsfalsewhen grammars directory is emptyisWasmAvailable()returnstruewhen grammars are builtbenchmark.jswithout WASM grammars — should skip WASM and run native-onlybenchmark.jswith WASM grammars — should run both engines as before