perf(store): simplify string hash caching in ImmutableMap#7838
perf(store): simplify string hash caching in ImmutableMap#7838steveruizok merged 1 commit intoperformance-stack-1from
Conversation
Remove cache size limits and always cache string hashes: - Cache all strings regardless of length (removed 16-char minimum) - Remove cache eviction logic (no more 255 entry limit) - Simplify to unbounded cache since shape IDs are finite per document Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
5 Skipped Deployments
|
steveruizok
temporarily deployed
to
e2e-dotcom
GitHub Actions
Inactive
steveruizok
temporarily deployed
to
bemo-canary
GitHub Actions
Inactive
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| if (hashed === undefined) { | ||
| hashed = hashString(string) | ||
| if (STRING_HASH_CACHE_SIZE === STRING_HASH_CACHE_MAX_SIZE) { | ||
| STRING_HASH_CACHE_SIZE = 0 |
There was a problem hiding this comment.
Cache returns inherited Object properties
High Severity
stringHashCache is a plain {} and cachedHashString reads via stringHashCache[string], so keys like toString/constructor can resolve to inherited properties instead of undefined. That makes hash() return non-numbers, which then get coerced in bitwise ops and can cause incorrect map behavior (missed lookups/collisions).
Additional Locations (2)
| let hashed = stringHashCache[string] | ||
| if (hashed === undefined) { | ||
| hashed = hashString(string) | ||
| if (STRING_HASH_CACHE_SIZE === STRING_HASH_CACHE_MAX_SIZE) { |
There was a problem hiding this comment.
Prototype pollution via string hash cache
High Severity
Because stringHashCache is a normal object, caching attacker-controlled strings like __proto__ (now included due to caching all strings) can mutate the cache object’s prototype via the __proto__ setter. This can destabilize hashing and is a prototype-pollution vector if hash() is reachable with untrusted input.
Additional Locations (1)
| if (hashed === undefined) { | ||
| hashed = hashString(string) | ||
| if (STRING_HASH_CACHE_SIZE === STRING_HASH_CACHE_MAX_SIZE) { | ||
| STRING_HASH_CACHE_SIZE = 0 |
There was a problem hiding this comment.
Unbounded global cache can leak memory
Medium Severity
Removing the eviction/reset logic makes stringHashCache grow for the entire process lifetime. If hash() sees many distinct strings over time (multiple documents/sessions, transient IDs, or any non-shape keys), the cache retains them indefinitely and can create a memory leak in long-running apps.
Additional Locations (1)
1 participant
Closes #7833. Previously, once the number of shapes grew to 256, we were creating new hashes for all shapes on every frame.
Summary
The previous implementation avoided caching short strings and had an eviction policy. However, in tldraw's use case, the number of unique string keys is bounded by the document's shape/record count, so an unbounded cache is safe and reduces overhead.
Test plan
🤖 Generated with Claude Code
Note
Medium Risk
Changes the hashing hot-path used by
ImmutableMapto always cache string hashes and removes cache eviction, which could increase memory use or alter performance characteristics on workloads with many unique strings.Overview
Simplifies string hash caching in
ImmutableMapby always routing string hashing throughcachedHashString(removing the short-string threshold).Removes the cache size/eviction logic and related constants, making
stringHashCachean unbounded in-memory map that retains all hashed strings for the process lifetime.Written by Cursor Bugbot for commit e3fefe0. This will update automatically on new commits. Configure here.