EPIC 8: frame text arena + slice-referenced text ops#218
Merged
RtlZeroMemory merged 4 commits intomainfrom Feb 26, 2026
Merged
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (7)
✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. 📝 WalkthroughWalkthroughAdds a per-frame UTF‑8 text arena and integrates arena-backed text allocation/encoding into the drawlist builder (new FrameTextArena, text-perf counters, reserve/get APIs, builder signature changes, layout changes), updates many tests to arena semantics, and adds a benchmark script comparing legacy vs arena encoding. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Builder as DrawlistBuilder
participant Arena as FrameTextArena
participant Encoder as Utf8Encoder
participant Buffer as BinaryBuffer
Client->>Builder: drawText(text, style)
Builder->>Builder: allocTextSlice(text)
Builder->>Arena: allocUtf8(text)
Arena->>Arena: reserve if needed
Arena->>Encoder: encodeInto(text)
Encoder-->>Arena: encoded bytes
Arena->>Arena: update counters (textEncoderCalls, textSegments, bytes)
Arena-->>Builder: TextArenaSlice {off, len}
Builder->>Builder: appendDrawTextCommand(..., byteOff, byteLen)
Builder->>Buffer: write header, commands, and arena bytes
Buffer-->>Client: encoded drawlist
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/drawlist/__tests__/builder.alignment.test.ts (1)
267-274:⚠️ Potential issue | 🟡 MinorFormatting needs to be applied in this updated assertion block.
CI reports formatter-required changes in this range; please run the formatter so the test file passes pipeline checks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/drawlist/__tests__/builder.alignment.test.ts` around lines 267 - 274, The updated assertion block around the test "reuseOutputBuffer keeps odd-string padding zeroed across reset/build cycles" is not properly formatted; run the project's code formatter (e.g., npm run format or prettier) and reformat the assertions that reference h, bytes, h.stringsBytesLen, h.stringsBytesOffset and the String.fromCharCode(...) check so the file packages/core/src/drawlist/__tests__/builder.alignment.test.ts adheres to the repository formatting rules and the CI will pass.
🧹 Nitpick comments (1)
packages/core/package.json (1)
19-19: Build before benchmarking to avoid stale/missingdistartifacts.
bench-text-arena.mjsimports from../dist/...(Line 3 in the script), so running this on a clean checkout (or after source edits) can fail or benchmark stale output.♻️ Proposed script tweak
- "bench:text-arena": "node scripts/bench-text-arena.mjs" + "bench:text-arena": "npm run build && node scripts/bench-text-arena.mjs"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/package.json` at line 19, The bench script ("bench:text-arena") runs node scripts/bench-text-arena.mjs which imports from ../dist, causing failures or stale benchmarks on clean checkouts; update the "bench:text-arena" npm script to first build the package (run the existing build script, e.g., "npm run build" or the monorepo build command) before invoking node scripts/bench-text-arena.mjs so that ../dist is fresh and present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/scripts/bench-text-arena.mjs`:
- Around line 84-86: The three-line block with the bench calls (symbols: bench,
legacy, arena, legacyEncodeFrame, arenaEncodeFrame) is misformatted and failing
CI; run the repository's code formatter (e.g., prettier/format script) on
packages/core/scripts/bench-text-arena.mjs (or run the project’s lint/format npm
script) to reformat those lines so they match project style, then commit the
resulting changes so CI passes.
- Around line 5-7: The benchmark reads SEGMENTS, ITERATIONS, and WARMUP from env
but doesn't validate them, causing avgMs (and other metrics) to divide by zero
or produce NaN/Infinity; update the parsing/initialization for SEGMENTS,
ITERATIONS, and WARMUP to ensure they are finite positive integers (fallback to
the existing defaults if Number(...) yields NaN or non-positive/Infinity), and
add a runtime guard before computing avgMs (and any other division by
iterations) to ensure iterations > 0 — if invalid, either clamp to 1 or emit a
clear error and exit; reference the constants SEGMENTS, ITERATIONS, WARMUP and
the avgMs calculation to locate where to add validation and the guard.
In `@packages/core/src/drawlist/__tests__/builder.text-arena.equivalence.test.ts`:
- Around line 90-108: The failing CI formatter error is due to formatting in the
decodeTextSlice function; open the decodeTextSlice implementation (references:
function decodeTextSlice, Header type h, helper u32, and TextDecoder usage) and
reformat the block to match project style (or run the project's
formatter/Prettier/ESLint --fix) so spacing/line breaks around the return and
asserts conform to the codebase rules, then commit the formatted file.
In `@packages/core/src/drawlist/builderBase.ts`:
- Around line 1054-1061: The CI formatter flagged formatting around the
calculations for string/span offsets in builderBase.ts — reformat the block
defining persistentStringsCount, hasTextArenaSpan, stringsCount,
stringsSpanBytes, stringsSpanOffset, stringsBytesOffset, and stringsBytesAligned
to match the project's code style (run the repository formatter or adjust
whitespace/line breaks to align with surrounding code), preserving the exact
logic and variable names (persistentStringsCount, hasTextArenaSpan,
stringsCount, stringsSpanBytes, stringsSpanOffset, stringsBytesOffset,
stringsBytesAligned).
- Around line 157-160: Reformat the TextEncoder initialization block to match
the repo's formatter style: adjust spacing and line breaks around the typed
globalThis cast and the ternary so the declaration of textEncoderCtor and
assignment to this.encoder follow the project's formatting (e.g., collapse into
fewer lines or align parentheses as other files do); ensure the typed cast to
Readonly<{ TextEncoder?: new () => Utf8Encoder }> remains, and keep the
identifiers textEncoderCtor, Utf8Encoder and this.encoder unchanged. Run the
project's automatic formatter (e.g., Prettier/format task) after editing to
confirm CI will pass.
In `@packages/core/src/drawlist/textArena.ts`:
- Around line 38-47: The constructor parameter order in TextArena is wrong for
the linter: move the required encoder parameter (Utf8EncoderInto) before the
optional initialCapacity with its default (DEFAULT_INITIAL_CAPACITY) in the
TextArena constructor signature, update the internal variable names/uses
(initialCapacity, encoder, buf) accordingly, and then update all call sites that
construct TextArena to pass the encoder first and the optional initialCapacity
second so the capacity selection logic (Number.isFinite/Number.isInteger checks
and Uint8Array allocation) remains unchanged.
---
Outside diff comments:
In `@packages/core/src/drawlist/__tests__/builder.alignment.test.ts`:
- Around line 267-274: The updated assertion block around the test
"reuseOutputBuffer keeps odd-string padding zeroed across reset/build cycles" is
not properly formatted; run the project's code formatter (e.g., npm run format
or prettier) and reformat the assertions that reference h, bytes,
h.stringsBytesLen, h.stringsBytesOffset and the String.fromCharCode(...) check
so the file packages/core/src/drawlist/__tests__/builder.alignment.test.ts
adheres to the repository formatting rules and the CI will pass.
---
Nitpick comments:
In `@packages/core/package.json`:
- Line 19: The bench script ("bench:text-arena") runs node
scripts/bench-text-arena.mjs which imports from ../dist, causing failures or
stale benchmarks on clean checkouts; update the "bench:text-arena" npm script to
first build the package (run the existing build script, e.g., "npm run build" or
the monorepo build command) before invoking node scripts/bench-text-arena.mjs so
that ../dist is fresh and present.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/testkit/fixtures/zrdl-v1/golden/clip_nested.binis excluded by!**/*.binpackages/testkit/fixtures/zrdl-v1/golden/draw_text_interned.binis excluded by!**/*.binpackages/testkit/fixtures/zrdl-v1/golden/fill_rect.binis excluded by!**/*.bin
📒 Files selected for processing (20)
packages/core/package.jsonpackages/core/scripts/bench-text-arena.mjspackages/core/src/drawlist/__tests__/builder.alignment.test.tspackages/core/src/drawlist/__tests__/builder.golden.test.tspackages/core/src/drawlist/__tests__/builder.graphics.test.tspackages/core/src/drawlist/__tests__/builder.limits.test.tspackages/core/src/drawlist/__tests__/builder.reset.test.tspackages/core/src/drawlist/__tests__/builder.round-trip.test.tspackages/core/src/drawlist/__tests__/builder.string-cache.test.tspackages/core/src/drawlist/__tests__/builder.string-intern.test.tspackages/core/src/drawlist/__tests__/builder.text-arena.equivalence.test.tspackages/core/src/drawlist/__tests__/builder.text-run.test.tspackages/core/src/drawlist/__tests__/builder.validate-caps.test.tspackages/core/src/drawlist/__tests__/builder_style_attrs.test.tspackages/core/src/drawlist/builder.tspackages/core/src/drawlist/builderBase.tspackages/core/src/drawlist/index.tspackages/core/src/drawlist/textArena.tspackages/core/src/drawlist/types.tspackages/core/src/index.ts
packages/core/src/drawlist/__tests__/builder.text-arena.equivalence.test.ts
Show resolved
Hide resolved
![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: 0931e4afd7
ℹ️ 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".
Owner
Author
|
Addressed all outstanding review comments and pushed follow-ups:\n\n- : formatter/lint feedback + benchmark script hardening\n - bench script now builds first (avoids stale/missing )\n - env inputs are validated as positive integers\n - added runtime guard for invalid iteration counts\n - constructor param order fixed for \ and call sites updated\n - formatter fixes applied to flagged files\n\n- : safety preflights for large text allocations\n - \ now bounds-checks estimated total size before reserving\n - transient text allocation preflights UTF-8 length + estimated frame size before encoding\n - added validation tests for both failure paths\n\nVerification:\n- \ (packages/core) ✅\n- drawlist tests: \ passed ✅\n- \ ✅\n\nAlso resolved the two open Codex review threads on this PR. |
Owner
Author
|
Addressed all outstanding review comments and pushed follow-ups:
Verification:
Also resolved the two previously open Codex review threads on this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/drawlist/builderBase.ts (1)
466-490:⚠️ Potential issue | 🟠 MajorRequire full test suite run evidence before merge.
Binary protocol builder changes are a danger zone. While drawlist tests provide unit coverage, the full test suite (
node scripts/run-tests.mjs) must run before merge to catch subtle regressions across rendering, layout, and runtime integration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/drawlist/builderBase.ts` around lines 466 - 490, The PR modifies binary protocol builder logic (see buildWithVersion and buildIntoWithVersion in builderBase.ts) which can cause subtle regressions; before merging, run the full test suite via the project's canonical runner (node scripts/run-tests.mjs) and include the full test output as proof in the PR, addressing any failing tests (especially around rendering, layout, and runtime integration) and not just the drawlist unit tests; attach the test log and any fixes to failing targets before approving the merge.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/drawlist/builderBase.ts`:
- Around line 748-754: allocTextSlice and reserveTextArena bypass the
maxStringBytes limit causing textArena.byteLength() + newStringBytes to exceed
the allowed budget; before allocating or reserving, compute the UTF-8 byte
length of the incoming text (using the existing encoder) and check that
textArena.byteLength() + stringBytesLen <= maxStringBytes; if the check fails
call this.fail("ZRDL_INTERNAL", "...") and return null (or false) to prevent
allocation. Apply the same guard to reserveTextArena and any other arena-backed
text allocation paths referenced (e.g., the other alloc/reserve sites and
internString interactions) so all arena allocations respect maxStringBytes.
---
Outside diff comments:
In `@packages/core/src/drawlist/builderBase.ts`:
- Around line 466-490: The PR modifies binary protocol builder logic (see
buildWithVersion and buildIntoWithVersion in builderBase.ts) which can cause
subtle regressions; before merging, run the full test suite via the project's
canonical runner (node scripts/run-tests.mjs) and include the full test output
as proof in the PR, addressing any failing tests (especially around rendering,
layout, and runtime integration) and not just the drawlist unit tests; attach
the test log and any fixes to failing targets before approving the merge.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/core/package.jsonpackages/core/scripts/bench-text-arena.mjspackages/core/src/drawlist/__tests__/builder.alignment.test.tspackages/core/src/drawlist/__tests__/builder.text-arena.equivalence.test.tspackages/core/src/drawlist/builderBase.tspackages/core/src/drawlist/textArena.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core/src/drawlist/tests/builder.alignment.test.ts
- packages/core/src/drawlist/textArena.ts
- packages/core/package.json
- packages/core/scripts/bench-text-arena.mjs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/core/src/drawlist/builderBase.ts (1)
207-230:⚠️ Potential issue | 🟠 Major
maxStringBytesis still bypassed for arena-backed text.
reserveTextArena()/allocTextSlice()enforcemaxDrawlistBytes, but layout/build combine arena bytes with persistent string bytes into one strings payload. This still allowsmaxStringBytesto be exceeded without tripping a cap.💡 Suggested guard (combined string budget)
@@ reserveTextArena(bytes: number): void { @@ const nextRequired = this.textArena.byteLength() + bi; + const projectedStringBytes = nextRequired + this.stringBytesLen; + if (projectedStringBytes > this.maxStringBytes) { + this.fail( + "ZRDL_TOO_LARGE", + `reserveTextArena: maxStringBytes exceeded (bytes=${projectedStringBytes}, max=${this.maxStringBytes})`, + ); + return; + } @@ protected allocTextSlice(text: string): TextArenaSlice | null { @@ const nextArenaLen = this.textArena.byteLength() + utf8Len; + const projectedStringBytes = nextArenaLen + this.stringBytesLen; + if (projectedStringBytes > this.maxStringBytes) { + this.fail( + "ZRDL_TOO_LARGE", + `drawText: maxStringBytes exceeded (bytes=${projectedStringBytes}, max=${this.maxStringBytes})`, + ); + return null; + } @@ const stringsBytesRawLen = textArenaBytesLen + persistentStringsBytesLen; + if (stringsBytesRawLen > this.maxStringBytes) { + return { + ok: false, + error: { + code: "ZRDL_TOO_LARGE", + detail: `build: maxStringBytes exceeded (bytes=${stringsBytesRawLen}, max=${this.maxStringBytes})`, + }, + }; + }Also applies to: 548-560, 760-789
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/drawlist/builderBase.ts` around lines 207 - 230, reserveTextArena and allocTextSlice only check maxDrawlistBytes, allowing combined arena-backed text + persistent string bytes to exceed maxStringBytes; update reserveTextArena (and the same checks in allocTextSlice and the other noted locations) to compute a combined string budget using this.textArena.byteLength() + arenaAllocation + persistentStringsTotal (use the same persistent string accounting used elsewhere) and fail with the appropriate error when that combined total would exceed maxStringBytes rather than only checking maxDrawlistBytes; reference validateParams/requireI32NonNeg, estimateTotalSizeWithArenaState, textArena.reserve, maybeFailTooLargeAfterWrite and ensure the new guard mirrors the failure behavior used for maxStringBytes elsewhere.
🧹 Nitpick comments (1)
packages/core/src/drawlist/builderBase.ts (1)
245-251:allocTextSlice()emitsdrawText-specific error details for multiple callers.This helper is used by both
drawTextandaddTextRunBlob, but failures always reportdrawText: ..., which makes diagnostics misleading.🧹 Suggested refactor for caller-specific errors
- const textSlice = this.allocTextSlice(text); + const textSlice = this.allocTextSlice("drawText", text); @@ - const textSlice = this.allocTextSlice(seg0.text); + const textSlice = this.allocTextSlice("addTextRunBlob", seg0.text); @@ - protected allocTextSlice(text: string): TextArenaSlice | null { + protected allocTextSlice(method: "drawText" | "addTextRunBlob", text: string): TextArenaSlice | null { @@ - this.fail("ZRDL_INTERNAL", "drawText: TextEncoder is not available"); + this.fail("ZRDL_INTERNAL", `${method}: TextEncoder is not available`); @@ - `drawText: maxDrawlistBytes exceeded (utf8Len=${utf8Len}, max=${this.maxDrawlistBytes})`, + `${method}: maxDrawlistBytes exceeded (utf8Len=${utf8Len}, max=${this.maxDrawlistBytes})`, @@ - `drawText: maxDrawlistBytes exceeded (estimatedTotal=${nextEstimatedTotal}, max=${this.maxDrawlistBytes})`, + `${method}: maxDrawlistBytes exceeded (estimatedTotal=${nextEstimatedTotal}, max=${this.maxDrawlistBytes})`,Also applies to: 363-368, 760-784
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/drawlist/builderBase.ts` around lines 245 - 251, allocTextSlice currently emits errors labeled for drawText even when called from other callers like addTextRunBlob; update the callers (e.g., drawText and addTextRunBlob where allocTextSlice is invoked and lines around appendDrawTextCommand/encodeDrawTextStyle) to detect a null/this.error result and, instead of relying on allocTextSlice to log, have each caller produce its own specific error message (e.g., "drawText: ..." vs "addTextRunBlob: ...") before returning; remove or demote the hardcoded drawText text in allocTextSlice's error path so allocTextSlice only returns failure, and ensure callers construct and log the appropriate, caller-specific diagnostic when they see textSlice === null or this.error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/drawlist/builderBase.ts`:
- Around line 559-560: The current align4 implementation uses bitwise arithmetic
which can overflow due to JS coercion to signed 32-bit ints; update the align4
function (referenced where stringsBytesLen is computed and used) to a
modulo-based implementation that returns n if n % 4 === 0 else n + (4 - (n % 4))
so it works correctly for values near INT32_MAX; then ensure the call site
computing stringsBytesLen (the expression stringsCount === 0 ? 0 :
align4(stringsBytesRawLen)) remains unchanged so cursor arithmetic (cursor +=
stringsBytesLen) uses the safe alignment result.
---
Duplicate comments:
In `@packages/core/src/drawlist/builderBase.ts`:
- Around line 207-230: reserveTextArena and allocTextSlice only check
maxDrawlistBytes, allowing combined arena-backed text + persistent string bytes
to exceed maxStringBytes; update reserveTextArena (and the same checks in
allocTextSlice and the other noted locations) to compute a combined string
budget using this.textArena.byteLength() + arenaAllocation +
persistentStringsTotal (use the same persistent string accounting used
elsewhere) and fail with the appropriate error when that combined total would
exceed maxStringBytes rather than only checking maxDrawlistBytes; reference
validateParams/requireI32NonNeg, estimateTotalSizeWithArenaState,
textArena.reserve, maybeFailTooLargeAfterWrite and ensure the new guard mirrors
the failure behavior used for maxStringBytes elsewhere.
---
Nitpick comments:
In `@packages/core/src/drawlist/builderBase.ts`:
- Around line 245-251: allocTextSlice currently emits errors labeled for
drawText even when called from other callers like addTextRunBlob; update the
callers (e.g., drawText and addTextRunBlob where allocTextSlice is invoked and
lines around appendDrawTextCommand/encodeDrawTextStyle) to detect a
null/this.error result and, instead of relying on allocTextSlice to log, have
each caller produce its own specific error message (e.g., "drawText: ..." vs
"addTextRunBlob: ...") before returning; remove or demote the hardcoded drawText
text in allocTextSlice's error path so allocTextSlice only returns failure, and
ensure callers construct and log the appropriate, caller-specific diagnostic
when they see textSlice === null or this.error.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/drawlist/__tests__/builder.validate-caps.test.tspackages/core/src/drawlist/builderBase.ts
Owner
Author
|
Addressed the Changes in
Validation:
|
This was referenced Feb 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
npm run build(repo root): passpackages/coredrawlist tests: 417/417 passBenchmark
segments=20000,iterations=407.337ms1.875ms20001 -> 13.91xSummary by CodeRabbit
New Features
Performance Improvements
Chores