diff --git a/velox/common/memory/MemoryArbitrator.h b/velox/common/memory/MemoryArbitrator.h index d4b266208f4a..20b50e601288 100644 --- a/velox/common/memory/MemoryArbitrator.h +++ b/velox/common/memory/MemoryArbitrator.h @@ -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, diff --git a/velox/common/memory/SharedArbitrator.cpp b/velox/common/memory/SharedArbitrator.cpp index fc785576c9e2..cd51312f1bcf 100644 --- a/velox/common/memory/SharedArbitrator.cpp +++ b/velox/common/memory/SharedArbitrator.cpp @@ -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 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) { diff --git a/velox/common/memory/tests/MemoryArbitratorTest.cpp b/velox/common/memory/tests/MemoryArbitratorTest.cpp index 30ff9acda286..848ab33f7960 100644 --- a/velox/common/memory/tests/MemoryArbitratorTest.cpp +++ b/velox/common/memory/tests/MemoryArbitratorTest.cpp @@ -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> 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()); diff --git a/velox/common/memory/tests/MockSharedArbitratorTest.cpp b/velox/common/memory/tests/MockSharedArbitratorTest.cpp index 9f0356f8ca10..3b701391fab9 100644 --- a/velox/common/memory/tests/MockSharedArbitratorTest.cpp +++ b/velox/common/memory/tests/MockSharedArbitratorTest.cpp @@ -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), @@ -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, @@ -843,9 +823,9 @@ TEST_F(MockSharedArbitrationTest, shrinkPools) { memoryPoolReserveCapacity, false}}, 0, - 12 << 20, - 18 << 20, - reservedMemoryCapacity, + 0, + 6 << 20, + 6 << 20, true, false}, @@ -862,8 +842,8 @@ TEST_F(MockSharedArbitrationTest, shrinkPools) { memoryPoolReserveCapacity, false}}, 0, - 20 << 20, - 26 << 20, + 8 << 20, + 14 << 20, reservedMemoryCapacity, true, false}, @@ -881,9 +861,9 @@ TEST_F(MockSharedArbitrationTest, shrinkPools) { memoryPoolReserveCapacity, false}}, 0, - 12 << 20, - 18 << 20, - reservedMemoryCapacity, + 0, + 6 << 20, + 6 << 20, false, false}, @@ -900,9 +880,9 @@ TEST_F(MockSharedArbitrationTest, shrinkPools) { memoryPoolReserveCapacity, false}}, 0, - 12 << 20, - 18 << 20, - reservedMemoryCapacity, + 0, + 6 << 20, + 6 << 20, true, false}, @@ -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, @@ -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}, @@ -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, @@ -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,