Skip to content

Commit

Permalink
MB-44539: Reduce cost of MemoryAllocator 'read bytes allocated'
Browse files Browse the repository at this point in the history 
MemoryTrackingAllocator::getBytesAllocated returns a shared_ptr
meaning all calls to getBytesAllocated result in ref count inc/dec.
This is visible in profiling and accumulated 1.2% of a front-end
thread time.

Commit changes getBytesAllocated to return the size so that
functions like Checkpoint::getMemoryOverhead don't copy
shared_ptr objects and inc/dec ref-counts.

Change-Id: I62080b160f1eaa8fde37212a1aeacf5e17ccfa8d
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/148275
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
  • Loading branch information
@jimwwalker @daverigby
jimwwalker authored and daverigby committed Mar 11, 2021
1 parent 113471f commit 3e32796
Show file tree
Hide file tree
Showing 6 changed files with 44 additions and 35 deletions.
10 changes: 5 additions & 5 deletions engines/ep/src/checkpoint.h
View file
Expand Up @@ -616,9 +616,9 @@ class Checkpoint {
// same allocator and therefore getting the bytes allocated for the
// one will include the others.
return sizeof(Checkpoint) +
*(committedKeyIndex.get_allocator().getBytesAllocated()) +
*(toWrite.get_allocator().getBytesAllocated()) +
*(keyIndexKeyTrackingAllocator).getBytesAllocated();
(committedKeyIndex.get_allocator().getBytesAllocated()) +
(toWrite.get_allocator().getBytesAllocated()) +
(keyIndexKeyTrackingAllocator).getBytesAllocated();
}

/**
Expand Down Expand Up @@ -652,12 +652,12 @@ class Checkpoint {

/// @return bytes allocated to keyIndex/metaKeyIndex as a signed type
ssize_t getKeyIndexAllocatorBytes() const {
return ssize_t(*keyIndexTrackingAllocator.getBytesAllocated());
return ssize_t(keyIndexTrackingAllocator.getBytesAllocated());
}

/// @return bytes allocated to the toWrite as a signed type
ssize_t getWriteQueueAllocatorBytes() const {
return *trackingAllocator.getBytesAllocated();
return trackingAllocator.getBytesAllocated();
}

// see member variable definition for info
Expand Down
2 changes: 1 addition & 1 deletion engines/ep/src/checkpoint_manager.cc
View file
Expand Up @@ -708,7 +708,7 @@ CheckpointManager::expelUnreferencedCheckpointItems() {

// Part 2 of calculating the estimate (see comment above).
estimateOfAmountOfRecoveredMemory +=
*(expelledItems.get_allocator().getBytesAllocated());
expelledItems.get_allocator().getBytesAllocated();

/*
* We are now outside of the queueLock when the method exits,
Expand Down
14 changes: 7 additions & 7 deletions engines/ep/tests/module_tests/checkpoint_remover_test.cc
View file
Expand Up @@ -165,8 +165,8 @@ TEST_F(CheckpointRemoverEPTest, CheckpointManagerMemoryUsage) {
}

const auto metaKeyIndexSize =
*(metaKeyIndex.get_allocator().getBytesAllocated()) +
*(keyIndexKeyTrackingAllocator.getBytesAllocated());
metaKeyIndex.get_allocator().getBytesAllocated() +
keyIndexKeyTrackingAllocator.getBytesAllocated();
ASSERT_EQ(expected_size + metaKeyIndexSize,
checkpointManager->getMemoryUsage());

Expand All @@ -187,8 +187,8 @@ TEST_F(CheckpointRemoverEPTest, CheckpointManagerMemoryUsage) {
// the same allocator, retrieving the bytes allocated for the keyIndex,
// will also include the bytes allocated for the other indexes.
const size_t keyIndexSize =
*(committedKeyIndex.get_allocator().getBytesAllocated()) +
*(keyIndexKeyTrackingAllocator.getBytesAllocated());
committedKeyIndex.get_allocator().getBytesAllocated() +
keyIndexKeyTrackingAllocator.getBytesAllocated();
ASSERT_EQ(new_expected_size + keyIndexSize,
checkpointManager->getMemoryUsage());
}
Expand Down Expand Up @@ -284,7 +284,7 @@ TEST_F(CheckpointRemoverEPTest, CursorDropMemoryFreed) {
// Grab the initial size of the keyIndex because on Windows an empty
// std::unordered_map allocated 200 bytes.
const auto initialKeyIndexSize =
*(keyIndex.get_allocator().getBytesAllocated());
keyIndex.get_allocator().getBytesAllocated();
ChkptQueueIterator iterator =
CheckpointManagerTestIntrospector::public_getCheckpointList(
*checkpointManager)
Expand Down Expand Up @@ -336,10 +336,10 @@ TEST_F(CheckpointRemoverEPTest, CursorDropMemoryFreed) {
keyIndexKeyTrackingAllocator),
entry);

const auto keyIndexSize = *(keyIndex.get_allocator().getBytesAllocated());
const auto keyIndexSize = keyIndex.get_allocator().getBytesAllocated();
expectedFreedMemoryFromItems +=
(keyIndexSize - initialKeyIndexSize +
*keyIndexKeyTrackingAllocator.getBytesAllocated());
keyIndexKeyTrackingAllocator.getBytesAllocated());

// Manually handle the slow stream, this is the same logic as the checkpoint
// remover task uses, just without the overhead of setting up the task
Expand Down
10 changes: 5 additions & 5 deletions engines/ep/tests/module_tests/checkpoint_test.cc
View file
Expand Up @@ -1667,7 +1667,7 @@ TEST_P(CheckpointTest, checkpointMemoryTest) {
// Grab the initial size of the keyIndex because on Windows an empty
// std::unordered_map allocated 200 bytes.
const auto initialKeyIndexSize =
*(keyIndex.get_allocator().getBytesAllocated());
keyIndex.get_allocator().getBytesAllocated();
ChkptQueueIterator iterator =
CheckpointManagerTestIntrospector::public_getCheckpointList(
*(this->manager))
Expand Down Expand Up @@ -1711,7 +1711,7 @@ TEST_P(CheckpointTest, checkpointMemoryTest) {
keyIndexKeyTrackingAllocator),
entry);

auto keyIndexSize = *(keyIndex.get_allocator().getBytesAllocated());
auto keyIndexSize = keyIndex.get_allocator().getBytesAllocated();
expectedSize += (keyIndexSize - initialKeyIndexSize);

EXPECT_EQ(expectedSize, this->manager->getMemoryUsage());
Expand Down Expand Up @@ -1747,7 +1747,7 @@ TEST_P(CheckpointTest, checkpointMemoryTest) {
keyIndexKeyTrackingAllocator),
entry);

keyIndexSize = *(keyIndex.get_allocator().getBytesAllocated());
keyIndexSize = keyIndex.get_allocator().getBytesAllocated();
expectedSize += (keyIndexSize - initialKeyIndexSize);

EXPECT_EQ(expectedSize, this->manager->getMemoryUsage());
Expand Down Expand Up @@ -1801,7 +1801,7 @@ TEST_P(CheckpointTest, checkpointTrackingMemoryOverheadTest) {
// Grab the initial size of the keyIndex because on Windows an empty
// std::unordered_map allocated 200 bytes.
const auto initialKeyIndexSize =
*(keyIndex.get_allocator().getBytesAllocated());
keyIndex.get_allocator().getBytesAllocated();

ChkptQueueIterator iterator =
CheckpointManagerTestIntrospector::public_getCheckpointList(
Expand Down Expand Up @@ -1838,7 +1838,7 @@ TEST_P(CheckpointTest, checkpointTrackingMemoryOverheadTest) {
keyIndexKeyTrackingAllocator),
entry);

const auto keyIndexSize = *(keyIndex.get_allocator().getBytesAllocated());
const auto keyIndexSize = keyIndex.get_allocator().getBytesAllocated();
EXPECT_EQ(perElementListOverhead + (keyIndexSize - initialKeyIndexSize),
updatedOverhead - initialOverhead);

Expand Down
22 changes: 11 additions & 11 deletions engines/ep/tests/module_tests/memory_tracking_allocator_test.cc
View file
Expand Up @@ -54,16 +54,16 @@ class MemoryTrackingAllocatorListTest : public ::testing::Test {

// Test empty List
TEST_F(MemoryTrackingAllocatorListTest, initialValueForList) {
EXPECT_EQ(extra + 0, *(theList.get_allocator().getBytesAllocated()));
EXPECT_EQ(extra + 0, theList.get_allocator().getBytesAllocated());
}

// Test adding single int to List
TEST_F(MemoryTrackingAllocatorListTest, addElementToList) {
theList.push_back(1);
EXPECT_EQ(extra + perElementOverhead,
*(theList.get_allocator().getBytesAllocated()));
theList.get_allocator().getBytesAllocated());
theList.clear();
EXPECT_EQ(extra + 0, *(theList.get_allocator().getBytesAllocated()));
EXPECT_EQ(extra + 0, theList.get_allocator().getBytesAllocated());
}

// Test adding 4096 ints to List.
Expand All @@ -72,9 +72,9 @@ TEST_F(MemoryTrackingAllocatorListTest, addManyElementsToList) {
theList.push_back(ii);
}
EXPECT_EQ(extra + (perElementOverhead * 4096),
*(theList.get_allocator().getBytesAllocated()));
theList.get_allocator().getBytesAllocated());
theList.clear();
EXPECT_EQ(extra + 0, *(theList.get_allocator().getBytesAllocated()));
EXPECT_EQ(extra + 0, theList.get_allocator().getBytesAllocated());
}

// Test bytesAllocates is correct when a re-bind occurs.
Expand All @@ -90,9 +90,9 @@ TEST(MemoryTrackingAllocatorTest, rebindTest) {
notCorrectlyAllocatedDeque.push_back(1);

auto correctlyAllocatedSize =
*(correctlyAllocatedDeque.get_allocator().getBytesAllocated());
correctlyAllocatedDeque.get_allocator().getBytesAllocated();
auto notCorrectlyAllocatedSize =
*(notCorrectlyAllocatedDeque.get_allocator().getBytesAllocated());
notCorrectlyAllocatedDeque.get_allocator().getBytesAllocated();
#ifdef _LIBCPP_VERSION
// When using libc++ the correctly allocated deque will be larger
// because it combines the size allocated for metadata and the size
Expand All @@ -112,20 +112,20 @@ TEST(MemoryTrackingAllocatorTest, copyTest) {
MemoryTrackingAllocator<int> allocator;
Deque theDeque(allocator);
theDeque.push_back(0);
auto theDequeSize = *(theDeque.get_allocator().getBytesAllocated());
auto theDequeSize = theDeque.get_allocator().getBytesAllocated();

// Copy the deque.
Deque copy = theDeque;
auto copySize = *(copy.get_allocator().getBytesAllocated());
auto copySize = copy.get_allocator().getBytesAllocated();
EXPECT_EQ(theDequeSize, copySize);

// Add a further 4095 items to the deque - which will cause a resize
for (int ii = 1; ii < 4096; ++ii) {
theDeque.push_back(ii);
}

auto newTheDequeSize = *(theDeque.get_allocator().getBytesAllocated());
auto newCopySize = *(copy.get_allocator().getBytesAllocated());
auto newTheDequeSize = theDeque.get_allocator().getBytesAllocated();
auto newCopySize = copy.get_allocator().getBytesAllocated();
// The original deque should have increased in size.
EXPECT_LT(theDequeSize, newTheDequeSize);
// The copied deque should not have changed in size.
Expand Down
21 changes: 15 additions & 6 deletions utilities/memory_tracking_allocator.h
View file
Expand Up @@ -37,7 +37,7 @@
*
* To return the bytes allocated use:
*
* *(theDeque.get_allocator().getBytesAllocated())
* theDeque.get_allocator().getBytesAllocated();
*
* See /engines/ep/tests/module_tests/memory_tracking_allocator_test.cc for
* full code.
Expand Down Expand Up @@ -72,7 +72,7 @@ class MemoryTrackingAllocator {
* Used during a rebind and therefore need to copy over the
* byteAllocated shared pointer.
*/
: bytesAllocated(other.getBytesAllocated()) {
: bytesAllocated(other.getUnderlyingCounter()) {
}

MemoryTrackingAllocator(const MemoryTrackingAllocator& other) noexcept =
Expand All @@ -85,7 +85,7 @@ class MemoryTrackingAllocator {
// in a std::list). As such, we need to ensure the moved-from
// container's allocator (i.e. other) can still perform deallocations,
// hence bytesAllocated should only be copied, not moved-from.
: bytesAllocated(other.getBytesAllocated()) {
: bytesAllocated(other.getUnderlyingCounter()) {
}

MemoryTrackingAllocator& operator=(
Expand All @@ -95,7 +95,7 @@ class MemoryTrackingAllocator {
MemoryTrackingAllocator&& other) noexcept {
// Need to copy bytesAllocated even though this is the move-assignment
// operator - see comment in move ctor for rationale.
bytesAllocated = other.getBytesAllocated();
bytesAllocated = other.getUnderlyingCounter();
return *this;
}

Expand All @@ -117,11 +117,20 @@ class MemoryTrackingAllocator {
return MemoryTrackingAllocator();
}

auto getBytesAllocated() const {
return bytesAllocated;
// @return the value of the bytesAllocated counter
size_t getBytesAllocated() const {
return *bytesAllocated;
}

private:
// @return a shared_ptr to the bytesAllocated counter
auto getUnderlyingCounter() const {
return bytesAllocated;
}

template <class U>
friend class MemoryTrackingAllocator;

std::shared_ptr<cb::NonNegativeCounter<size_t>> bytesAllocated;
};

Expand Down

0 comments on commit 3e32796

Please sign in to comment.