Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/core/compact_object.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1633,11 +1633,18 @@ MemoryResource* CompactObj::memory_resource() {
}

bool CompactObj::JsonConsT::DefragIfNeeded(PageUsage* page_usage) {
if (JsonType* old = json_ptr; ShouldDefragment(page_usage)) {
JsonType* old = json_ptr;
if (ShouldDefragment(page_usage)) {
const MiMemoryResource* mr = static_cast<MiMemoryResource*>(memory_resource());
const ssize_t before = mr->used();
json_ptr = AllocateMR<JsonType>(DeepCopyJSON(old));
DeleteMR<JsonType>(old);
if (const ssize_t delta = mr->used() - before; delta != 0) {
bytes_used += delta;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bytes_used is a size_t, while delta is ssize_t and may be negative; using += here can cause unsigned wraparound when defragmentation reduces size, leading to incorrect memory accounting.

🤖 Was this useful? React with 👍 or 👎

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is intentionally ssize_t, delta is expected to be negative. It should not lead to a wraparound though as delta is only caused by reallocation.

}
return true;
}

return false;
}

Expand Down
16 changes: 15 additions & 1 deletion src/core/page_usage_stats_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -190,17 +190,31 @@ TEST_F(PageUsageStatsTest, JSONCons) {
// still fail. This is because freeing the compact object code path takes the wrong branch based
// on encoding. The flat encoding was tested manually adjusting this same test with changed
// encoding.
std::string_view data{R"#({"data": "some", "count": 1, "checked": false})#"};
std::string data = R"({"contents":[)";
for (size_t i = 0; i < 1000; ++i) {
const auto si = std::to_string(i);
data += R"({"id":)" + si + R"(,"class":"v___)" + si + R"("})";
if (i < 999) {
data += ",";
}
}
data += R"(], "data": "some", "count": 1, "checked": false})";

auto* mr = static_cast<MiMemoryResource*>(CompactObj::memory_resource());
size_t before = mr->used();

auto parsed = ParseJsonUsingShardHeap(data);
EXPECT_TRUE(parsed.has_value());

c_obj_.SetJson(std::move(parsed.value()));
c_obj_.SetJsonSize(mr->used() - before);
EXPECT_GT(c_obj_.MallocUsed(), 0);

PageUsage p{CollectPageStats::YES, 0.1};
p.SetForceReallocate(true);

c_obj_.DefragIfNeeded(&p);
EXPECT_GT(c_obj_.MallocUsed(), 0);

const auto stats = p.CollectedStats();
EXPECT_GT(stats.pages_scanned, 0);
Expand Down
8 changes: 7 additions & 1 deletion src/server/engine_shard.cc
Original file line number Diff line number Diff line change
Expand Up @@ -351,14 +351,20 @@ std::optional<CollectedPageStats> EngineShard::DoDefrag(CollectPageStats collect
uint64_t attempts = 0;

PageUsage page_usage{collect_page_stats, threshold};
DbTable* db_table = slice.GetDBTable(defrag_state_.dbid);
do {
cur = prime_table->Traverse(cur, [&](PrimeIterator it) {
// for each value check whether we should move it because it
// seats on underutilized page of memory, and if so, do it.
bool did = it->second.DefragIfNeeded(&page_usage);
const ssize_t original_size = it->second.MallocUsed();
const bool did = it->second.DefragIfNeeded(&page_usage);
attempts++;
if (did) {
reallocations++;
if (const ssize_t delta = it->second.MallocUsed() - original_size;
delta != 0 && db_table != nullptr) {
db_table->stats.AddTypeMemoryUsage(it->second.ObjType(), delta);
}
}
});
traverses_count++;
Expand Down
Loading