fix: stabilize benchmark targets across engines and preserve README links#527
fix: stabilize benchmark targets across engines and preserve README links#527carlos-alm merged 3 commits intomainfrom
Conversation
|
Claude finished @carlos-alm's task —— View job Pull Request ReviewReviewing PR #527: "fix: stabilize benchmark targets across engines and preserve README links" Root Cause AnalysisProblem Being Solved: Three legitimate benchmark infrastructure issues:
Why This Approach: Targeted, appropriate fixes:
Risk Assessment: ✅ LOW RISK - Genuine infrastructure fixes, not symptom masking. Backlog Compliance
Critical Concerns (Minor)Test Integrity: C# parity test ( Code Quality: Strong implementation quality across all changes:
Final Recommendation
|
Greptile SummaryThis PR addresses three follow-up issues from the v3.3.0 benchmark update (PR #524): it stabilises query targets across engines so WASM and native benchmark the same symbols, fixes a link-preservation regression in the README update script, and adds a detailed regression note to the benchmark document. Key changes:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant P as Parent (forkEngines)
participant W as WASM Worker
participant N as Native Worker
P->>W: fork(env: {__BENCH_ENGINE__: "wasm"})
W->>W: selectTargets() → {hub, leaf}
W-->>P: JSON result (includes targets)
P->>P: process.env[__BENCH_TARGETS__] = JSON.stringify(targets)
P->>N: fork(env: {__BENCH_ENGINE__: "native", __BENCH_TARGETS__: "..."})
N->>N: workerTargets() → reuse WASM targets
N-->>P: JSON result
P->>P: delete process.env[__BENCH_TARGETS__]
P-->>P: return {wasm, native}
Last reviewed commit: "fix: remove dead res..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
scripts/update-benchmark-report.js
Outdated
| const linksMatch = readme.match(/Self-measured on every release via CI \(([^)]+)\)/); | ||
| if (linksMatch) { | ||
| benchmarkLinks = linksMatch[1]; | ||
| } |
There was a problem hiding this comment.
The pattern [^)]+ stops at the first ) it encounters. Since markdown links have the form [text](url), the closing ) of the URL is encountered before the outer closing paren of the CI (...) construct. As a result, linksMatch[1] captures a truncated, broken string on any README that already contains markdown links.
Trace for a two-link README:
Self-measured on every release via CI ([build benchmarks](generated/benchmarks/BUILD-BENCHMARKS.md) | ...):
\(matches(before[build[^)]+greedily consumes until the first)→ captures[build benchmarks](generated/benchmarks/BUILD-BENCHMARKS.md\)matches)after.md
On the next script run benchmarkLinks becomes the truncated/malformed value and gets written back into the README, silently corrupting it every CI release.
A simple fix is to match up to the last ) before the colon at end-of-line instead of using [^)]+:
const linksMatch = readme.match(/Self-measured on every release via CI \((.+?)\):/);This uses a non-greedy (.+?) followed by ): which anchors to the correct outer closing paren + colon, allowing ) characters inside markdown links to be included in the capture.
There was a problem hiding this comment.
Fixed — switched to non-greedy (.+?): pattern that anchors on the outer ): delimiter, correctly capturing markdown links with parentheses in URLs.
scripts/lib/fork-engine.js
Outdated
| } | ||
|
|
||
| // Propagate targets from the first engine to the second | ||
| const firstResult = results.wasm || results.native; |
There was a problem hiding this comment.
results.native is always null here
At this point in the sequential flow, results.native has not been assigned yet (native runs on lines 186–188). The || results.native arm is therefore always null and can never contribute a value. The comment above says "Propagate targets from the first engine to the second", which matches only results.wasm.
Keeping the dead || results.native could mislead future readers into thinking both results might be populated here, or that this handles the native-only case (it doesn't — when only native runs there is nothing to propagate, which is the correct behaviour).
| const firstResult = results.wasm || results.native; | |
| const firstResult = results.wasm; |
There was a problem hiding this comment.
Fixed — removed the dead || results.native arm since native hasn't run yet at this point in the flow.
… README links - Share selectTargets() results from first engine worker to second via env, ensuring both engines benchmark the same hub/mid/leaf symbols - Preserve existing README benchmark links instead of hardcoding a subset - Add regression notes explaining v3.1.4 → v3.3.0 build performance increase Impact: 2 functions changed, 4 affected
…ark line The previous [^)]+ pattern stopped at the first ) inside markdown link URLs, truncating and corrupting the captured link string on every run. Switch to (.+?): which anchors on the outer ): delimiter.
results.native is always null at this point since native runs after target propagation. The || results.native arm was unreachable. Impact: 1 functions changed, 0 affected
carlos-alm
force-pushed
the
fix/review-522
branch
from
75a85fc to
56e866f
Compare
1 participant
Summary
Fixes three issues identified in PR #524 (3.3.0 benchmark update):
selectTargets()ran independently per engine worker, producing different hub/mid/leaf symbols because native and WASM build slightly different graphs (7634 vs 7609 nodes). Now the first engine's targets are propagated to the second viaTARGETS_ENV_KEYenv var, ensuring apples-to-apples comparison.update-benchmark-report.jshardcoded only 2 benchmark links (build + embedding), dropping query, incremental, and resolution links on every run. Now preserves existing links from the README.Test plan
node scripts/benchmark.js --version devlocally and verify both engines report the sametargetsin stderr outputnode scripts/update-benchmark-report.jsagainst existing benchmark JSON and verify README links are preserved