Skip to content

Commit

Permalink
Let shrinkCapacity not shrink free cap (facebookincubator#10780)
Browse files Browse the repository at this point in the history 
Summary:
Shrinking free capacity when calling global shrinkCapacity method does not help with system memory pressure. Remove the free capacity shrink at the beginning of the method to help with actual memory pressure release as this method is only called by push back mechanism when there is memory pressure.

Pull Request resolved: facebookincubator#10780

Reviewed By: xiaoxmeng

Differential Revision: D61496392

Pulled By: tanjialiang

fbshipit-source-id: a178f45c904ba3b21af3128d19beedf47360119a
  • Loading branch information
@tanjialiang @facebook-github-bot
tanjialiang authored and facebook-github-bot committed Aug 22, 2024
1 parent 024d10d commit 9387a9d
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 183 deletions.
26 changes: 17 additions & 9 deletions velox/common/memory/MemoryArbitrator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,15 +126,23 @@ class MemoryArbitrator {
/// pool. The function returns the actual freed capacity from 'pool'.
virtual uint64_t shrinkCapacity(MemoryPool* pool, uint64_t targetBytes) = 0;

/// Invoked by the memory manager to shrink memory capacity from memory pools
/// by reclaiming free and used memory. The freed memory capacity is given
/// back to the arbitrator. If 'targetBytes' is zero, then try to reclaim all
/// the memory from 'pools'. The function returns the actual freed memory
/// capacity in bytes. If 'allowSpill' is true, it reclaims the used memory by
/// spilling. If 'allowAbort' is true, it reclaims the used memory by aborting
/// the queries with the most memory usage. If both are true, it first
/// reclaims the used memory by spilling and then abort queries to reach the
/// reclaim target.
/// Invoked by the memory manager to globally shrink memory from
/// memory pools by reclaiming only used memory, to reduce system memory
/// pressure. The freed memory capacity is given back to the arbitrator. If
/// 'targetBytes' is zero, then try to reclaim all the memory from 'pools'.
/// The function returns the actual freed memory capacity in bytes. If
/// 'allowSpill' is true, it reclaims the used memory by spilling. If
/// 'allowAbort' is true, it reclaims the used memory by aborting the queries
/// with the most memory usage. If both are true, it first reclaims the used
/// memory by spilling and then abort queries to reach the reclaim target.
///
/// NOTE: The actual reclaimed used memory (hence system memory) may be less
/// than 'targetBytes' due to the accounting of free capacity reclaimed. This
/// is okay because when this method is called, system is normally under
/// memory pressure, and there normally isn't much free capacity to reclaim.
/// So the reclaimed used memory in this case should be very close to
/// 'targetBytes' if enough used memory is reclaimable. We should improve this
/// in the future.
virtual uint64_t shrinkCapacity(
uint64_t targetBytes,
bool allowSpill = true,
Expand Down
39 changes: 20 additions & 19 deletions velox/common/memory/SharedArbitrator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -496,37 +496,38 @@ uint64_t SharedArbitrator::shrinkCapacity(
ArbitrationOperation op(requestBytes);
ScopedArbitration scopedArbitration(this, &op);

uint64_t fastReclaimTargetBytes =
std::max(memoryPoolTransferCapacity_, requestBytes);

std::lock_guard<std::shared_mutex> exclusiveLock(arbitrationLock_);
getCandidates(&op);
uint64_t freedBytes =
reclaimFreeMemoryFromCandidates(&op, fastReclaimTargetBytes, false);
auto freeGuard = folly::makeGuard([&]() {
// Returns the freed memory capacity back to the arbitrator.
if (freedBytes > 0) {
incrementFreeCapacity(freedBytes);
}
});
if (freedBytes >= op.requestBytes) {
return freedBytes;
}
uint64_t unreturnedFreedBytes{0};
uint64_t totalFreedBytes{0};

RECORD_METRIC_VALUE(kMetricArbitratorSlowGlobalArbitrationCount);

if (allowSpill) {
reclaimUsedMemoryFromCandidatesBySpill(&op, freedBytes);
if (freedBytes >= op.requestBytes) {
return freedBytes;
reclaimUsedMemoryFromCandidatesBySpill(&op, unreturnedFreedBytes);
totalFreedBytes += unreturnedFreedBytes;
if (unreturnedFreedBytes > 0) {
incrementFreeCapacity(unreturnedFreedBytes);
unreturnedFreedBytes = 0;
}
if (totalFreedBytes >= op.requestBytes) {
return totalFreedBytes;
}
if (allowAbort) {
// Candidate stats may change after spilling.
getCandidates(&op);
}
}

if (allowAbort) {
reclaimUsedMemoryFromCandidatesByAbort(&op, freedBytes);
reclaimUsedMemoryFromCandidatesByAbort(&op, unreturnedFreedBytes);
totalFreedBytes += unreturnedFreedBytes;
if (unreturnedFreedBytes > 0) {
incrementFreeCapacity(unreturnedFreedBytes);
unreturnedFreedBytes = 0;
}
}
return freedBytes;
return totalFreedBytes;
}

void SharedArbitrator::testingFreeCapacity(uint64_t capacity) {
Expand Down
24 changes: 0 additions & 24 deletions velox/common/memory/tests/MemoryArbitratorTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -285,30 +285,6 @@ TEST_F(MemoryArbitrationTest, reservedCapacityFreeByPoolRelease) {
ASSERT_EQ(arbitrator->stats().freeCapacityBytes, 6 << 20);
}

TEST_F(MemoryArbitrationTest, reservedCapacityFreeByPoolShrink) {
MemoryManagerOptions options;
options.arbitratorKind = "SHARED";
options.arbitratorReservedCapacity = 4 << 20;
options.arbitratorCapacity = 8 << 20;
options.allocatorCapacity = options.arbitratorCapacity;
options.memoryPoolInitCapacity = 2 << 20;
options.memoryPoolReservedCapacity = 1 << 20;

MemoryManager manager(options);
auto* arbitrator = manager.arbitrator();
const int numPools = 6;
std::vector<std::shared_ptr<MemoryPool>> pools;
for (int i = 0; i < numPools; ++i) {
pools.push_back(manager.addRootPool("", kMaxMemory));
ASSERT_GE(pools.back()->capacity(), 1 << 20);
}
ASSERT_EQ(arbitrator->stats().freeCapacityBytes, 0);
pools.push_back(manager.addRootPool("", kMaxMemory));

ASSERT_GE(pools.back()->capacity(), 0);
ASSERT_EQ(arbitrator->shrinkCapacity(1 << 20), 2 << 20);
}

TEST_F(MemoryArbitrationTest, arbitratorStats) {
const MemoryArbitrator::Stats emptyStats;
ASSERT_TRUE(emptyStats.empty());
Expand Down
155 changes: 24 additions & 131 deletions velox/common/memory/tests/MockSharedArbitratorTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -795,10 +795,12 @@ TEST_F(MockSharedArbitrationTest, shrinkPools) {
for (const auto& testTask : testTasks) {
tasksOss << "[";
tasksOss << testTask.debugString();
tasksOss << "], ";
tasksOss << "], \n";
}
return fmt::format(
"testTasks: [{}], targetBytes: {}, expectedFreedBytes: {}, expectedFreeCapacity: {}, expectedReservedFreeCapacity: {}, allowSpill: {}, allowAbort: {}",
"testTasks: \n[{}], \ntargetBytes: {}, \nexpectedFreedBytes: {}, "
"\nexpectedFreeCapacity: {}, \nexpectedReservedFreeCapacity: {}, \n"
"allowSpill: {}, \nallowAbort: {}",
tasksOss.str(),
succinctBytes(targetBytes),
succinctBytes(expectedFreedBytes),
Expand All @@ -808,28 +810,6 @@ TEST_F(MockSharedArbitrationTest, shrinkPools) {
allowAbort);
}
} testSettings[] = {
{{{memoryPoolInitCapacity, true, 0, memoryPoolInitCapacity, false},
{memoryPoolInitCapacity, true, 0, memoryPoolInitCapacity, false},
{memoryPoolInitCapacity, false, 0, memoryPoolInitCapacity, false},
{memoryPoolInitCapacity, false, 0, memoryPoolReserveCapacity, false}},
0,
18 << 20,
24 << 20,
reservedMemoryCapacity,
true,
false},

{{{memoryPoolInitCapacity, true, 0, memoryPoolInitCapacity, false},
{memoryPoolInitCapacity, true, 0, memoryPoolInitCapacity, false},
{memoryPoolInitCapacity, false, 0, memoryPoolInitCapacity, false},
{memoryPoolInitCapacity, true, 0, memoryPoolReserveCapacity, false}},
0,
18 << 20,
24 << 20,
reservedMemoryCapacity,
true,
false},

{{{memoryPoolInitCapacity,
false,
memoryPoolInitCapacity,
Expand All @@ -843,9 +823,9 @@ TEST_F(MockSharedArbitrationTest, shrinkPools) {
memoryPoolReserveCapacity,
false}},
0,
12 << 20,
18 << 20,
reservedMemoryCapacity,
0,
6 << 20,
6 << 20,
true,
false},

Expand All @@ -862,8 +842,8 @@ TEST_F(MockSharedArbitrationTest, shrinkPools) {
memoryPoolReserveCapacity,
false}},
0,
20 << 20,
26 << 20,
8 << 20,
14 << 20,
reservedMemoryCapacity,
true,
false},
Expand All @@ -881,9 +861,9 @@ TEST_F(MockSharedArbitrationTest, shrinkPools) {
memoryPoolReserveCapacity,
false}},
0,
12 << 20,
18 << 20,
reservedMemoryCapacity,
0,
6 << 20,
6 << 20,
false,
false},

Expand All @@ -900,9 +880,9 @@ TEST_F(MockSharedArbitrationTest, shrinkPools) {
memoryPoolReserveCapacity,
false}},
0,
12 << 20,
18 << 20,
reservedMemoryCapacity,
0,
6 << 20,
6 << 20,
true,
false},

Expand Down Expand Up @@ -941,26 +921,15 @@ TEST_F(MockSharedArbitrationTest, shrinkPools) {
{memoryPoolInitCapacity, false, 0, memoryPoolInitCapacity, false},
{memoryPoolInitCapacity, true, 0, memoryPoolReserveCapacity, false}},
16 << 20,
16 << 20,
22 << 20,
reservedMemoryCapacity,
0,
6 << 20,
6 << 20,
false,
false},

{{{memoryPoolInitCapacity, true, 0, memoryPoolInitCapacity, false},
{memoryPoolInitCapacity, true, 0, memoryPoolInitCapacity, false},
{memoryPoolInitCapacity, false, 0, memoryPoolInitCapacity, false},
{memoryPoolInitCapacity, true, 0, memoryPoolReserveCapacity, false}},
16 << 20,
16 << 20,
22 << 20,
reservedMemoryCapacity,
true,
false},

{{{memoryPoolInitCapacity, true, 0, memoryPoolInitCapacity, false},
{memoryPoolInitCapacity, true, 0, memoryPoolInitCapacity, false},
{memoryPoolInitCapacity, false, 0, memoryPoolInitCapacity, false},
{memoryPoolInitCapacity, false, 1 << 10, memoryPoolInitCapacity, true},
{memoryPoolInitCapacity, false, 1 << 10, memoryPoolInitCapacity, true},
{memoryPoolInitCapacity, true, 0, memoryPoolReserveCapacity, false}},
16 << 20,
16 << 20,
Expand All @@ -970,34 +939,12 @@ TEST_F(MockSharedArbitrationTest, shrinkPools) {
true},

{{{memoryPoolInitCapacity, true, 0, memoryPoolInitCapacity, false},
{memoryPoolInitCapacity, true, 0, memoryPoolInitCapacity, false},
{memoryPoolInitCapacity, false, 0, memoryPoolInitCapacity, false},
{memoryPoolInitCapacity, true, 0, memoryPoolReserveCapacity, false}},
14 << 20,
14 << 20,
20 << 20,
reservedMemoryCapacity,
false,
false},

{{{memoryPoolInitCapacity, true, 0, memoryPoolInitCapacity, false},
{memoryPoolInitCapacity, true, 0, memoryPoolInitCapacity, false},
{memoryPoolInitCapacity, false, 0, memoryPoolInitCapacity, false},
{memoryPoolInitCapacity, true, 0, memoryPoolReserveCapacity, false}},
12 << 20,
12 << 20,
18 << 20,
reservedMemoryCapacity,
true,
false},

{{{memoryPoolInitCapacity, true, 0, memoryPoolInitCapacity, false},
{memoryPoolInitCapacity, true, 0, memoryPoolInitCapacity, false},
{memoryPoolInitCapacity, false, 0, memoryPoolInitCapacity, false},
{memoryPoolInitCapacity, false, 1 << 10, memoryPoolInitCapacity, true},
{memoryPoolInitCapacity, false, 1 << 10, memoryPoolInitCapacity, true},
{memoryPoolInitCapacity, true, 0, memoryPoolReserveCapacity, false}},
14 << 20,
14 << 20,
20 << 20,
16 << 20,
22 << 20,
reservedMemoryCapacity,
true,
true},
Expand Down Expand Up @@ -1029,33 +976,6 @@ TEST_F(MockSharedArbitrationTest, shrinkPools) {
true,
false},

{{{memoryPoolInitCapacity,
true,
memoryPoolInitCapacity,
memoryPoolInitCapacity,
false},
{memoryPoolInitCapacity,
true,
memoryPoolInitCapacity,
memoryPoolInitCapacity,
false},
{memoryPoolInitCapacity,
false,
memoryPoolInitCapacity,
memoryPoolInitCapacity,
false},
{memoryPoolInitCapacity,
true,
memoryPoolReserveCapacity,
memoryPoolReserveCapacity,
false}},
14 << 20,
0,
6 << 20,
6 << 20,
false,
false},

{{{memoryPoolInitCapacity,
true,
memoryPoolInitCapacity,
Expand Down Expand Up @@ -1083,33 +1003,6 @@ TEST_F(MockSharedArbitrationTest, shrinkPools) {
false,
true},

{{{memoryPoolInitCapacity,
false,
memoryPoolInitCapacity,
memoryPoolInitCapacity,
false},
{memoryPoolInitCapacity,
false,
memoryPoolInitCapacity,
memoryPoolInitCapacity,
false},
{memoryPoolInitCapacity,
false,
memoryPoolInitCapacity,
memoryPoolInitCapacity,
false},
{memoryPoolInitCapacity,
false,
memoryPoolReserveCapacity,
memoryPoolReserveCapacity,
false}},
14 << 20,
0,
6 << 20,
6 << 20,
false,
false},

{{{memoryPoolInitCapacity,
false,
memoryPoolInitCapacity,
Expand Down

0 comments on commit 9387a9d

Please sign in to comment.