Skip to content

Commit

Permalink
Add reserve and release counter in SharedArbitrator::Stats (facebooki…
Browse files Browse the repository at this point in the history 
…ncubator#7530)

Summary:
MemoryManager calls `SharedArbitrator::reserveMemory` each time
a root memory pool is created by its API `addRootPool`, and calls
`SharedArbitrator::releaseMemory` through a callback method in the
destructor of the `MemoryPoolImpl` when destructing a memory pool.

This PR adds two counters and for `SharedArbitrator::reserveMemory`
and `SharedArbitrator::releaseMemory` to count their call counts,
which should be the same, at the end of the `MemoryManager` life cycle.

Pull Request resolved: facebookincubator#7530

Reviewed By: tanjialiang

Differential Revision: D51242751

Pulled By: xiaoxmeng

fbshipit-source-id: 5de0274b42b0264bedf0b19a5e81fc7fb1c2a85a
  • Loading branch information
@duanmeng @facebook-github-bot
duanmeng authored and facebook-github-bot committed Nov 13, 2023
1 parent 45fd3b4 commit e39c667
Show file tree
Hide file tree
Showing 8 changed files with 306 additions and 203 deletions.
2 changes: 1 addition & 1 deletion velox/common/memory/Memory.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ class MemoryManager {
const uint16_t alignment_;
const bool checkUsageLeak_;
const bool debugEnabled_;
// The destruction callback set for the allocated root memory pools which are
// The destruction callback set for the allocated root memory pools which are
// tracked by 'pools_'. It is invoked on the root pool destruction and removes
// the pool from 'pools_'.
const MemoryPoolImpl::DestructionCallback poolDestructionCb_;
Expand Down
27 changes: 22 additions & 5 deletions velox/common/memory/MemoryArbitrator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,9 @@ MemoryArbitrator::Stats::Stats(
uint64_t _maxCapacityBytes,
uint64_t _freeCapacityBytes,
uint64_t _reclaimTimeUs,
uint64_t _numNonReclaimableAttempts)
uint64_t _numNonReclaimableAttempts,
uint64_t _numReserveRequest,
uint64_t _numReleaseRequest)
: numRequests(_numRequests),
numSucceeded(_numSucceeded),
numAborted(_numAborted),
Expand All @@ -276,16 +278,23 @@ MemoryArbitrator::Stats::Stats(
maxCapacityBytes(_maxCapacityBytes),
freeCapacityBytes(_freeCapacityBytes),
reclaimTimeUs(_reclaimTimeUs),
numNonReclaimableAttempts(_numNonReclaimableAttempts) {}
numNonReclaimableAttempts(_numNonReclaimableAttempts),
numReserveRequest(_numReserveRequest),
numReleaseRequest(_numReleaseRequest) {}

std::string MemoryArbitrator::Stats::toString() const {
return fmt::format(
"STATS[numRequests {} numSucceeded {} numAborted {} numFailures {} numNonReclaimableAttempts {} queueTime {} arbitrationTime {} reclaimTime {} shrunkMemory {} reclaimedMemory {} maxCapacity {} freeCapacity {}]",
"STATS[numRequests {} numSucceeded {} numAborted {} numFailures {} "
"numNonReclaimableAttempts {} numReserveRequest {} numReleaseRequest {} "
"queueTime {} arbitrationTime {} reclaimTime {} shrunkMemory {} "
"reclaimedMemory {} maxCapacity {} freeCapacity {}]",
numRequests,
numSucceeded,
numAborted,
numFailures,
numNonReclaimableAttempts,
numReserveRequest,
numReleaseRequest,
succinctMicros(queueTimeUs),
succinctMicros(arbitrationTimeUs),
succinctMicros(reclaimTimeUs),
Expand All @@ -311,6 +320,8 @@ MemoryArbitrator::Stats MemoryArbitrator::Stats::operator-(
result.reclaimTimeUs = reclaimTimeUs - other.reclaimTimeUs;
result.numNonReclaimableAttempts =
numNonReclaimableAttempts - other.numNonReclaimableAttempts;
result.numReserveRequest = numReserveRequest - other.numReserveRequest;
result.numReleaseRequest = numReleaseRequest - other.numReleaseRequest;
return result;
}

Expand All @@ -327,7 +338,9 @@ bool MemoryArbitrator::Stats::operator==(const Stats& other) const {
maxCapacityBytes,
freeCapacityBytes,
reclaimTimeUs,
numNonReclaimableAttempts) ==
numNonReclaimableAttempts,
numReserveRequest,
numReleaseRequest) ==
std::tie(
other.numRequests,
other.numSucceeded,
Expand All @@ -340,7 +353,9 @@ bool MemoryArbitrator::Stats::operator==(const Stats& other) const {
other.maxCapacityBytes,
other.freeCapacityBytes,
other.reclaimTimeUs,
other.numNonReclaimableAttempts);
other.numNonReclaimableAttempts,
other.numReserveRequest,
other.numReleaseRequest);
}

bool MemoryArbitrator::Stats::operator!=(const Stats& other) const {
Expand Down Expand Up @@ -372,6 +387,8 @@ bool MemoryArbitrator::Stats::operator<(const Stats& other) const {
UPDATE_COUNTER(numReclaimedBytes);
UPDATE_COUNTER(reclaimTimeUs);
UPDATE_COUNTER(numNonReclaimableAttempts);
UPDATE_COUNTER(numReserveRequest);
UPDATE_COUNTER(numReleaseRequest);
#undef UPDATE_COUNTER
VELOX_CHECK(
!((gtCount > 0) && (ltCount > 0)),
Expand Down
8 changes: 7 additions & 1 deletion velox/common/memory/MemoryArbitrator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,10 @@ class MemoryArbitrator {
/// The total number of times of the reclaim attempts that end up failing
/// due to reclaiming at non-reclaimable stage.
uint64_t numNonReclaimableAttempts{0};
/// The total number of invoking reserveMemory method.
uint64_t numReserveRequest{0};
/// The total number of invoking releaseMemory method.
uint64_t numReleaseRequest{0};

Stats(
uint64_t _numRequests,
Expand All @@ -186,7 +190,9 @@ class MemoryArbitrator {
uint64_t _maxCapacityBytes,
uint64_t _freeCapacityBytes,
uint64_t _reclaimTimeUs,
uint64_t _numNonReclaimableAttempts);
uint64_t _numNonReclaimableAttempts,
uint64_t _numReserveRequest,
uint64_t _numReleaseRequest);

Stats() = default;

Expand Down
20 changes: 14 additions & 6 deletions velox/common/memory/tests/MemoryArbitratorTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,11 @@ TEST_F(MemoryArbitrationTest, stats) {
stats.numNonReclaimableAttempts = 5;
ASSERT_EQ(
stats.toString(),
"STATS[numRequests 2 numSucceeded 0 numAborted 3 numFailures 100 numNonReclaimableAttempts 5 queueTime 230.00ms arbitrationTime 1.02ms reclaimTime 1.00ms shrunkMemory 95.37MB reclaimedMemory 9.77KB maxCapacity 0B freeCapacity 0B]");
"STATS[numRequests 2 numSucceeded 0 numAborted 3 numFailures 100 "
"numNonReclaimableAttempts 5 numReserveRequest 0 numReleaseRequest 0 "
"queueTime 230.00ms arbitrationTime 1.02ms reclaimTime 1.00ms "
"shrunkMemory 95.37MB reclaimedMemory 9.77KB "
"maxCapacity 0B freeCapacity 0B]");
}

TEST_F(MemoryArbitrationTest, create) {
Expand Down Expand Up @@ -121,9 +125,11 @@ TEST_F(MemoryArbitrationTest, queryMemoryCapacity) {
TEST_F(MemoryArbitrationTest, arbitratorStats) {
const MemoryArbitrator::Stats emptyStats;
ASSERT_TRUE(emptyStats.empty());
const MemoryArbitrator::Stats anchorStats(5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5);
const MemoryArbitrator::Stats anchorStats(
5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5);
ASSERT_FALSE(anchorStats.empty());
const MemoryArbitrator::Stats largeStats(8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8);
const MemoryArbitrator::Stats largeStats(
8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8);
ASSERT_FALSE(largeStats.empty());
ASSERT_TRUE(!(anchorStats == largeStats));
ASSERT_TRUE(anchorStats != largeStats);
Expand All @@ -132,9 +138,11 @@ TEST_F(MemoryArbitrationTest, arbitratorStats) {
ASSERT_TRUE(anchorStats <= largeStats);
ASSERT_TRUE(!(anchorStats >= largeStats));
const auto delta = largeStats - anchorStats;
ASSERT_EQ(delta, MemoryArbitrator::Stats(3, 3, 3, 3, 3, 3, 3, 3, 8, 8, 3, 3));
ASSERT_EQ(
delta, MemoryArbitrator::Stats(3, 3, 3, 3, 3, 3, 3, 3, 8, 8, 3, 3, 3, 3));

const MemoryArbitrator::Stats smallStats(2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2);
const MemoryArbitrator::Stats smallStats(
2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2);
ASSERT_TRUE(!(anchorStats == smallStats));
ASSERT_TRUE(anchorStats != smallStats);
ASSERT_TRUE(!(anchorStats < smallStats));
Expand All @@ -143,7 +151,7 @@ TEST_F(MemoryArbitrationTest, arbitratorStats) {
ASSERT_TRUE(anchorStats >= smallStats);

const MemoryArbitrator::Stats invalidStats(
2, 2, 2, 2, 2, 2, 8, 8, 8, 8, 8, 2);
2, 2, 2, 2, 2, 2, 8, 8, 8, 8, 8, 2, 8, 2);
ASSERT_TRUE(!(anchorStats == invalidStats));
ASSERT_TRUE(anchorStats != invalidStats);
ASSERT_THROW(anchorStats < invalidStats, VeloxException);
Expand Down
10 changes: 9 additions & 1 deletion velox/common/memory/tests/MemoryManagerTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,15 @@ TEST_F(MemoryManagerTest, Ctor) {
ASSERT_EQ(arbitrator->stats().maxCapacityBytes, kCapacity);
ASSERT_EQ(
manager.toString(),
"Memory Manager[capacity 4.00GB alignment 64B usedBytes 0B number of pools 0\nList of root pools:\n\t__default_root__\nMemory Allocator[MALLOC capacity 4.00GB allocated bytes 0 allocated pages 0 mapped pages 0]\nARBITRATOR[SHARED CAPACITY[4.00GB] STATS[numRequests 0 numSucceeded 0 numAborted 0 numFailures 0 numNonReclaimableAttempts 0 queueTime 0us arbitrationTime 0us reclaimTime 0us shrunkMemory 0B reclaimedMemory 0B maxCapacity 4.00GB freeCapacity 4.00GB]]]");
"Memory Manager[capacity 4.00GB alignment 64B usedBytes 0B number of "
"pools 0\nList of root pools:\n\t__default_root__\n"
"Memory Allocator[MALLOC capacity 4.00GB allocated bytes 0 "
"allocated pages 0 mapped pages 0]\n"
"ARBITRATOR[SHARED CAPACITY[4.00GB] STATS[numRequests 0 numSucceeded 0 "
"numAborted 0 numFailures 0 numNonReclaimableAttempts 0 "
"numReserveRequest 0 numReleaseRequest 0 queueTime 0us "
"arbitrationTime 0us reclaimTime 0us shrunkMemory 0B "
"reclaimedMemory 0B maxCapacity 4.00GB freeCapacity 4.00GB]]]");
}
{
// Test construction failure due to inconsistent allocator capacity setting.
Expand Down
4 changes: 4 additions & 0 deletions velox/exec/SharedArbitrator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ void SharedArbitrator::reserveMemory(MemoryPool* pool, uint64_t /*unused*/) {
const int64_t bytesToReserve =
std::min<int64_t>(maxGrowBytes(*pool), memoryPoolInitCapacity_);
std::lock_guard<std::mutex> l(mutex_);
++numReserveRequest_;
if (running_) {
// NOTE: if there is a running memory arbitration, then we shall skip
// reserving the free memory for the newly created memory pool but let it
Expand All @@ -164,6 +165,7 @@ void SharedArbitrator::reserveMemory(MemoryPool* pool, uint64_t /*unused*/) {

void SharedArbitrator::releaseMemory(MemoryPool* pool) {
std::lock_guard<std::mutex> l(mutex_);
++numReleaseRequest_;
const uint64_t freedBytes = pool->shrink(0);
incrementFreeCapacityLocked(freedBytes);
}
Expand Down Expand Up @@ -499,6 +501,8 @@ MemoryArbitrator::Stats SharedArbitrator::statsLocked() const {
stats.freeCapacityBytes = freeCapacity_;
stats.reclaimTimeUs = reclaimTimeUs_;
stats.numNonReclaimableAttempts = numNonReclaimableAttempts_;
stats.numReserveRequest = numReserveRequest_;
stats.numReleaseRequest = numReleaseRequest_;
return stats;
}

Expand Down
2 changes: 2 additions & 0 deletions velox/exec/SharedArbitrator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,5 +203,7 @@ class SharedArbitrator : public memory::MemoryArbitrator {
tsan_atomic<uint64_t> numReclaimedBytes_{0};
tsan_atomic<uint64_t> reclaimTimeUs_{0};
tsan_atomic<uint64_t> numNonReclaimableAttempts_{0};
tsan_atomic<uint64_t> numReserveRequest_{0};
tsan_atomic<uint64_t> numReleaseRequest_{0};
};
} // namespace facebook::velox::exec
Loading

0 comments on commit e39c667

Please sign in to comment.