feat: add backend beginFrame zero-copy SAB frame path#213
feat: add backend beginFrame zero-copy SAB frame path#213RtlZeroMemory merged 2 commits intomainfrom
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a backend begin-frame API and zero-copy frame writer path; renderers use beginFrame/commit/abort when available to build drawlists directly into backend-provided buffers. Node backend implements beginFrame with SAB slot management and tests exercise the new writer path. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer (rawRenderer / widgetRenderer)
participant Builder as Builder (buildInto)
participant Backend as Backend (NodeBackend)
participant SAB as SAB Transport
Renderer->>Backend: beginFrame(minCapacity)
activate Backend
Backend->>Backend: reserveFrameSubmission()
Backend->>SAB: acquireSlot()
Backend-->>Renderer: BackendFrameWriter(buf, commit, abort)
deactivate Backend
Renderer->>Builder: buildInto(buf)
alt buildInto succeeds
Renderer->>Backend: frameWriter.commit(byteLen)
activate Backend
Backend->>SAB: publishSabFrame / Atomics.notify
Backend-->>Renderer: commit Promise resolves (in-flight)
deactivate Backend
else buildInto fails
Renderer->>Backend: frameWriter.abort()
activate Backend
Backend->>SAB: releaseSlot()
Backend-->>Renderer: abort handled
deactivate Backend
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/app/widgetRenderer.ts (2)
3608-3701:⚠️ Potential issue | 🔴 CriticalEnsure frame writer abort is guaranteed on every exceptional exit before commit.
After Line 3614, some throw paths can bypass
abortFrameWriter()(for example, ifbuildIntothrows at Line 3634, or if later logic throws before Line 3690), which can leak a reserved SAB slot.Suggested hardening (wrap writer-owned section and always abort on failure)
- const buildToken = perfMarkStart("drawlist_build"); - const built: DrawlistBuildResult = - frameWriter === null - ? this.builder.build() - : ( - this.builder as unknown as { - buildInto: (buf: Uint8Array) => DrawlistBuildResult; - } - ).buildInto(frameWriter.buf); - perfMarkEnd("drawlist_build", buildToken); + let built: DrawlistBuildResult; + const buildToken = perfMarkStart("drawlist_build"); + try { + built = + frameWriter === null + ? this.builder.build() + : ( + this.builder as unknown as { + buildInto: (buf: Uint8Array) => DrawlistBuildResult; + } + ).buildInto(frameWriter.buf); + } catch (e: unknown) { + abortFrameWriter(); + return { ok: false, code: "ZRUI_DRAWLIST_BUILD_ERROR", detail: describeThrown(e) }; + } finally { + perfMarkEnd("drawlist_build", buildToken); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/app/widgetRenderer.ts` around lines 3608 - 3701, The frameWriter allocated from beginFrame can be left reserved when exceptions (e.g. from builder.buildInto or later user code) escape before the successful commit; ensure the writer is always aborted on any exceptional exit by wrapping the entire "writer-owned" region (from creation of frameWriter / call to buildInto through the backend request/commit) in a try/finally that calls abortFrameWriter() in the finally when the writer was not successfully committed, and only clear frameWriter (set to null) after a successful commit path (the branch that calls frameWriter.commit or backend.requestFrame). Concretely, locate beginFrame, frameWriter, abortFrameWriter, the buildInto call and the branch that does frameWriter.commit/this.backend.requestFrame and restructure so the commit/return happens inside the try and abortFrameWriter() is guaranteed in the finally for all throw paths; keep existing abortFrameWriter() semantics and ensure frameWriter is nulled only after commit.
3608-3697:⚠️ Potential issue | 🟡 MinorRun the full test suite before merging this renderer change.
This is a renderer-path change and requires full test suite verification with
npm test(or equivalently,node scripts/run-tests.mjs) per code-standards.md before committing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/app/widgetRenderer.ts` around lines 3608 - 3697, This change affects the renderer path (functions/variables: beginFrame, frameWriter, abortFrameWriter, backend.requestFrame, frameWriter.commit) and must not be merged until the full test suite is run; run the full test suite locally with `npm test` or `node scripts/run-tests.mjs`, fix any failures that arise (including unit, integration, and renderer-related tests), re-run tests until all pass, and ensure CI also reports green before merging.
🧹 Nitpick comments (2)
packages/node/src/__tests__/worker_integration.test.ts (1)
765-778: Test-only buildInto shim is acceptable but note the error message.The shim's error code
"ZRDL_TOO_LARGE"with detail"target buffer too small for buildInto shim"is slightly misleading since it's the target that's too small, not the drawlist being too large. Since this is test-only code, it's acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/node/src/__tests__/worker_integration.test.ts` around lines 765 - 778, The test-only shim for builderWithInto.buildInto uses an error object whose code and message are slightly misleading; update the shim in the builderWithInto.buildInto assignment so the error.detail clearly states that the target buffer is too small (e.g., "target buffer too small") and, if desired, adjust the error.code from "ZRDL_TOO_LARGE" to a more accurate identifier (reference builderWithInto.buildInto and the returned error object) so the error reflects that the target buffer is undersized rather than the drawlist being too large.packages/node/src/backend/nodeBackend.ts (1)
1010-1018: Consider consistent error types for commit validation failures.The commit method throws
RangeErrorfor invalidbyteLen, while other backend errors useZrUiError. This inconsistency is minor but worth noting. The current approach is acceptable since these are programming errors (invalid arguments) rather than runtime backend errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/node/src/backend/nodeBackend.ts` around lines 1010 - 1018, Replace the thrown RangeError instances in NodeBackend's commit method with the project's backend error type so error handling is consistent: change the two checks that throw RangeError("NodeBackend: frame byteLen must be an integer") and the RangeError with the out-of-range message to throw ZrUiError (preserving the existing descriptive messages and including byteLen and slotBytes), ensuring the validation in commit(byteLen: number) uses ZrUiError instead of RangeError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/core/src/app/widgetRenderer.ts`:
- Around line 3608-3701: The frameWriter allocated from beginFrame can be left
reserved when exceptions (e.g. from builder.buildInto or later user code) escape
before the successful commit; ensure the writer is always aborted on any
exceptional exit by wrapping the entire "writer-owned" region (from creation of
frameWriter / call to buildInto through the backend request/commit) in a
try/finally that calls abortFrameWriter() in the finally when the writer was not
successfully committed, and only clear frameWriter (set to null) after a
successful commit path (the branch that calls frameWriter.commit or
backend.requestFrame). Concretely, locate beginFrame, frameWriter,
abortFrameWriter, the buildInto call and the branch that does
frameWriter.commit/this.backend.requestFrame and restructure so the
commit/return happens inside the try and abortFrameWriter() is guaranteed in the
finally for all throw paths; keep existing abortFrameWriter() semantics and
ensure frameWriter is nulled only after commit.
- Around line 3608-3697: This change affects the renderer path
(functions/variables: beginFrame, frameWriter, abortFrameWriter,
backend.requestFrame, frameWriter.commit) and must not be merged until the full
test suite is run; run the full test suite locally with `npm test` or `node
scripts/run-tests.mjs`, fix any failures that arise (including unit,
integration, and renderer-related tests), re-run tests until all pass, and
ensure CI also reports green before merging.
---
Nitpick comments:
In `@packages/node/src/__tests__/worker_integration.test.ts`:
- Around line 765-778: The test-only shim for builderWithInto.buildInto uses an
error object whose code and message are slightly misleading; update the shim in
the builderWithInto.buildInto assignment so the error.detail clearly states that
the target buffer is too small (e.g., "target buffer too small") and, if
desired, adjust the error.code from "ZRDL_TOO_LARGE" to a more accurate
identifier (reference builderWithInto.buildInto and the returned error object)
so the error reflects that the target buffer is undersized rather than the
drawlist being too large.
In `@packages/node/src/backend/nodeBackend.ts`:
- Around line 1010-1018: Replace the thrown RangeError instances in
NodeBackend's commit method with the project's backend error type so error
handling is consistent: change the two checks that throw
RangeError("NodeBackend: frame byteLen must be an integer") and the RangeError
with the out-of-range message to throw ZrUiError (preserving the existing
descriptive messages and including byteLen and slotBytes), ensuring the
validation in commit(byteLen: number) uses ZrUiError instead of RangeError.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/core/src/app/__tests__/widgetRenderer.integration.test.tspackages/core/src/app/rawRenderer.tspackages/core/src/app/widgetRenderer.tspackages/core/src/backend.tspackages/core/src/index.tspackages/node/src/__tests__/worker_integration.test.tspackages/node/src/backend/nodeBackend.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 626e086573
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return null; | ||
| } | ||
|
|
||
| const slotIndex = acquireSabSlot(sabFrameTransport); |
There was a problem hiding this comment.
Using acquireSabSlot() here can steal a slot in READY state, but beginFrame() does not publish a replacement header/token until commit() (and abort() never does). Under SAB contention (all slots occupied), the worker can still read the old published frame metadata and then fail in engineWorker.tick() because the slot is no longer READY with the expected token, which escalates to a backend fatal and rejects in-flight frames. This can happen if frame building takes time before commit() or when a reclaimed writer is aborted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in follow-up PR #214: beginFrame now only acquires FREE SAB slots (no READY-slot reclaim), plus a regression test for single-slot pressure.
1 participant
Summary
beginFramewritersbeginFramecapability for SAB slot writers withcommit/abortand preserved ACK marker behaviorWidgetRendererandRawRendererto preferbeginFrame + builder.buildInto(...)when both backend + builder support it, with safe abort fallback pathsTesting
Summary by CodeRabbit
New Features
Stability / Error Handling
Tests