Skip to content

Commit 9decf85

Browse files
committed
MB-41804: Correct replica overhead tracking
Mad-hatter uses MemoryTrackingAllocator to accurately account for the overhead of checkpoint overheads. This should be used in tracking replica overhead instead of the approximate/manual accounting based on sizeof. Additionally, expelling was not present in alice, where replica overhead tracking was introduced. Replica overhead must also be adjusted during expelling. Change-Id: I6c39874618c4a6479b55324c1e42f5fc3edaa13c Reviewed-on: http://review.couchbase.org/c/kv_engine/+/140256 Well-Formed: Build Bot <[email protected]> Reviewed-by: Dave Rigby <[email protected]> Tested-by: Build Bot <[email protected]>
1 parent 41426c6 commit 9decf85

File tree

3 files changed

+28
-8
lines changed

3 files changed

+28
-8
lines changed

engines/ep/src/checkpoint.cc

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -203,7 +203,11 @@ Checkpoint::Checkpoint(EPStats& st,
203203
highCompletedSeqno(highCompletedSeqno),
204204
memOverheadChangedCallback(memOverheadChangedCallback) {
205205
stats.coreLocal.get()->memOverhead.fetch_add(sizeof(Checkpoint));
206-
memOverheadChangedCallback(sizeof(Checkpoint));
206+
// the memOverheadChangedCallback uses the accurately tracked overhead
207+
// from trackingAllocator. The above memOverhead stat is "manually"
208+
// accounted in queueDirty, and approximates the overhead based on
209+
// key sizes and the size of queued_item and index_entry.
210+
memOverheadChangedCallback(getMemoryOverhead());
207211
}
208212

209213
Checkpoint::~Checkpoint() {
@@ -216,9 +220,9 @@ Checkpoint::~Checkpoint() {
216220
* of queued_items in the checkpoint.
217221
*/
218222
auto queueMemOverhead = sizeof(queued_item) * toWrite.size();
219-
auto overhead = sizeof(Checkpoint) + keyIndexMemUsage + queueMemOverhead;
220-
stats.coreLocal.get()->memOverhead.fetch_sub(overhead);
221-
memOverheadChangedCallback(-ssize_t(overhead));
223+
stats.coreLocal.get()->memOverhead.fetch_sub(
224+
sizeof(Checkpoint) + keyIndexMemUsage + queueMemOverhead);
225+
memOverheadChangedCallback(-getMemoryOverhead());
222226
}
223227

224228
QueueDirtyStatus Checkpoint::queueDirty(const queued_item& qi,
@@ -231,6 +235,14 @@ QueueDirtyStatus Checkpoint::queueDirty(const queued_item& qi,
231235
}
232236

233237
QueueDirtyStatus rv;
238+
// trigger the memOverheadChangedCallback if the overhead is different
239+
// when this helper is destroyed
240+
auto overheadCheck = gsl::finally([pre = getMemoryOverhead(), this]() {
241+
auto post = getMemoryOverhead();
242+
if (pre != post) {
243+
memOverheadChangedCallback(post - pre);
244+
}
245+
});
234246

235247
// Check if the item is a meta item
236248
if (qi->isCheckPointMetaItem()) {
@@ -388,9 +400,8 @@ QueueDirtyStatus Checkpoint::queueDirty(const queued_item& qi,
388400
* item to the queue (toWrite). This is approximated to the
389401
* addition to metaKeyIndex / keyIndex plus sizeof(queued_item).
390402
*/
391-
auto overhead = indexKeyUsage + sizeof(queued_item);
392-
stats.coreLocal.get()->memOverhead.fetch_add(overhead);
393-
memOverheadChangedCallback(overhead);
403+
stats.coreLocal.get()->memOverhead.fetch_add(indexKeyUsage +
404+
sizeof(queued_item));
394405
/**
395406
* Update the total metaKeyIndex / keyIndex memory usage which is
396407
* used when the checkpoint is destructed to manually account

engines/ep/src/checkpoint_manager.cc

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -571,6 +571,15 @@ size_t CheckpointManager::removeClosedUnrefCheckpoints(
571571

572572
CheckpointManager::ExpelResult
573573
CheckpointManager::expelUnreferencedCheckpointItems() {
574+
// trigger the overheadChangedCallback if the overhead is different
575+
// when this helper is destroyed - which occurs _after_ the destruction
576+
// of expelledItems (declared below)
577+
auto overheadCheck = gsl::finally([pre = getMemoryOverhead(), this]() {
578+
auto post = getMemoryOverhead();
579+
if (pre != post) {
580+
overheadChangedCallback(post - pre);
581+
}
582+
});
574583
CheckpointQueue expelledItems;
575584
{
576585
LockHolder lh(queueLock);

engines/ep/tests/module_tests/ephemeral_bucket_test.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,7 @@ TEST_F(EphemeralBucketStatTest, ReplicaMemoryTrackingStateChange) {
241241
setVBucketStateAndRunPersistTask(vbid, vbucket_state_active);
242242

243243
EXPECT_EQ(0, stats.replicaHTMemory);
244-
EXPECT_LT(stats.replicaCheckpointOverhead, 80);
244+
EXPECT_EQ(0, stats.replicaCheckpointOverhead);
245245
}
246246

247247
TEST_F(EphemeralBucketStatTest, ReplicaCheckpointMemoryTracking) {

0 commit comments

Comments
 (0)