use unique pointers as hash set entries - #21
TalBarYakar wants to merge 3 commits into
Conversation
![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 using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2c28cc9. Configure here.
| .get_val(self.as_str()) | ||
| .expect("live string missing from cache") | ||
| .upgrade() | ||
| .0 |
There was a problem hiding this comment.
Concurrent clone races on string cache
High Severity
Heap IString clone now looks up and mutates the global intern table, including Cell refcounts and WeakIString tags. IValue is still Sync, so concurrent clone (and mem_allocated) on the default ThreadUnsafe cache alias a raw &mut HashSet and race on those cells. Previously clone only did an atomic increment and did not touch the cache, so sharing interned strings across threads was defined; this path is now unsound and can corrupt the intern table.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 2c28cc9. Configure here.
| enum StringCache { | ||
| ThreadSafe(Mutex<HashSet<WeakIString>>), | ||
| ThreadUnsafe(HashSet<WeakIString>), | ||
| ThreadUnsafe(UnsafeCell<HashSet<WeakIString>>), |
There was a problem hiding this comment.
One thing that comes to my mind(and I've jsut started reviewing it so take it with a grain of salt).
We mainly use now the ThreadSafe one, primarly because ASM. I don't know if you also need to change something around the ThreadSafe impl, but FYI
AvivDavid23
left a comment
AvivDavid23
left a comment
There was a problem hiding this comment.
Seems to have performance degridation, need to bench it.
Also please check my comments
|
|
||
| struct SharedHeader { | ||
| data: NonNull<Header>, | ||
| rc: Cell<u32>, |
There was a problem hiding this comment.
rc is not Atomic anymore, which is fine with our usage since we are using currently the ThreadSafe varient. If one day we will go back this might bite us
| StringCache::ThreadSafe(_) => true, | ||
| StringCache::ThreadUnsafe(_) => false, | ||
| } | ||
| fn get_cache() -> &'static StringCache { |
There was a problem hiding this comment.
Why not returning mut anymore? Anyway the only usage is mutating it later(maybe I'm missing something)
There was a problem hiding this comment.
Ignore.. Cell provides interior mutability
| *s_c = HashSet::new(); | ||
| } | ||
| } | ||
| // The cache now owns reference counts; live entries must survive a reset. |
There was a problem hiding this comment.
What? Why we don't reset the Cache anymore?
There was a problem hiding this comment.
Reset is part of RedisJSON’s defrag cycle. This change prevents shared strings from being
relocated. How will shared-string defrag remain supported?
| unsafe { self.0.raw_copy() } | ||
| let cache = get_cache_guard(); | ||
| cache | ||
| .get_val(self.as_str()) |
There was a problem hiding this comment.
Now each clone(and drop, function below) is much slower since we first need to hash the str to get the rc..
Not sure about the solution tough, pointing this out
|
|
||
| // Run in its own test process because cache initialization is process-wide. | ||
| #[test] | ||
| #[ignore = "run separately with --ignored --exact"] |
There was a problem hiding this comment.
please verify its running the ci, this specific test
2 participants
Memory impact
The unique-string optimization avoids shared-reference metadata for strings that occur only once, promoting them to the shared representation when reused.
The following comparison excludes unused array/object capacity from both versions and includes global string-table allocations and allocator rounding.
These measurements exclude unused array/object capacity and include global string-table allocations and allocator rounding.
1 million unique strings — 25 bytes each
1 million nested records with unique 25-byte strings
Real-world countries/states/cities JSON
Measured locally on macOS ARM64 with the libc allocator. Savings depend on string lengths and allocator size classes.
Note
High Risk
Core string lifetime, refcounting, and defrag semantics changed with tagged pointers and cache mutex on all clone/drop paths; bugs could cause use-after-free, leaks, or stale pointers under concurrency.
Overview
Reworks non–
thread_safeheap string interning so the global hash set holds unique allocations until a second owner appears, then promotes to a taggedSharedHeaderwith a cache-owned ref count. Per-stringAtomicU32onHeaderis removed; clone/drop/intern take the cache guard and callupgrade/release, demoting back to unique when only the table entry remains.reinit_shared_string_cachenow shrinks the set instead of clearing it, so live strings and shared metadata stay valid.IString::defragin-place relocates only unique entries (updates cache + value); shared strings are left pinned.Docs and
mem_allocatedreflect the smaller unique footprint (tests expect 16 bytes for a lone heap string withoutthread_safe). New tests cover promote/demote, defrag, and optional concurrent promotion.Reviewed by Cursor Bugbot for commit 347962d. Bugbot is set up for automated code reviews on this repo. Configure here.