Repository navigation
Conversation
Clay looked up cached text measurements by hash alone, so two strings whose hashes collide shared one measurement: the second was sized and wrapped as the first, in the same frame or a later one. A Clay patch now also compares the text bytes and config on lookup. Each Term keeps copies of cached keys in its own storage, because a frame's wire bytes do not outlive its render. Storage is reclaimed by resetting Clay's cache before a layout that might not fit, and a frame larger than the storage keys on its own bytes, with the cache cleared once it has been presented.
Changeset suggestionThe current changeset no longer matches this PR. This review contains a corrected replacement. Why: Changeset package scope does not match the affected packages confidently. Changeset is not sufficiently consumer-focused. View the proposed changeset---
'@bomb.sh/tty': patch
---
TODO: Write changeset message (inference failed: HTTP 401: {"error":{"message":"User not found.","code":401}})Review this changeset manually If this draft is correct, react with 🚀 and Cooper will commit it to this branch. |
![bombshell-cooper[bot]](https://avatars.githubusercontent.com/in/4226441?s=60&v=4){kind=small}
| @@ -0,0 +1,5 @@ | |||
| --- | |||
There was a problem hiding this comment.
Changeset needs revision.
Changeset package scope does not match the affected packages confidently. Changeset is not sufficiently consumer-focused.
View the proposed replacement
---
'@bomb.sh/tty': patch
---
TODO: Write changeset message (inference failed: HTTP 401: {"error":{"message":"User not found.","code":401}})
commit: |
There was a problem hiding this comment.
I had a look at xxhash.com as suggested by the original Clay author in one of the linked issues, and ran a prototype to see if I could get it to work. It did.
Could we use XXH3-128 for the measurement cache instead of retaining copies of the text? We could pin xxHash as a git submodule and include its header through the existing Clay patch mechanism.
The patch would store and compare the full 128-bit fingerprint, along with text length and the relevant font settings. Clay could still derive its bucket index from part of the hash.
This would have two main benefits:
• Significantly lower memory usage: approximately 192 KiB of additional cache storage at maximum capacity, avoiding the roughly 2 MiB text-copy arena.
• The fix stays entirely within Clay: apart from adding the dependency and patch, no changes to tty’s renderer or memory-management code would be needed. There would be no key-copying callback, storage rollover, or borrowed-key lifetime to manage.
In total, it looks like it would add around 3 KiB to the bundled wasm.ts.
The tradeoff is that a 128-bit fingerprint makes accidental collisions possible, but extraordinarily unlikely.... as in like really unlikely, whereas comparing the original bytes guarantees correct collision handling. If that tradeoff is acceptable, then it would be substantially simpler to maintain.
| **Measurement cache.** Clay caches each text's measurement under a hash of its | ||
| content and text config. A local Clay patch reuses a cached measurement only | ||
| when the text bytes and config also match, so strings whose hashes collide are | ||
| each measured as themselves. A frame's text bytes do not outlive its render, so | ||
| each renderer instance copies the keys of cached measurements into a fixed | ||
| region of its own state, clearing Clay's cache before any layout whose text | ||
| might not fit there. A frame too large for that region keys its measurements on | ||
| its own bytes, and the cache is cleared once that frame is presented. | ||
|
|
There was a problem hiding this comment.
As far as I can tell, this is all a private implementation detail and so should not be in the render-spec
What does this PR do?
Fixes incorrect text widths caused by hash collisions in Clay’s measurement cache. Two public ASCII strings with widths 57 and 91 share a 32-bit cache ID; measuring them together or across calls can return 57 for both and lose a drawn text tail.
Refs nicbarker/clay#664, which includes a standalone C reproduction against current Clay main.
The Clay patch verifies text/config equality after a hash match. tty owns the cached key bytes per instance and copies them only when a cache entry is published. Every copy checks remaining storage; oversized frames borrow their own immutable input bytes and clear those keys after presentation. The existing Clay patch mechanism and submodule revision are retained. Public APIs are unchanged.
Each Term reserves 2,031,616 bytes for keys, matching its nominal transfer budget. Independent local measurements found 2 MiB additional allocated WASM memory at 80×24, about 3.4 µs added ordinary-frame cost, and near-capacity rendering increasing from 7.93 to 22.14 ms. Startup samples overlapped. These are measured correctness trade-offs, not performance improvements.
Five new regressions cover same-frame/cross-frame collisions, retained storage reuse, large frames after resize and capacity-refusal input safety. The corrected width cases pass 9/9; all five new cases fail on pristine upstream. Removing storage bounds makes the rollover test trap, confirming the safety guard is needed.
Local validation at signed
b1f96bbcae7ebf8be8c88f5072a3e4e985a79145:deno task fmt,deno task fmt:checkanddeno task verifypass.The independent acceptance/code review passed at this exact commit, including an equal-byte collision probe that fails if only the borrowed-key reset is removed. The final independent Fable review also found no blocking defect, but its unchanged report was rejected by the workflow because the summary is 1504 characters against a 1,500-character limit. Its original result and rejection are retained; no schema exception or replacement review was applied. This PR remains draft with that review-record hold and the Size Report permission failure visible. No check was bypassed or rerun.
Type of change
Checklist
deno task test)deno task fmt,deno task fmt:check)AI-generated code disclosure