perf(surface): batch immutable text writes - #91
ben-ranford wants to merge 3 commits into
Conversation
ben-ranford
added
area:rendering
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 3844e48ca2
ℹ️ 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".
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate issues remain in zero-size handling and regression-test coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Batches immutable Surface.WithText updates through a seeded Builder to reduce allocations while preserving rendering and interning behavior.
Changes:
- Shares normalized text traversal between immutable and Builder paths.
- Seeds existing style and link indexes.
- Adds parity, immutability, allocation, and patch-roundtrip tests.
File summaries
| File | Description |
|---|---|
surface/surface.go |
Implements batched immutable text writes. |
surface/surface_test.go |
Adds regression and allocation coverage. |
Review details
Suppressed comments (1)
surface/surface_test.go:126
- This snapshot is only a shallow copy, so
beforeshares the cells, styles, and links backing arrays withbase. A regression that mutates the input in place would mutate both values and still pass this immutability assertion; take a deep snapshot before invokingWithTextso the acceptance check can detect that failure.
before := base
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
1370e78 to
9a8f590
Compare
Summary
Closes #61.
Surface.WithTextpreviously cloned and finalized the full immutable surface for every grapheme cluster. It now creates one seeded Builder over a single clone, uses the same normalized cluster traversal asBuilder.WithText, and finalizes once. The seeded indexes retain the existing first-match style and link IDs, including surfaces reconstructed from a patch with duplicate interned entries.The regression coverage compares the immutable and seeded Builder results across tabs, clipping, wide graphemes, links, and successive style writes; confirms input immutability and diff/patch round trips; and constrains the allocation envelope. On the checked workload, immutable
WithTextuses 16 allocations and the Builder uses 15.Scope firewall
A review item is in scope only when it directly prevents #61 acceptance and is an incremental correction in
surface/surface.goorsurface/surface_test.go. Sharing a file or a rendering concern is not sufficient.Validation
go test ./surface -run 'TestWithText' -count=1 -vgo test -race ./surface -run 'TestWithText' -count=1go test ./surface -count=1git diff --checkmake ci→ exit0(captured in/tmp/stave-61-make-ci-final.logand/tmp/stave-61-make-ci-final.exit)Release Notes
Improves the allocation behavior of immutable
Surface.WithTextwithout changing its public signature or rendering semantics.