Fake PR to test integrate oah set branch#7247
Conversation
BorysTheDev
changed the title
There was a problem hiding this comment.
Pull request overview
Integrates the runtime-selectable OAHSet backend for kEncodingStrMap2 SETs, updating server logic to operate on either StringSet or OAHSet via a shared dispatch layer.
Changes:
- Introduces
dfly::g_use_oah_set,VisitSet(), andKey()helpers to abstract overStringSetvsOAHSet. - Updates SET read/write paths, RDB save/load, journaling, and generic container iteration to use
VisitSet()and set-time handling. - Extends async deletion and
DEBUG SHRINKlogic to attempt to support OAHSet-backed sets.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/server/zset_family.cc | Switches dense-set time-touch to VisitSet() dispatch. |
| src/server/test_utils.cc | Plumbs --use_oah_set into g_use_oah_set for tests. |
| src/server/set_family.h | Changes ConvertToStrSet to return void* (StringSet/OAHSet). |
| src/server/set_family.cc | Adds --use_oah_set, uses VisitSet() throughout SET ops, and builds dense sets as StringSet/OAHSet. |
| src/server/server_family.cc | Adapts DEBUG SHRINK to handle OAHSet-backed SETs. |
| src/server/rdb_save.cc | Uses VisitSet() for SET expiry/type decisions and serialization. |
| src/server/rdb_load.cc | Loads dense sets as either StringSet or OAHSet depending on g_use_oah_set. |
| src/server/journal/cmd_serializer.cc | Uses VisitSet() to disable/restore lazy expiry during set serialization. |
| src/server/generic_family.cc | Uses VisitSet() to set dense-set time before iterating set elements. |
| src/server/dfly_main.cc | Initializes g_use_oah_set from --use_oah_set at startup. |
| src/server/db_slice.cc | Extends async deletion to support OAHSet via templated deleter callbacks. |
| src/server/container_utils.cc | Iterates dense sets via VisitSet() and Key() extraction. |
| src/server/collection_family_fallback.cc | Updates fallback signature for ConvertToStrSet to void*. |
| src/core/oah_set_test.cc | Renames alloc accounting APIs in tests to ObjMallocUsed/SetMallocUsed. |
| src/core/oah_set.h | Adds VisitSet(), Key(), inline g_use_oah_set, and iterator expiry-on-advance. |
| src/core/compact_object.cc | Uses VisitSet() for freeing, defrag, and size computation of dense sets. |
| auto step = +[](ClearNode* n) { | ||
| auto* s = static_cast<Set*>(n->ds); | ||
| n->cursor = s->ClearStep(n->cursor, kClearStepSize); | ||
| if (n->cursor == s->BucketCount()) { | ||
| CompactObj::DeleteMR<Set>(s); | ||
| return true; | ||
| } | ||
| return false; |
| auto schedule = [](auto* ds) { | ||
| uint32_t next = ds->ClearStep(0, 512); | ||
| if (next < ds->BucketCount()) | ||
| AsyncDeleter::EnqueDeletion(next, ds); | ||
| else | ||
| CompactObj::DeleteMR<std::remove_pointer_t<decltype(ds)>>(ds); | ||
| }; |
| size_t bucket_bytes_before = ds->BucketCount() * sizeof(void*); | ||
| size_t optimal_size = std::max(size_t(8), absl::bit_ceil(ds->UpperBoundSize())); | ||
| ds->Shrink(optimal_size); | ||
| size_t bucket_bytes_after = ds->BucketCount() * sizeof(void*); |
🤖 Augment PR SummarySummary: Makes the dense SET backing implementation runtime-selectable, enabling Changes:
Technical Notes: 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| auto step = +[](ClearNode* n) { | ||
| auto* s = static_cast<Set*>(n->ds); | ||
| n->cursor = s->ClearStep(n->cursor, kClearStepSize); | ||
| if (n->cursor == s->BucketCount()) { |
There was a problem hiding this comment.
For OAHSet, ClearStep() advances cursor up to Capacity() (entries_.size), but this code compares against BucketCount() (2^capacity_log) which is smaller; that can terminate early and delete the set while displacement buckets still contain live entries. This looks like it can leak memory and/or leave dangling state during async deletion.
Severity: high
Other Locations
src/server/db_slice.cc:1843src/server/db_slice.cc:1846
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| PrimeValue& pv = it_res.it->second; | ||
| DenseSet* ds = static_cast<DenseSet*>(pv.RObjPtr()); | ||
| ds->set_time(MemberTimeSeconds(t->GetDbContext().time_now_ms)); | ||
| size_t bucket_bytes_before = ds->BucketCount() * sizeof(void*); |
There was a problem hiding this comment.
bucket_bytes_before/after uses BucketCount() * sizeof(void*), but OAHSet allocates Capacity() buckets of OAHEntry (including the displacement area), so the reported reclaimed bytes for SHRINK will be inaccurate under --use_oah_set. Consider ensuring the reported delta matches the container’s actual bucket-storage allocation.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| LOG(WARNING) << "SWAP is enabled. Consider disabling it when running Dragonfly."; | ||
|
|
||
| dfly::max_memory_limit = absl::GetFlag(FLAGS_maxmemory); | ||
| dfly::g_use_oah_set = absl::GetFlag(FLAGS_use_oah_set); |
There was a problem hiding this comment.
The PR title/description look like placeholders and don’t describe the change intent, which makes the review/audit trail hard to follow. Please align with the repo PR guideline that titles be descriptive and summaries explain what/why (Rule: AGENTS.md).
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
test changes from #7246