Add a dependency-free shared-WebGL feasibility harness - #417
Conversation
📝 WalkthroughWalkthroughAdded a dependency-free WebGL2 benchmark harness. It compares shared and native contexts across rendering, picking, recovery, and performance checks. It includes a browser interface, automation APIs, report validation, captured Chromium results, and production-scope documentation. ChangesShared WebGL experiment
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ExperimentApp
participant ChartView
participant SharedBackend
participant WebGL2Context
participant Canvas2D
ExperimentApp->>ChartView: schedule chart updates
ChartView->>SharedBackend: render chart and process picking
SharedBackend->>WebGL2Context: submit shared WebGL2 draws
SharedBackend->>Canvas2D: copy presentation crops
WebGL2Context-->>SharedBackend: report context loss or restoration
SharedBackend-->>ExperimentApp: expose statistics and recovery state
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
benchmarks/shared_webgl_spike/experiment.js (5)
1313-1316: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe third
coverageCompletecondition is always true.Line 1316 asserts
testCharts.length * indices.length === stats.requestedCharts * indices.length. Line 1315 already assertstestCharts.length === stats.requestedCharts. The third comparison is the second one multiplied by a constant, so it adds no coverage. Remove it.♻️ Proposed simplification
const coverageComplete = - rendered === stats.requestedCharts && - testCharts.length === stats.requestedCharts && - testCharts.length * indices.length === stats.requestedCharts * indices.length; + rendered === stats.requestedCharts && testCharts.length === stats.requestedCharts;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/shared_webgl_spike/experiment.js` around lines 1313 - 1316, Remove the redundant third comparison from the coverageComplete expression in the surrounding benchmark logic; retain the rendered count check and the testCharts.length === stats.requestedCharts check unchanged.
837-855: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
cycleResolve(true)reports success even when the rebuild fails.The
catchblock sets the chart state to"error"and logs the failure. Line 852 then resolves the cycle promise withtrue.ExperimentApp.cycleContexttreats a truthy result as a successful restore.SharedBackend._handleContextRestoredresolves withfullyRebuiltinstead.The subsequent
verify()call still fails, so the reported outcome stays correct. Resolve with the actual rebuild result for symmetry.♻️ Proposed change
chart.canvas.addEventListener("webglcontextrestored", () => { if (record.destroying) return; + let rebuilt = false; try { record.programs = createProgramSet(gl); record.lossExtension = gl.getExtension("WEBGL_lose_context"); createChartGpu(gl, chart); chart.pickTarget = null; chart.setState("live"); this.app.needsDraw = true; + rebuilt = true; this.app.onContextRestored(`Native context ${chart.index + 1} restored.`); } catch (error) { chart.setState("error", "restore failed"); this.app.log(`Native chart ${chart.index + 1} restore failed: ${error.message}`); } if (record.cycleResolve) { - record.cycleResolve(true); + record.cycleResolve(rebuilt); record.cycleResolve = null; } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/shared_webgl_spike/experiment.js` around lines 837 - 855, Update the context-restoration handler’s cycleResolve outcome to reflect whether the rebuild succeeded: track a success flag initialized false, set it true only after the try block completes, and resolve cycleResolve with that flag instead of always true. Preserve the existing error state and logging in the catch block.
174-223: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftThe state-stress check is a closed loop, so it cannot detect a missing reset.
poisonStatedirties exactly the state thatbeginPassrestores:BLEND,DEPTH_TEST,CULL_FACE,colorMask,viewport,scissor,activeTexture, the VAO binding, and the current program. The canary therefore always passes, even ifbeginPassomits a state that a real client would leak.Several persistent WebGL2 states are not reset here:
frontFace,depthFunc,blendColor,stencilFunc,polygonOffset,readBuffer,drawBuffers, the transform-feedback binding,UNIFORM_BUFFERbindings, andUNPACK_FLIP_Y_WEBGL/UNPACK_PREMULTIPLY_ALPHA_WEBGL. The productionGLHostin#407will need those, so add them topoisonStateto make the harness result transferable.♻️ Suggested extension of the poison set
function poisonState(gl, programs, width, height) { gl.disable(gl.BLEND); gl.enable(gl.DEPTH_TEST); gl.enable(gl.CULL_FACE); gl.colorMask(false, false, false, false); + gl.frontFace(gl.CW); + gl.depthFunc(gl.GREATER); + gl.blendColor(1, 0, 1, 0.5); + gl.pixelStorei(gl.UNPACK_FLIP_Y_WEBGL, true); + gl.pixelStorei(gl.PACK_ALIGNMENT, 1); gl.viewport(1, 2, Math.max(1, width - 3), Math.max(1, height - 4));Add the matching resets to
beginPass.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/shared_webgl_spike/experiment.js` around lines 174 - 223, Extend poisonState with persistent WebGL2 state that beginPass does not currently reset, including frontFace, depthFunc, blendColor, stencilFunc, polygonOffset, readBuffer, drawBuffers, transform-feedback binding, UNIFORM_BUFFER bindings, and UNPACK_FLIP_Y_WEBGL/UNPACK_PREMULTIPLY_ALPHA_WEBGL. Add corresponding restoration calls to beginPass so the canary detects omissions while each pass still starts from the expected baseline.
1367-1382: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
benchmarkandcycleContextdo not restore control state when an error occurs. Both methods disable a button and mutate UI state, then perform asynchronous work without afinallyblock. If any awaited call orgetStats()raises, the button stays disabled for the rest of the session.verify()at lines 1274-1365 already usestry/finallyfor exactly this reason; apply the same pattern.
benchmarks/shared_webgl_spike/experiment.js#L1367-L1382: wrap the body afterconst start = performance.now();intry, and movethis.ui.streaming.checked = wasStreaming;,this.ui.benchmark.disabled = false;, andthis.needsDraw = true;into afinallyblock.benchmarks/shared_webgl_spike/experiment.js#L1457-L1468: wrap the body afterthis.ui.cycle.disabled = true;intry, and movethis.ui.cycle.disabled = false;into afinallyblock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/shared_webgl_spike/experiment.js` around lines 1367 - 1382, Update benchmark and cycleContext in benchmarks/shared_webgl_spike/experiment.js at lines 1367-1382 and 1457-1468: wrap each asynchronous body in try/finally so benchmark always restores streaming state, re-enables the benchmark button, and sets needsDraw, while cycleContext always re-enables its button; preserve the existing success behavior and ensure cleanup runs when awaited calls or getStats() throw.
965-995: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWire or remove backend disposal.
No code path calls
SharedBackend.disposeorNativeBackend.dispose.navigatereloads the document, and the automation API does not expose disposal. Add disposal before navigation, expose it for automation, or remove the dead methods.NativeBackend.cycleContextalso does not retain its timers for cancellation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/shared_webgl_spike/experiment.js` around lines 965 - 995, Address the unused backend disposal paths by either invoking SharedBackend.dispose and NativeBackend.dispose before navigate, exposing disposal through the automation API, or removing the dead methods. Also update NativeBackend.cycleContext to retain its restore and timeout timer handles so dispose can cancel them safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/shared_webgl_spike/experiment.js`:
- Around line 394-396: Update sampleMatches so the frame-counter channel encoded
from chart.frameByte is compared exactly, while retaining the existing tolerance
for color channels affected by drawImage conversion drift. Ensure stale frames
differing by 1–3 renders cannot pass the canary.
- Around line 619-627: Update prepareVerification to size capacity using the
tallest chart in this.charts, including the existing non-zero source-Y padding,
before calculating cropOffsetPixels. Ensure the returned offset is based on the
final capacityHeight that render will use for every chart, while preserving the
current early return behavior.
In `@benchmarks/shared_webgl_spike/styles.css`:
- Around line 17-23: Update the affected CSS declarations in the stylesheet,
including the repeated locations, to comply with the configured Stylelint rules
while preserving the existing visual behavior and values. Verify the stylesheet
lint command passes with no violations.
---
Nitpick comments:
In `@benchmarks/shared_webgl_spike/experiment.js`:
- Around line 1313-1316: Remove the redundant third comparison from the
coverageComplete expression in the surrounding benchmark logic; retain the
rendered count check and the testCharts.length === stats.requestedCharts check
unchanged.
- Around line 837-855: Update the context-restoration handler’s cycleResolve
outcome to reflect whether the rebuild succeeded: track a success flag
initialized false, set it true only after the try block completes, and resolve
cycleResolve with that flag instead of always true. Preserve the existing error
state and logging in the catch block.
- Around line 174-223: Extend poisonState with persistent WebGL2 state that
beginPass does not currently reset, including frontFace, depthFunc, blendColor,
stencilFunc, polygonOffset, readBuffer, drawBuffers, transform-feedback binding,
UNIFORM_BUFFER bindings, and UNPACK_FLIP_Y_WEBGL/UNPACK_PREMULTIPLY_ALPHA_WEBGL.
Add corresponding restoration calls to beginPass so the canary detects omissions
while each pass still starts from the expected baseline.
- Around line 1367-1382: Update benchmark and cycleContext in
benchmarks/shared_webgl_spike/experiment.js at lines 1367-1382 and 1457-1468:
wrap each asynchronous body in try/finally so benchmark always restores
streaming state, re-enables the benchmark button, and sets needsDraw, while
cycleContext always re-enables its button; preserve the existing success
behavior and ensure cleanup runs when awaited calls or getStats() throw.
- Around line 965-995: Address the unused backend disposal paths by either
invoking SharedBackend.dispose and NativeBackend.dispose before navigate,
exposing disposal through the automation API, or removing the dead methods. Also
update NativeBackend.cycleContext to retain its restore and timeout timer
handles so dispose can cancel them safely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: de17eb6c-11a5-4568-8172-76fd9cc6d52c
📒 Files selected for processing (8)
benchmarks/README.mdbenchmarks/shared_webgl_spike/README.mdbenchmarks/shared_webgl_spike/RESULTS.mdbenchmarks/shared_webgl_spike/experiment.jsbenchmarks/shared_webgl_spike/index.htmlbenchmarks/shared_webgl_spike/results/chromium-2026-07-31.jsonbenchmarks/shared_webgl_spike/styles.cssspec/design-dossier.md
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
There was a problem hiding this comment.
3 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="benchmarks/shared_webgl_spike/styles.css">
<violation number="1" location="benchmarks/shared_webgl_spike/styles.css:619">
P3: The dense chart grid overflows the page at viewport widths 941–952px because eight 108px minimum columns plus gaps need 913px. Allowing these eight tracks to shrink removes the narrow breakpoint gap.</violation>
</file>
<file name="benchmarks/shared_webgl_spike/results/chromium-2026-07-31.json">
<violation number="1" location="benchmarks/shared_webgl_spike/results/chromium-2026-07-31.json:2">
P2: This published machine-readable result cannot pass the repository's required benchmark-artifact verifier: schema 1, the unregistered `shared-webgl-spike` kind, and incomplete standard environment metadata are rejected. Adding validator support for this result shape and emitting the required schema/environment fields would keep the evidence artifact independently verifiable.</violation>
</file>
<file name="benchmarks/shared_webgl_spike/index.html">
<violation number="1" location="benchmarks/shared_webgl_spike/index.html:120">
P3: Context-loss progress and renderer failure messages are not announced because the dynamically updated `#status` element is not a live region. Consider exposing it as a status region and deduplicating the periodic identical updates in `setStatus()` to avoid repeated announcements.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| @@ -0,0 +1,112 @@ | |||
| { | |||
| "schema_version": 1, | |||
There was a problem hiding this comment.
P2: This published machine-readable result cannot pass the repository's required benchmark-artifact verifier: schema 1, the unregistered shared-webgl-spike kind, and incomplete standard environment metadata are rejected. Adding validator support for this result shape and emitting the required schema/environment fields would keep the evidence artifact independently verifiable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At benchmarks/shared_webgl_spike/results/chromium-2026-07-31.json, line 2:
<comment>This published machine-readable result cannot pass the repository's required benchmark-artifact verifier: schema 1, the unregistered `shared-webgl-spike` kind, and incomplete standard environment metadata are rejected. Adding validator support for this result shape and emitting the required schema/environment fields would keep the evidence artifact independently verifiable.</comment>
<file context>
@@ -0,0 +1,112 @@
+{
+ "schema_version": 1,
+ "kind": "shared-webgl-spike",
+ "source_issue": "https://github.com/reflex-dev/xy/issues/407",
</file context>
| } | ||
|
|
||
| .chart-grid--dense { | ||
| grid-template-columns: repeat(8, minmax(108px, 1fr)); |
There was a problem hiding this comment.
P3: The dense chart grid overflows the page at viewport widths 941–952px because eight 108px minimum columns plus gaps need 913px. Allowing these eight tracks to shrink removes the narrow breakpoint gap.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At benchmarks/shared_webgl_spike/styles.css, line 619:
<comment>The dense chart grid overflows the page at viewport widths 941–952px because eight 108px minimum columns plus gaps need 913px. Allowing these eight tracks to shrink removes the narrow breakpoint gap.</comment>
<file context>
@@ -0,0 +1,738 @@
+ }
+
+ .chart-grid--dense {
+ grid-template-columns: repeat(8, minmax(108px, 1fr));
+ }
+}
</file context>
|
|
||
| <section class="result-strip"> | ||
| <div id="health-dot" class="health-dot" data-state="working"></div> | ||
| <p id="status">Building the shared renderer…</p> |
There was a problem hiding this comment.
P3: Context-loss progress and renderer failure messages are not announced because the dynamically updated #status element is not a live region. Consider exposing it as a status region and deduplicating the periodic identical updates in setStatus() to avoid repeated announcements.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At benchmarks/shared_webgl_spike/index.html, line 120:
<comment>Context-loss progress and renderer failure messages are not announced because the dynamically updated `#status` element is not a live region. Consider exposing it as a status region and deduplicating the periodic identical updates in `setStatus()` to avoid repeated announcements.</comment>
<file context>
@@ -0,0 +1,149 @@
+
+ <section class="result-strip">
+ <div id="health-dot" class="health-dot" data-state="working"></div>
+ <p id="status">Building the shared renderer…</p>
+ <p id="last-check" class="result-strip__aside" role="status">Checks not run yet</p>
+ </section>
</file context>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
scripts/verify_benchmark_report.py (2)
2034-2049: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider adding recovery invariants.
The recovery block validates types only. Two relations are cheap to enforce and catch transcription errors:
context_restores <= context_losseslive_charts_after_restore <= live_charts♻️ Proposed additional checks
for key in ("context_losses", "context_restores", "live_charts_after_restore"): _require_nonnegative_integer(recovery, key, recovery_path, errors) + losses = recovery.get("context_losses") + restores = recovery.get("context_restores") + if _is_number(losses) and _is_number(restores) and restores > losses: + errors.append(f"{recovery_path}.context_restores must be <= context_losses") + after = recovery.get("live_charts_after_restore") + if _is_number(after) and isinstance(live, int) and not isinstance(live, bool) and after > live: + errors.append(f"{recovery_path}.live_charts_after_restore must be <= live_charts") _require_boolean(recovery, "correctness_after_restore", recovery_path, errors)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/verify_benchmark_report.py` around lines 2034 - 2049, Extend the shared-profile recovery validation in the existing recovery block to enforce that context_restores does not exceed context_losses and live_charts_after_restore does not exceed live_charts. Reuse the established error-reporting pattern and live_charts value already available in the surrounding profile validation, while preserving the current type and nonnegative-integer checks.
1886-1899: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a report conversion step or document the manual mapping.
The harness emits camelCase fields and
canvasPixelsranges, while the report uses snake_case fields andcanvas_pixels.width/height. The README documents capture but not this transformation. Add the mapping tobenchmarks/shared_webgl_spike/README.mdor provide a converter script.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/verify_benchmark_report.py` around lines 1886 - 1899, Document the harness-to-report field transformation in the shared WebGL benchmark README, covering camelCase-to-snake_case names and conversion of canvasPixels ranges into canvas_pixels.width and canvas_pixels.height. Anchor the documentation near the existing capture instructions and describe the manual mapping unless an existing conversion utility should be extended.benchmarks/shared_webgl_spike/results/chromium-2026-07-31.json (1)
127-143: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider recording native recovery data.
The native profile lost 34 of 50 contexts, but it records no
recoveryblock. The shared profile records one. The comparison therefore reports recovery behavior for one backend only, while context loss is the failure mode the native backend actually exhibited.Recording native loss and restore counts would make the A/B comparison symmetric. If the harness cannot restore evicted native contexts, state that in
benchmarks/shared_webgl_spike/RESULTS.md.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/shared_webgl_spike/results/chromium-2026-07-31.json` around lines 127 - 143, Add a native recovery block to the benchmark result data, recording context-loss and restore counts consistently with the shared profile; if native contexts cannot be restored, represent that explicitly rather than omitting the block. Update the benchmark documentation in RESULTS.md to state the native restoration limitation when applicable, preserving symmetric A/B recovery reporting.tests/test_verify_benchmark_report.py (2)
854-925: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the remaining new invariants.
The new tests cover workloads, metrics, correctness coverage, state stress, and environment. Three new branches in
_validate_shared_webgl_profileand_validate_shared_webgl_quantileshave no test:
- the shared invariant
live_contexts must be 1 while shared charts are live- the native invariant
live_contexts must equal live_charts- the quantile ordering rule
quantiles must be nondecreasingThe first invariant encodes the central claim of the experiment. Add a negative test for each.
💚 Proposed tests
def test_shared_webgl_spike_rejects_multiple_shared_contexts(tmp_path: Path) -> None: payload = _shared_webgl_spike_report() payload["profiles"]["shared"]["live_contexts"] = 2 path = _write_report(tmp_path, payload) errors = verify_benchmark_report.validate_report(path, kind="shared-webgl-spike") assert any("live_contexts must be 1" in error for error in errors) def test_shared_webgl_spike_rejects_unordered_quantiles(tmp_path: Path) -> None: payload = _shared_webgl_spike_report() payload["profiles"]["shared"]["benchmark"]["frame_ms"]["p99"] = 0.0 path = _write_report(tmp_path, payload) errors = verify_benchmark_report.validate_report(path, kind="shared-webgl-spike") assert any("quantiles must be nondecreasing" in error for error in errors)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_verify_benchmark_report.py` around lines 854 - 925, Add negative tests covering the remaining validation branches in _validate_shared_webgl_profile and _validate_shared_webgl_quantiles: verify shared profiles reject live_contexts other than 1 when charts are live, native profiles reject live_contexts values that differ from live_charts, and benchmark frame_ms quantiles reject decreasing ordering. Use _shared_webgl_spike_report, _write_report, and validate_report, asserting the corresponding validation messages.
67-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider resolving the results file by glob.
The helper hardcodes
chromium-2026-07-31.json. A future capture adds a new date-stamped file, and these tests keep validating only the old one. Renaming or removing the file breaks seven tests with aFileNotFoundErrorinstead of a clear message.♻️ Proposed change to validate every captured result
def _shared_webgl_spike_report() -> dict: - path = ( - Path(__file__).resolve().parents[1] - / "benchmarks" - / "shared_webgl_spike" - / "results" - / "chromium-2026-07-31.json" - ) - return json.loads(path.read_text(encoding="utf-8")) + results = ( + Path(__file__).resolve().parents[1] / "benchmarks" / "shared_webgl_spike" / "results" + ) + paths = sorted(results.glob("*.json")) + assert paths, f"no shared WebGL spike results in {results}" + return json.loads(paths[-1].read_text(encoding="utf-8"))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_verify_benchmark_report.py` around lines 67 - 75, Update _shared_webgl_spike_report to discover the date-stamped Chromium results file via glob rather than hardcoding chromium-2026-07-31.json, validate that the expected result is found, and load the resolved file so tests follow future captures and fail clearly when no result exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/shared_webgl_spike/results/chromium-2026-07-31.json`:
- Around line 54-58: Update benchmarks/shared_webgl_spike/RESULTS.md next to the
existing environment-specificity caveat to state that this capture is not
reproducible because environment.git.dirty is true and its commit differs from
base_commit. Include the relevant commit identifiers and explain that the exact
benchmark code cannot be reconstructed from them.
- Line 91: Update the results narrative in RESULTS.md to document both timed-run
limitations: state isolation was validated only by correctness while timed
state-reset work was omitted from both profiles, and recovery measured client
restoration without checking visible frames during the loss window. Explicitly
note that the shared throughput and ratio are not production estimates, while
preserving the feasibility-spike qualification.
---
Nitpick comments:
In `@benchmarks/shared_webgl_spike/results/chromium-2026-07-31.json`:
- Around line 127-143: Add a native recovery block to the benchmark result data,
recording context-loss and restore counts consistently with the shared profile;
if native contexts cannot be restored, represent that explicitly rather than
omitting the block. Update the benchmark documentation in RESULTS.md to state
the native restoration limitation when applicable, preserving symmetric A/B
recovery reporting.
In `@scripts/verify_benchmark_report.py`:
- Around line 2034-2049: Extend the shared-profile recovery validation in the
existing recovery block to enforce that context_restores does not exceed
context_losses and live_charts_after_restore does not exceed live_charts. Reuse
the established error-reporting pattern and live_charts value already available
in the surrounding profile validation, while preserving the current type and
nonnegative-integer checks.
- Around line 1886-1899: Document the harness-to-report field transformation in
the shared WebGL benchmark README, covering camelCase-to-snake_case names and
conversion of canvasPixels ranges into canvas_pixels.width and
canvas_pixels.height. Anchor the documentation near the existing capture
instructions and describe the manual mapping unless an existing conversion
utility should be extended.
In `@tests/test_verify_benchmark_report.py`:
- Around line 854-925: Add negative tests covering the remaining validation
branches in _validate_shared_webgl_profile and _validate_shared_webgl_quantiles:
verify shared profiles reject live_contexts other than 1 when charts are live,
native profiles reject live_contexts values that differ from live_charts, and
benchmark frame_ms quantiles reject decreasing ordering. Use
_shared_webgl_spike_report, _write_report, and validate_report, asserting the
corresponding validation messages.
- Around line 67-75: Update _shared_webgl_spike_report to discover the
date-stamped Chromium results file via glob rather than hardcoding
chromium-2026-07-31.json, validate that the expected result is found, and load
the resolved file so tests follow future captures and fail clearly when no
result exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 84426a88-4521-4448-b6e3-2004c1c27064
📒 Files selected for processing (9)
benchmarks/README.mdbenchmarks/shared_webgl_spike/RESULTS.mdbenchmarks/shared_webgl_spike/experiment.jsbenchmarks/shared_webgl_spike/index.htmlbenchmarks/shared_webgl_spike/results/chromium-2026-07-31.jsonbenchmarks/shared_webgl_spike/styles.cssscripts/verify_benchmark_report.pyspec/process/contributing.mdtests/test_verify_benchmark_report.py
🚧 Files skipped from review as they are similar to previous changes (5)
- benchmarks/README.md
- benchmarks/shared_webgl_spike/RESULTS.md
- benchmarks/shared_webgl_spike/index.html
- benchmarks/shared_webgl_spike/styles.css
- benchmarks/shared_webgl_spike/experiment.js
| "git": { | ||
| "commit": "2e169101f16cfdbe4881a704e3fe095379dc7b9e", | ||
| "branch": "agent/shared-webgl-spike", | ||
| "dirty": true | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Record that the capture is not reproducible.
environment.git.dirty is true, and environment.git.commit (2e169101…) differs from base_commit (d505ef57, line 5). A reader cannot reconstruct the exact code that produced these numbers from either identifier.
State this limitation in benchmarks/shared_webgl_spike/RESULTS.md, next to the existing environment-specificity caveat.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/shared_webgl_spike/results/chromium-2026-07-31.json` around lines
54 - 58, Update benchmarks/shared_webgl_spike/RESULTS.md next to the existing
environment-specificity caveat to state that this capture is not reproducible
because environment.git.dirty is true and its commit differs from base_commit.
Include the relevant commit identifiers and explain that the exact benchmark
code cannot be reconstructed from them.
| "pick_checks": 150, | ||
| "pick_failures": 0, | ||
| "crop_offset_pixels": 17, | ||
| "state_stress": true, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document two gaps in the timed run.
Two recorded flags limit what the headline throughput numbers support:
-
correctness.state_stressistrue, butbenchmark.state_stressisfalsein both profiles. Correctness validated state isolation. The timed run did not include the state-reset work. Shared-context compositing requires an explicit GL state reset per chart, so the shared profile's 2448.9 presentations/s excludes a cost that the production design in#407must pay. State this inbenchmarks/shared_webgl_spike/RESULTS.mdso the shared-versus-native ratio is not read as a production estimate. -
recovery.visible_frames_during_loss_checkedisfalse. The recovery result therefore covers restoration of clients, not the visual state that a user sees during the loss window.
Both are acceptable for a feasibility spike. Both should be explicit in the results narrative.
Also applies to: 100-100, 122-122
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/shared_webgl_spike/results/chromium-2026-07-31.json` at line 91,
Update the results narrative in RESULTS.md to document both timed-run
limitations: state isolation was validated only by correctness while timed
state-reset work was omitted from both profiles, and recovery measured client
restoration without checking visible frames during the loss window. Explicitly
note that the shared throughput and ratio are not production estimates, while
preserving the feasibility-spike qualification.
There was a problem hiding this comment.
6 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="benchmarks/README.md">
<violation number="1" location="benchmarks/README.md:280">
P2: The validation command never runs when this code block is followed sequentially because the preceding `python3 -m http.server` process blocks. Label the `make` invocation as a second-terminal command (or explicitly background the server) so the documented reproduction flow is executable.</violation>
</file>
<file name="scripts/verify_benchmark_report.py">
<violation number="1" location="scripts/verify_benchmark_report.py:1941">
P2: Native artifacts can claim more live contexts than were created, or more created contexts than requested, and still pass verification. Validate `live_charts <= created_contexts <= requested_charts`.</violation>
<violation number="2" location="scripts/verify_benchmark_report.py:1980">
P3: Malformed verification timestamps ending in `Z` are accepted as ISO UTC evidence. Parse the value as a datetime and require a UTC offset instead of relying only on `endswith`.</violation>
<violation number="3" location="scripts/verify_benchmark_report.py:2048">
P1: An artifact can claim successful recovery without a loss, with more restores than losses, or with more restored charts than requested. Cross-check the recovery counters and require a successful result to represent a completed loss/restore with all requested shared charts live.</violation>
<violation number="4" location="scripts/verify_benchmark_report.py:2076">
P1: Internally contradictory throughput metrics can pass and be published as benchmark evidence. Recompute and compare the generated rates, expected batches, and dropped intervals from `duration_ms`, `target_fps`, and the corresponding counters, using a small float tolerance for rates.</violation>
<violation number="5" location="scripts/verify_benchmark_report.py:2101">
P1: A/B artifacts with different chart counts or state-stress modes are accepted as matching workloads because the comparison tuple omits both fields. Include `requested_charts` and `benchmark.state_stress` in the workload identity.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| return None | ||
| for key in ("duration_ms", "target_fps"): | ||
| _require_positive_number(benchmark, key, benchmark_path, errors) | ||
| for key in ("observed_fps", "chart_presentations_per_second"): |
There was a problem hiding this comment.
P1: Internally contradictory throughput metrics can pass and be published as benchmark evidence. Recompute and compare the generated rates, expected batches, and dropped intervals from duration_ms, target_fps, and the corresponding counters, using a small float tolerance for rates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/verify_benchmark_report.py, line 2076:
<comment>Internally contradictory throughput metrics can pass and be published as benchmark evidence. Recompute and compare the generated rates, expected batches, and dropped intervals from `duration_ms`, `target_fps`, and the corresponding counters, using a small float tolerance for rates.</comment>
<file context>
@@ -1840,6 +1846,300 @@ def _validate_dashboard_telemetry(row: dict[str, Any], path: str, errors: list[s
+ return None
+ for key in ("duration_ms", "target_fps"):
+ _require_positive_number(benchmark, key, benchmark_path, errors)
+ for key in ("observed_fps", "chart_presentations_per_second"):
+ _require_nonnegative_number(benchmark, key, benchmark_path, errors)
+ _require_positive_integer(benchmark, "points_per_chart", benchmark_path, errors)
</file context>
| _require_boolean(recovery, "correctness_after_restore", recovery_path, errors) | ||
| _require_boolean(recovery, "visible_frames_during_loss_checked", recovery_path, errors) |
There was a problem hiding this comment.
P1: An artifact can claim successful recovery without a loss, with more restores than losses, or with more restored charts than requested. Cross-check the recovery counters and require a successful result to represent a completed loss/restore with all requested shared charts live.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/verify_benchmark_report.py, line 2048:
<comment>An artifact can claim successful recovery without a loss, with more restores than losses, or with more restored charts than requested. Cross-check the recovery counters and require a successful result to represent a completed loss/restore with all requested shared charts live.</comment>
<file context>
@@ -1840,6 +1846,300 @@ def _validate_dashboard_telemetry(row: dict[str, Any], path: str, errors: list[s
+ if isinstance(recovery, dict):
+ for key in ("context_losses", "context_restores", "live_charts_after_restore"):
+ _require_nonnegative_integer(recovery, key, recovery_path, errors)
+ _require_boolean(recovery, "correctness_after_restore", recovery_path, errors)
+ _require_boolean(recovery, "visible_frames_during_loss_checked", recovery_path, errors)
+
</file context>
| _require_boolean(recovery, "correctness_after_restore", recovery_path, errors) | |
| _require_boolean(recovery, "visible_frames_during_loss_checked", recovery_path, errors) | |
| _require_boolean(recovery, "correctness_after_restore", recovery_path, errors) | |
| _require_boolean(recovery, "visible_frames_during_loss_checked", recovery_path, errors) | |
| losses = recovery.get("context_losses") | |
| restores = recovery.get("context_restores") | |
| live_after_restore = recovery.get("live_charts_after_restore") | |
| if ( | |
| isinstance(losses, int) | |
| and not isinstance(losses, bool) | |
| and isinstance(restores, int) | |
| and not isinstance(restores, bool) | |
| and restores > losses | |
| ): | |
| errors.append(f"{recovery_path}.context_restores must be <= context_losses") | |
| if ( | |
| isinstance(live_after_restore, int) | |
| and not isinstance(live_after_restore, bool) | |
| and isinstance(requested, int) | |
| and not isinstance(requested, bool) | |
| and live_after_restore > requested | |
| ): | |
| errors.append( | |
| f"{recovery_path}.live_charts_after_restore must be <= requested_charts" | |
| ) | |
| if ( | |
| recovery.get("correctness_after_restore") is True | |
| and isinstance(losses, int) | |
| and not isinstance(losses, bool) | |
| and isinstance(restores, int) | |
| and not isinstance(restores, bool) | |
| and isinstance(live_after_restore, int) | |
| and not isinstance(live_after_restore, bool) | |
| and isinstance(requested, int) | |
| and not isinstance(requested, bool) | |
| and (losses == 0 or restores != losses or live_after_restore != requested) | |
| ): | |
| errors.append( | |
| f"{recovery_path}.correctness_after_restore is inconsistent with recovery telemetry" | |
| ) |
| _validate_shared_webgl_quantiles( | ||
| present, f"{benchmark_path}.present_ms_per_chart", ("p50", "p95"), errors | ||
| ) | ||
| return ( |
There was a problem hiding this comment.
P1: A/B artifacts with different chart counts or state-stress modes are accepted as matching workloads because the comparison tuple omits both fields. Include requested_charts and benchmark.state_stress in the workload identity.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/verify_benchmark_report.py, line 2101:
<comment>A/B artifacts with different chart counts or state-stress modes are accepted as matching workloads because the comparison tuple omits both fields. Include `requested_charts` and `benchmark.state_stress` in the workload identity.</comment>
<file context>
@@ -1840,6 +1846,300 @@ def _validate_dashboard_telemetry(row: dict[str, Any], path: str, errors: list[s
+ _validate_shared_webgl_quantiles(
+ present, f"{benchmark_path}.present_ms_per_chart", ("p50", "p95"), errors
+ )
+ return (
+ benchmark.get("points_per_chart"),
+ benchmark.get("dense"),
</file context>
| make check-benchmark-report \ | ||
| BENCHMARK_JSON=benchmarks/shared_webgl_spike/results/chromium-2026-07-31.json \ | ||
| BENCHMARK_KIND=shared-webgl-spike |
There was a problem hiding this comment.
P2: The validation command never runs when this code block is followed sequentially because the preceding python3 -m http.server process blocks. Label the make invocation as a second-terminal command (or explicitly background the server) so the documented reproduction flow is executable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At benchmarks/README.md, line 280:
<comment>The validation command never runs when this code block is followed sequentially because the preceding `python3 -m http.server` process blocks. Label the `make` invocation as a second-terminal command (or explicitly background the server) so the documented reproduction flow is executable.</comment>
<file context>
@@ -277,6 +277,9 @@ exercises state isolation, crop/orientation canaries, picking, and context resto
```bash
python3 -m http.server 4173 --directory benchmarks/shared_webgl_spike
+make check-benchmark-report \
+ BENCHMARK_JSON=benchmarks/shared_webgl_spike/results/chromium-2026-07-31.json \
+ BENCHMARK_KIND=shared-webgl-spike
</file context>
| make check-benchmark-report \ | |
| BENCHMARK_JSON=benchmarks/shared_webgl_spike/results/chromium-2026-07-31.json \ | |
| BENCHMARK_KIND=shared-webgl-spike | |
| # In another terminal: | |
| make check-benchmark-report \ | |
| BENCHMARK_JSON=benchmarks/shared_webgl_spike/results/chromium-2026-07-31.json \ | |
| BENCHMARK_KIND=shared-webgl-spike |
| if profile == "shared" and isinstance(live, int) and live > 0 and contexts != 1: | ||
| errors.append(f"{path}.live_contexts must be 1 while shared charts are live") | ||
| if profile == "native": | ||
| _require_nonnegative_integer(profile_value, "created_contexts", path, errors) |
There was a problem hiding this comment.
P2: Native artifacts can claim more live contexts than were created, or more created contexts than requested, and still pass verification. Validate live_charts <= created_contexts <= requested_charts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/verify_benchmark_report.py, line 1941:
<comment>Native artifacts can claim more live contexts than were created, or more created contexts than requested, and still pass verification. Validate `live_charts <= created_contexts <= requested_charts`.</comment>
<file context>
@@ -1840,6 +1846,300 @@ def _validate_dashboard_telemetry(row: dict[str, Any], path: str, errors: list[s
+ if profile == "shared" and isinstance(live, int) and live > 0 and contexts != 1:
+ errors.append(f"{path}.live_contexts must be 1 while shared charts are live")
+ if profile == "native":
+ _require_nonnegative_integer(profile_value, "created_contexts", path, errors)
+ if isinstance(live, int) and not isinstance(live, bool) and contexts != live:
+ errors.append(f"{path}.live_contexts must equal live_charts")
</file context>
| _require_nonnegative_integer(profile_value, "created_contexts", path, errors) | |
| _require_nonnegative_integer(profile_value, "created_contexts", path, errors) | |
| created = profile_value.get("created_contexts") | |
| if ( | |
| isinstance(created, int) | |
| and not isinstance(created, bool) | |
| and isinstance(requested, int) | |
| and not isinstance(requested, bool) | |
| and isinstance(live, int) | |
| and not isinstance(live, bool) | |
| and not (live <= created <= requested) | |
| ): | |
| errors.append( | |
| f"{path}.created_contexts must be between live_charts and requested_charts" | |
| ) |
| f"{correctness_path}.state_stress_method must be {expected_stress_method!r}" | ||
| ) | ||
| verified_at_utc = correctness.get("verified_at_utc") | ||
| if not isinstance(verified_at_utc, str) or not verified_at_utc.endswith("Z"): |
There was a problem hiding this comment.
P3: Malformed verification timestamps ending in Z are accepted as ISO UTC evidence. Parse the value as a datetime and require a UTC offset instead of relying only on endswith.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/verify_benchmark_report.py, line 1980:
<comment>Malformed verification timestamps ending in `Z` are accepted as ISO UTC evidence. Parse the value as a datetime and require a UTC offset instead of relying only on `endswith`.</comment>
<file context>
@@ -1840,6 +1846,300 @@ def _validate_dashboard_telemetry(row: dict[str, Any], path: str, errors: list[s
+ f"{correctness_path}.state_stress_method must be {expected_stress_method!r}"
+ )
+ verified_at_utc = correctness.get("verified_at_utc")
+ if not isinstance(verified_at_utc, str) or not verified_at_utc.endswith("Z"):
+ errors.append(
+ f"{correctness_path}.verified_at_utc must be an ISO UTC string ending in Z"
</file context>
1 participant
Summary
Adds a standalone, dependency-free A/B harness for the blit-mode shared-context design proposed in #407.
The shared path uses one detached WebGL2 context to render synthetic chart clients sequentially, then synchronously presents each completed frame into that chart's visible 2D canvas. The native baseline renders the same workload with one WebGL2 context per canvas.
This is a feasibility experiment only. It does not modify xy's production renderer and does not close #407.
What the harness exercises
Exploratory result
One run in Chrome 150.0.0.0 using ANGLE's Metal renderer on an Apple M5 Pro produced:
The shared context was deliberately lost and restored. All 50 synthetic client resources were rebuilt and the complete checks passed again.
These numbers are environment-specific. Timing fields measure JavaScript submission cost, not completed GPU work, and should not be interpreted as a shared-vs-native speedup because the native arm rendered only its 16 surviving clients.
Scope and limitations
This does not yet validate the design against xy's production
ChartView.In particular, it does not cover:
The exact-ID assertions isolate one requested vertex. The normal dense hover pass renders all points and returns the last covered sample, so these assertions do not prove production interaction semantics.
The repository's benchmark-harness test suite validates metadata/report infrastructure; this standalone browser experiment is functionally verified through its own browser controls and automation surface, not that Python test suite.
Validation
node --check benchmarks/shared_webgl_spike/experiment.jsgit diff --checkmake check-benchmark-harness— 108 passeduv run --with pre-commit pre-commit run --all-filesuv run ruff check .uv run ruff format --check .Next step
Use this result to inform the phased production work in #407:
GLHostabstraction without changing behavior.ChartViewinstances to one host.Refs #407.
Summary by CodeRabbit
New Features
Documentation
Tests