Skip to content
Merged
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
16 changes: 15 additions & 1 deletion src/core/compact_object.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1633,11 +1633,25 @@ MemoryResource* CompactObj::memory_resource() {
}

bool CompactObj::JsonConsT::DefragIfNeeded(PageUsage* page_usage) {
if (JsonType* old = json_ptr; ShouldDefragment(page_usage)) {
if (ShouldDefragment(page_usage)) {
const MiMemoryResource* mr = static_cast<MiMemoryResource*>(memory_resource());

const int64_t before = static_cast<int64_t>(mr->used());
DCHECK_GE(before, 0) << "Memory usage is more than int64_t max value";

JsonType* old = json_ptr;
json_ptr = AllocateMR<JsonType>(DeepCopyJSON(old));
DeleteMR<JsonType>(old);

const int64_t after = static_cast<int64_t>(mr->used());
DCHECK_GE(after, 0) << "Memory usage is more than int64_t max value";

if (const int64_t delta = after - before; delta != 0) {
bytes_used = UpdateSize(bytes_used, delta);
}
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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can db_table be nullptr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably not but I added it to be defensive as we access a field via the pointer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i usually do not like these things as it raises questions where this state is possible. it's better to DCHECK to assert the precondition than have such ifs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, I will remove it in a follow up

db_table->stats.AddTypeMemoryUsage(it->second.ObjType(), delta);
}
}
});
traverses_count++;
Expand Down
Loading