From 4ad52afba8689b4c3b2b8f03bb1cbe1d30c7f033 Mon Sep 17 00:00:00 2001 From: Deepak Majeti Date: Wed, 4 Dec 2024 18:47:43 -0500 Subject: [PATCH 1/6] use global static struct --- velox/common/config/Config.cpp | 6 ++++ velox/common/config/GlobalConfig.h | 47 +++++++++++++++++++++++++ velox/common/memory/CMakeLists.txt | 2 -- velox/common/memory/MallocAllocator.cpp | 2 +- velox/common/memory/Memory.cpp | 7 ++-- velox/common/memory/Memory.h | 12 +++---- velox/common/memory/MemoryAllocator.cpp | 2 +- velox/common/memory/MemoryAllocator.h | 8 ++--- velox/common/memory/MemoryPool.cpp | 2 +- velox/common/memory/MemoryPool.h | 3 +- velox/common/memory/MmapAllocator.cpp | 2 +- 11 files changed, 69 insertions(+), 24 deletions(-) create mode 100644 velox/common/config/GlobalConfig.h diff --git a/velox/common/config/Config.cpp b/velox/common/config/Config.cpp index 18cc705949cd..20bb99fe7f34 100644 --- a/velox/common/config/Config.cpp +++ b/velox/common/config/Config.cpp @@ -17,9 +17,15 @@ #include #include "velox/common/config/Config.h" +#include "velox/common/config/GlobalConfig.h" namespace facebook::velox::config { +GlobalConfig& globalConfig() { + static GlobalConfig config; + return config; +} + double toBytesPerCapacityUnit(CapacityUnit unit) { switch (unit) { case CapacityUnit::BYTE: diff --git a/velox/common/config/GlobalConfig.h b/velox/common/config/GlobalConfig.h new file mode 100644 index 000000000000..97f22f24e6f8 --- /dev/null +++ b/velox/common/config/GlobalConfig.h @@ -0,0 +1,47 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include + +namespace facebook::velox::config { + +struct GlobalConfig { + /// Number of shared leaf memory pools per process. + int32_t memoryNumSharedLeafPoolsConfig{32}; + /// If true, check fails on any memory leaks in memory pool and memory + /// manager. + bool memoryLeakCheckEnabled{false}; + /// If true, 'MemoryPool' will be running in debug mode to track the + /// allocation and free call sites to detect the source of memory leak for + /// testing purpose. + bool memoryPoolDebugEnabled{false}; + /// If true, enable memory usage tracking in the default memory pool. + bool enableMemoryUsageTrackInDefaultMemoryPool{false}; + /// Record time and volume for large allocation/free. + bool timeAllocations{false}; + /// Use explicit huge pages + bool memoryUseHugepages{false}; + /// If true, suppress the verbose error message in memory capacity exceeded + /// exception. This is only used by test to control the test error output + /// size. + bool suppressMemoryCapacityExceedingErrorMessage{false}; +}; + +GlobalConfig& globalConfig(); + +} // namespace facebook::velox::config diff --git a/velox/common/memory/CMakeLists.txt b/velox/common/memory/CMakeLists.txt index 79a707fac304..ecd605ace64f 100644 --- a/velox/common/memory/CMakeLists.txt +++ b/velox/common/memory/CMakeLists.txt @@ -38,11 +38,9 @@ velox_link_libraries( PUBLIC velox_common_base velox_common_config velox_exception - velox_flag_definitions velox_time velox_type Folly::folly fmt::fmt - gflags::gflags glog::glog PRIVATE velox_test_util re2::re2) diff --git a/velox/common/memory/MallocAllocator.cpp b/velox/common/memory/MallocAllocator.cpp index ff44791763ac..da6adb557566 100644 --- a/velox/common/memory/MallocAllocator.cpp +++ b/velox/common/memory/MallocAllocator.cpp @@ -37,7 +37,7 @@ MallocAllocator::MallocAllocator(size_t capacity, uint32_t reservationByteLimit) MallocAllocator::~MallocAllocator() { // TODO: Remove the check when memory leak issue is resolved. - if (FLAGS_velox_memory_leak_check_enabled) { + if (config::GlobalConfig().memoryLeakCheckEnabled) { VELOX_CHECK( ((allocatedBytes_ - reservations_.read()) == 0) && (numAllocated_ == 0) && (numMapped_ == 0), diff --git a/velox/common/memory/Memory.cpp b/velox/common/memory/Memory.cpp index 83e670b948d9..03e46b8b0c63 100644 --- a/velox/common/memory/Memory.cpp +++ b/velox/common/memory/Memory.cpp @@ -20,11 +20,10 @@ #include "velox/common/base/Counters.h" #include "velox/common/base/StatsReporter.h" +#include "velox/common/config/GlobalConfig.h" #include "velox/common/memory/MallocAllocator.h" #include "velox/common/memory/MmapAllocator.h" -DECLARE_int32(velox_memory_num_shared_leaf_pools); - namespace facebook::velox::memory { namespace { constexpr std::string_view kSysRootName{"__sys_root__"}; @@ -79,7 +78,7 @@ std::vector> createSharedLeafMemoryPools( VELOX_CHECK_EQ(sysPool.name(), kSysRootName); std::vector> leafPools; const size_t numSharedPools = - std::max(1, FLAGS_velox_memory_num_shared_leaf_pools); + std::max(1, config::GlobalConfig().memoryNumSharedLeafPoolsConfig); leafPools.reserve(numSharedPools); for (size_t i = 0; i < numSharedPools; ++i) { leafPools.emplace_back( @@ -129,7 +128,7 @@ MemoryManager::MemoryManager(const MemoryManagerOptions& options) sysRoot_->name()); VELOX_CHECK_EQ( sharedLeafPools_.size(), - std::max(1, FLAGS_velox_memory_num_shared_leaf_pools)); + std::max(1, config::GlobalConfig().memoryNumSharedLeafPoolsConfig)); } MemoryManager::~MemoryManager() { diff --git a/velox/common/memory/Memory.h b/velox/common/memory/Memory.h index f460d25ffee6..ece4a87bbdf3 100644 --- a/velox/common/memory/Memory.h +++ b/velox/common/memory/Memory.h @@ -25,7 +25,6 @@ #include #include -#include #include #include @@ -36,14 +35,11 @@ #include "folly/SharedMutex.h" #include "velox/common/base/CheckedArithmetic.h" #include "velox/common/base/SuccinctPrinter.h" +#include "velox/common/config/GlobalConfig.h" #include "velox/common/memory/Allocation.h" #include "velox/common/memory/MemoryAllocator.h" #include "velox/common/memory/MemoryPool.h" -DECLARE_bool(velox_memory_leak_check_enabled); -DECLARE_bool(velox_memory_pool_debug_enabled); -DECLARE_bool(velox_enable_memory_usage_track_in_default_memory_pool); - namespace facebook::velox::memory { #define VELOX_MEM_LOG_PREFIX "[MEM] " #define VELOX_MEM_LOG(severity) LOG(severity) << VELOX_MEM_LOG_PREFIX @@ -65,18 +61,18 @@ struct MemoryManagerOptions { /// If true, enable memory usage tracking in the default memory pool. bool trackDefaultUsage{ - FLAGS_velox_enable_memory_usage_track_in_default_memory_pool}; + config::GlobalConfig().enableMemoryUsageTrackInDefaultMemoryPool}; /// If true, check the memory pool and usage leaks on destruction. /// /// TODO: deprecate this flag after all the existing memory leak use cases /// have been fixed. - bool checkUsageLeak{FLAGS_velox_memory_leak_check_enabled}; + bool checkUsageLeak{config::GlobalConfig().memoryLeakCheckEnabled}; /// If true, the memory pool will be running in debug mode to track the /// allocation and free call stacks to detect the source of memory leak for /// testing purpose. - bool debugEnabled{FLAGS_velox_memory_pool_debug_enabled}; + bool debugEnabled{config::GlobalConfig().memoryPoolDebugEnabled}; /// Terminates the process and generates a core file on an allocation failure bool coreOnAllocationFailureEnabled{false}; diff --git a/velox/common/memory/MemoryAllocator.cpp b/velox/common/memory/MemoryAllocator.cpp index 574009d8b1f1..e2cc55b7a91e 100644 --- a/velox/common/memory/MemoryAllocator.cpp +++ b/velox/common/memory/MemoryAllocator.cpp @@ -415,7 +415,7 @@ void MemoryAllocator::useHugePages( const ContiguousAllocation& data, bool enable) { #ifdef linux - if (!FLAGS_velox_memory_use_hugepages) { + if (config::GlobalConfig().memoryUseHugepages) { return; } auto maybeRange = data.hugePageRange(); diff --git a/velox/common/memory/MemoryAllocator.h b/velox/common/memory/MemoryAllocator.h index 4a7ac706c715..92f61b9dcfef 100644 --- a/velox/common/memory/MemoryAllocator.h +++ b/velox/common/memory/MemoryAllocator.h @@ -24,14 +24,12 @@ #include #include -#include #include "velox/common/base/CheckedArithmetic.h" #include "velox/common/base/Exceptions.h" +#include "velox/common/config/GlobalConfig.h" #include "velox/common/memory/Allocation.h" #include "velox/common/time/Timer.h" -DECLARE_bool(velox_time_allocations); - namespace facebook::velox::memory { struct SizeClassStats { @@ -96,7 +94,7 @@ struct Stats { template void recordAllocate(int64_t bytes, int32_t count, Op op) { - if (FLAGS_velox_time_allocations) { + if (config::GlobalConfig().timeAllocations) { auto index = sizeIndex(bytes); velox::ClockTimer timer(sizes[index].allocateClocks); op(); @@ -109,7 +107,7 @@ struct Stats { template void recordFree(int64_t bytes, Op op) { - if (FLAGS_velox_time_allocations) { + if (config::globalConfig().timeAllocations) { auto index = sizeIndex(bytes); ClockTimer timer(sizes[index].freeClocks); op(); diff --git a/velox/common/memory/MemoryPool.cpp b/velox/common/memory/MemoryPool.cpp index dbd67dd5a579..acc9adbf6bf3 100644 --- a/velox/common/memory/MemoryPool.cpp +++ b/velox/common/memory/MemoryPool.cpp @@ -971,7 +971,7 @@ std::string MemoryPoolImpl::treeMemoryUsage(bool skipEmptyPool) const { if (parent_ != nullptr) { return parent_->treeMemoryUsage(skipEmptyPool); } - if (FLAGS_velox_suppress_memory_capacity_exceeding_error_message) { + if (config::globalConfig().suppressMemoryCapacityExceedingErrorMessage) { return ""; } std::stringstream out; diff --git a/velox/common/memory/MemoryPool.h b/velox/common/memory/MemoryPool.h index 3a84037d9e39..6b8bb7b17544 100644 --- a/velox/common/memory/MemoryPool.h +++ b/velox/common/memory/MemoryPool.h @@ -24,6 +24,7 @@ #include "velox/common/base/BitUtil.h" #include "velox/common/base/Exceptions.h" #include "velox/common/base/Portability.h" +#include "velox/common/config/GlobalConfig.h" #include "velox/common/memory/Allocation.h" #include "velox/common/memory/MemoryAllocator.h" #include "velox/common/memory/MemoryArbitrator.h" @@ -154,7 +155,7 @@ class MemoryPool : public std::enable_shared_from_this { /// If true, tracks the allocation and free call stacks to detect the source /// of memory leak for testing purpose. - bool debugEnabled{FLAGS_velox_memory_pool_debug_enabled}; + bool debugEnabled{config::globalConfig().memoryPoolDebugEnabled}; /// Terminates the process and generates a core file on an allocation /// failure diff --git a/velox/common/memory/MmapAllocator.cpp b/velox/common/memory/MmapAllocator.cpp index 8ab440e21252..6060b4cd7b74 100644 --- a/velox/common/memory/MmapAllocator.cpp +++ b/velox/common/memory/MmapAllocator.cpp @@ -191,7 +191,7 @@ MachinePageCount MmapAllocator::freeNonContiguousInternal( ClockTimer timer(clocks); pages = sizeClass->free(allocation); } - if ((pages > 0) && FLAGS_velox_time_allocations) { + if ((pages > 0) && config::globalConfig().timeAllocations) { // Increment the free time only if the allocation contained // pages in the class. Note that size class indices in the // allocator are not necessarily the same as in the stats. From 5e7bcb6d55883639c698deffd6bf54651bd9cc6e Mon Sep 17 00:00:00 2001 From: Deepak Majeti Date: Thu, 5 Dec 2024 08:42:53 -0500 Subject: [PATCH 2/6] fix build failure --- velox/common/config/GlobalConfig.h | 3 +++ velox/common/memory/MallocAllocator.h | 2 -- velox/common/memory/MemoryAllocator.cpp | 2 -- velox/common/memory/MemoryPool.cpp | 7 ------- velox/common/memory/MemoryPool.h | 4 ---- velox/common/memory/tests/CMakeLists.txt | 1 + velox/common/memory/tests/MemoryAllocatorTest.cpp | 9 ++++----- velox/exec/Operator.cpp | 5 +++-- velox/exec/fuzzer/FuzzerUtil.cpp | 5 +++-- 9 files changed, 14 insertions(+), 24 deletions(-) diff --git a/velox/common/config/GlobalConfig.h b/velox/common/config/GlobalConfig.h index 97f22f24e6f8..a5eeea43e070 100644 --- a/velox/common/config/GlobalConfig.h +++ b/velox/common/config/GlobalConfig.h @@ -40,6 +40,9 @@ struct GlobalConfig { /// exception. This is only used by test to control the test error output /// size. bool suppressMemoryCapacityExceedingErrorMessage{false}; + /// Whether allow to memory capacity transfer between memory pools from + /// different tasks, which might happen in use case like Spark-Gluten + bool memoryPoolCapacityTransferAcrossTasks{false}; }; GlobalConfig& globalConfig(); diff --git a/velox/common/memory/MallocAllocator.h b/velox/common/memory/MallocAllocator.h index ade9536f0fb6..5600c0a25784 100644 --- a/velox/common/memory/MallocAllocator.h +++ b/velox/common/memory/MallocAllocator.h @@ -20,8 +20,6 @@ #include "velox/common/memory/Memory.h" #include "velox/common/memory/MemoryAllocator.h" -DECLARE_bool(velox_memory_leak_check_enabled); - namespace facebook::velox::memory { /// The implementation of MemoryAllocator using malloc. class MallocAllocator : public MemoryAllocator { diff --git a/velox/common/memory/MemoryAllocator.cpp b/velox/common/memory/MemoryAllocator.cpp index e2cc55b7a91e..f2ff0dab247d 100644 --- a/velox/common/memory/MemoryAllocator.cpp +++ b/velox/common/memory/MemoryAllocator.cpp @@ -25,8 +25,6 @@ #include "velox/common/base/BitUtil.h" #include "velox/common/memory/Memory.h" -DECLARE_bool(velox_memory_use_hugepages); - namespace facebook::velox::memory { // static diff --git a/velox/common/memory/MemoryPool.cpp b/velox/common/memory/MemoryPool.cpp index acc9adbf6bf3..10c74de8c017 100644 --- a/velox/common/memory/MemoryPool.cpp +++ b/velox/common/memory/MemoryPool.cpp @@ -27,13 +27,6 @@ #include -DEFINE_bool( - velox_memory_pool_capacity_transfer_across_tasks, - false, - "Whether allow to memory capacity transfer between memory pools from different tasks, which might happen in use case like Spark-Gluten"); - -DECLARE_bool(velox_suppress_memory_capacity_exceeding_error_message); - using facebook::velox::common::testutil::TestValue; namespace facebook::velox::memory { diff --git a/velox/common/memory/MemoryPool.h b/velox/common/memory/MemoryPool.h index 6b8bb7b17544..f95e44f30a88 100644 --- a/velox/common/memory/MemoryPool.h +++ b/velox/common/memory/MemoryPool.h @@ -29,10 +29,6 @@ #include "velox/common/memory/MemoryAllocator.h" #include "velox/common/memory/MemoryArbitrator.h" -DECLARE_bool(velox_memory_leak_check_enabled); -DECLARE_bool(velox_memory_pool_debug_enabled); -DECLARE_bool(velox_memory_pool_capacity_transfer_across_tasks); - namespace facebook::velox::exec { class ParallelMemoryReclaimer; } diff --git a/velox/common/memory/tests/CMakeLists.txt b/velox/common/memory/tests/CMakeLists.txt index 3a35c9060b3b..6d2ca66b9566 100644 --- a/velox/common/memory/tests/CMakeLists.txt +++ b/velox/common/memory/tests/CMakeLists.txt @@ -38,6 +38,7 @@ target_link_libraries( velox_exception velox_exec velox_exec_test_lib + velox_flag_definitions velox_memory velox_temp_path velox_test_util diff --git a/velox/common/memory/tests/MemoryAllocatorTest.cpp b/velox/common/memory/tests/MemoryAllocatorTest.cpp index e953318c7582..990ce4a510dc 100644 --- a/velox/common/memory/tests/MemoryAllocatorTest.cpp +++ b/velox/common/memory/tests/MemoryAllocatorTest.cpp @@ -16,6 +16,7 @@ #include "velox/common/memory/MemoryAllocator.h" #include #include "velox/common/base/tests/GTestUtils.h" +#include "velox/common/config/GlobalConfig.h" #include "velox/common/memory/AllocationPool.h" #include "velox/common/memory/MallocAllocator.h" #include "velox/common/memory/MmapAllocator.h" @@ -34,8 +35,6 @@ #include #endif // linux -DECLARE_bool(velox_memory_leak_check_enabled); - using namespace facebook::velox::common::testutil; namespace facebook::velox::memory { @@ -55,7 +54,7 @@ class MemoryAllocatorTest : public testing::TestWithParam { protected: static void SetUpTestCase() { TestValue::enable(); - FLAGS_velox_memory_leak_check_enabled = true; + config::globalConfig().memoryLeakCheckEnabled = true; } void SetUp() override { @@ -651,7 +650,7 @@ TEST_P(MemoryAllocatorTest, stats) { } gflags::FlagSaver flagSaver; - FLAGS_velox_time_allocations = true; + config::globalConfig().timeAllocations = true; for (auto i = 0; i < sizes.size(); ++i) { std::unique_ptr allocation = std::make_unique(); auto size = sizes[i]; @@ -670,7 +669,7 @@ TEST_P(MemoryAllocatorTest, singleAllocation) { return; } gflags::FlagSaver flagSaver; - FLAGS_velox_time_allocations = true; + config::globalConfig().timeAllocations = true; const std::vector& sizes = instance_->sizeClasses(); MachinePageCount capacity = kCapacityPages; for (auto i = 0; i < sizes.size(); ++i) { diff --git a/velox/exec/Operator.cpp b/velox/exec/Operator.cpp index 5ee37bd29814..35c13c5e34ee 100644 --- a/velox/exec/Operator.cpp +++ b/velox/exec/Operator.cpp @@ -17,6 +17,7 @@ #include "velox/common/base/Counters.h" #include "velox/common/base/StatsReporter.h" #include "velox/common/base/SuccinctPrinter.h" +#include "velox/common/config/GlobalConfig.h" #include "velox/common/testutil/TestValue.h" #include "velox/exec/Driver.h" #include "velox/exec/OperatorUtils.h" @@ -649,7 +650,7 @@ void Operator::MemoryReclaimer::enterArbitration() { } Driver* const runningDriver = driverThreadCtx->driverCtx()->driver; - if (!FLAGS_velox_memory_pool_capacity_transfer_across_tasks) { + if (!config::globalConfig().memoryPoolCapacityTransferAcrossTasks) { if (auto opDriver = ensureDriver()) { // NOTE: the current running driver might not be the driver of the // operator that requests memory arbitration. The reason is that an @@ -679,7 +680,7 @@ void Operator::MemoryReclaimer::leaveArbitration() noexcept { return; } Driver* const runningDriver = driverThreadCtx->driverCtx()->driver; - if (!FLAGS_velox_memory_pool_capacity_transfer_across_tasks) { + if (!config::globalConfig().memoryPoolCapacityTransferAcrossTasks) { if (auto opDriver = ensureDriver()) { VELOX_CHECK_EQ( runningDriver->task()->taskId(), diff --git a/velox/exec/fuzzer/FuzzerUtil.cpp b/velox/exec/fuzzer/FuzzerUtil.cpp index 036957964cc8..4921e4b9f653 100644 --- a/velox/exec/fuzzer/FuzzerUtil.cpp +++ b/velox/exec/fuzzer/FuzzerUtil.cpp @@ -16,6 +16,7 @@ #include "velox/exec/fuzzer/FuzzerUtil.h" #include #include +#include "velox/common/config/GlobalConfig.h" #include "velox/common/memory/SharedArbitrator.h" #include "velox/connectors/hive/HiveConnector.h" #include "velox/connectors/hive/HiveConnectorSplit.h" @@ -333,8 +334,8 @@ void setupMemory( int64_t allocatorCapacity, int64_t arbitratorCapacity, bool enableGlobalArbitration) { - FLAGS_velox_enable_memory_usage_track_in_default_memory_pool = true; - FLAGS_velox_memory_leak_check_enabled = true; + config::globalConfig().enableMemoryUsageTrackInDefaultMemoryPool = true; + config::globalConfig().memoryLeakCheckEnabled = true; facebook::velox::memory::SharedArbitrator::registerFactory(); facebook::velox::memory::MemoryManagerOptions options; options.allocatorCapacity = allocatorCapacity; From c9680169bf069acc6178d12324bd48d439e742c5 Mon Sep 17 00:00:00 2001 From: Deepak Majeti Date: Thu, 5 Dec 2024 17:28:30 -0500 Subject: [PATCH 3/6] change static to global --- velox/buffer/tests/BufferTest.cpp | 5 ++--- velox/common/config/Config.cpp | 5 +---- velox/common/config/GlobalConfig.h | 4 ++-- velox/common/memory/MallocAllocator.cpp | 2 +- velox/common/memory/Memory.cpp | 4 ++-- velox/common/memory/Memory.h | 6 +++--- velox/common/memory/MemoryAllocator.cpp | 2 +- velox/common/memory/MemoryAllocator.h | 4 ++-- velox/common/memory/MemoryPool.cpp | 2 +- velox/common/memory/MemoryPool.h | 2 +- velox/common/memory/MmapAllocator.cpp | 2 +- velox/common/memory/tests/MemoryAllocatorTest.cpp | 6 +++--- velox/exec/Operator.cpp | 4 ++-- velox/exec/fuzzer/FuzzerUtil.cpp | 4 ++-- 14 files changed, 24 insertions(+), 28 deletions(-) diff --git a/velox/buffer/tests/BufferTest.cpp b/velox/buffer/tests/BufferTest.cpp index 9be2d59c6f86..78486c235abd 100644 --- a/velox/buffer/tests/BufferTest.cpp +++ b/velox/buffer/tests/BufferTest.cpp @@ -18,6 +18,7 @@ #include "folly/Range.h" #include "velox/common/base/tests/GTestUtils.h" +#include "velox/common/config/GlobalConfig.h" #include "velox/common/testutil/TestValue.h" #include "velox/type/StringView.h" @@ -25,8 +26,6 @@ #include #include -DECLARE_bool(velox_enable_memory_usage_track_in_default_memory_pool); - namespace facebook { namespace velox { @@ -39,7 +38,7 @@ static_assert(!Buffer::is_pod_like_v>, ""); class BufferTest : public testing::Test { protected: static void SetUpTestCase() { - FLAGS_velox_enable_memory_usage_track_in_default_memory_pool = true; + config::globalConfig.enableMemoryUsageTrackInDefaultMemoryPool = true; } void SetUp() override { diff --git a/velox/common/config/Config.cpp b/velox/common/config/Config.cpp index 20bb99fe7f34..b5cbb914a711 100644 --- a/velox/common/config/Config.cpp +++ b/velox/common/config/Config.cpp @@ -21,10 +21,7 @@ namespace facebook::velox::config { -GlobalConfig& globalConfig() { - static GlobalConfig config; - return config; -} +GlobalConfiguration globalConfig; double toBytesPerCapacityUnit(CapacityUnit unit) { switch (unit) { diff --git a/velox/common/config/GlobalConfig.h b/velox/common/config/GlobalConfig.h index a5eeea43e070..e616838be677 100644 --- a/velox/common/config/GlobalConfig.h +++ b/velox/common/config/GlobalConfig.h @@ -20,7 +20,7 @@ namespace facebook::velox::config { -struct GlobalConfig { +struct GlobalConfiguration { /// Number of shared leaf memory pools per process. int32_t memoryNumSharedLeafPoolsConfig{32}; /// If true, check fails on any memory leaks in memory pool and memory @@ -45,6 +45,6 @@ struct GlobalConfig { bool memoryPoolCapacityTransferAcrossTasks{false}; }; -GlobalConfig& globalConfig(); +extern GlobalConfiguration globalConfig; } // namespace facebook::velox::config diff --git a/velox/common/memory/MallocAllocator.cpp b/velox/common/memory/MallocAllocator.cpp index da6adb557566..b28d41c5a8c4 100644 --- a/velox/common/memory/MallocAllocator.cpp +++ b/velox/common/memory/MallocAllocator.cpp @@ -37,7 +37,7 @@ MallocAllocator::MallocAllocator(size_t capacity, uint32_t reservationByteLimit) MallocAllocator::~MallocAllocator() { // TODO: Remove the check when memory leak issue is resolved. - if (config::GlobalConfig().memoryLeakCheckEnabled) { + if (config::globalConfig.memoryLeakCheckEnabled) { VELOX_CHECK( ((allocatedBytes_ - reservations_.read()) == 0) && (numAllocated_ == 0) && (numMapped_ == 0), diff --git a/velox/common/memory/Memory.cpp b/velox/common/memory/Memory.cpp index 03e46b8b0c63..9920a1de7e71 100644 --- a/velox/common/memory/Memory.cpp +++ b/velox/common/memory/Memory.cpp @@ -78,7 +78,7 @@ std::vector> createSharedLeafMemoryPools( VELOX_CHECK_EQ(sysPool.name(), kSysRootName); std::vector> leafPools; const size_t numSharedPools = - std::max(1, config::GlobalConfig().memoryNumSharedLeafPoolsConfig); + std::max(1, config::globalConfig.memoryNumSharedLeafPoolsConfig); leafPools.reserve(numSharedPools); for (size_t i = 0; i < numSharedPools; ++i) { leafPools.emplace_back( @@ -128,7 +128,7 @@ MemoryManager::MemoryManager(const MemoryManagerOptions& options) sysRoot_->name()); VELOX_CHECK_EQ( sharedLeafPools_.size(), - std::max(1, config::GlobalConfig().memoryNumSharedLeafPoolsConfig)); + std::max(1, config::globalConfig.memoryNumSharedLeafPoolsConfig)); } MemoryManager::~MemoryManager() { diff --git a/velox/common/memory/Memory.h b/velox/common/memory/Memory.h index ece4a87bbdf3..26b8576ce9aa 100644 --- a/velox/common/memory/Memory.h +++ b/velox/common/memory/Memory.h @@ -61,18 +61,18 @@ struct MemoryManagerOptions { /// If true, enable memory usage tracking in the default memory pool. bool trackDefaultUsage{ - config::GlobalConfig().enableMemoryUsageTrackInDefaultMemoryPool}; + config::globalConfig.enableMemoryUsageTrackInDefaultMemoryPool}; /// If true, check the memory pool and usage leaks on destruction. /// /// TODO: deprecate this flag after all the existing memory leak use cases /// have been fixed. - bool checkUsageLeak{config::GlobalConfig().memoryLeakCheckEnabled}; + bool checkUsageLeak{config::globalConfig.memoryLeakCheckEnabled}; /// If true, the memory pool will be running in debug mode to track the /// allocation and free call stacks to detect the source of memory leak for /// testing purpose. - bool debugEnabled{config::GlobalConfig().memoryPoolDebugEnabled}; + bool debugEnabled{config::globalConfig.memoryPoolDebugEnabled}; /// Terminates the process and generates a core file on an allocation failure bool coreOnAllocationFailureEnabled{false}; diff --git a/velox/common/memory/MemoryAllocator.cpp b/velox/common/memory/MemoryAllocator.cpp index f2ff0dab247d..cc575bfd2168 100644 --- a/velox/common/memory/MemoryAllocator.cpp +++ b/velox/common/memory/MemoryAllocator.cpp @@ -413,7 +413,7 @@ void MemoryAllocator::useHugePages( const ContiguousAllocation& data, bool enable) { #ifdef linux - if (config::GlobalConfig().memoryUseHugepages) { + if (config::globalConfig.memoryUseHugepages) { return; } auto maybeRange = data.hugePageRange(); diff --git a/velox/common/memory/MemoryAllocator.h b/velox/common/memory/MemoryAllocator.h index 92f61b9dcfef..d10ecce32292 100644 --- a/velox/common/memory/MemoryAllocator.h +++ b/velox/common/memory/MemoryAllocator.h @@ -94,7 +94,7 @@ struct Stats { template void recordAllocate(int64_t bytes, int32_t count, Op op) { - if (config::GlobalConfig().timeAllocations) { + if (config::globalConfig.timeAllocations) { auto index = sizeIndex(bytes); velox::ClockTimer timer(sizes[index].allocateClocks); op(); @@ -107,7 +107,7 @@ struct Stats { template void recordFree(int64_t bytes, Op op) { - if (config::globalConfig().timeAllocations) { + if (config::globalConfig.timeAllocations) { auto index = sizeIndex(bytes); ClockTimer timer(sizes[index].freeClocks); op(); diff --git a/velox/common/memory/MemoryPool.cpp b/velox/common/memory/MemoryPool.cpp index 10c74de8c017..fc1f428fe388 100644 --- a/velox/common/memory/MemoryPool.cpp +++ b/velox/common/memory/MemoryPool.cpp @@ -964,7 +964,7 @@ std::string MemoryPoolImpl::treeMemoryUsage(bool skipEmptyPool) const { if (parent_ != nullptr) { return parent_->treeMemoryUsage(skipEmptyPool); } - if (config::globalConfig().suppressMemoryCapacityExceedingErrorMessage) { + if (config::globalConfig.suppressMemoryCapacityExceedingErrorMessage) { return ""; } std::stringstream out; diff --git a/velox/common/memory/MemoryPool.h b/velox/common/memory/MemoryPool.h index f95e44f30a88..32d29625fd14 100644 --- a/velox/common/memory/MemoryPool.h +++ b/velox/common/memory/MemoryPool.h @@ -151,7 +151,7 @@ class MemoryPool : public std::enable_shared_from_this { /// If true, tracks the allocation and free call stacks to detect the source /// of memory leak for testing purpose. - bool debugEnabled{config::globalConfig().memoryPoolDebugEnabled}; + bool debugEnabled{config::globalConfig.memoryPoolDebugEnabled}; /// Terminates the process and generates a core file on an allocation /// failure diff --git a/velox/common/memory/MmapAllocator.cpp b/velox/common/memory/MmapAllocator.cpp index 6060b4cd7b74..9fef3d3473dc 100644 --- a/velox/common/memory/MmapAllocator.cpp +++ b/velox/common/memory/MmapAllocator.cpp @@ -191,7 +191,7 @@ MachinePageCount MmapAllocator::freeNonContiguousInternal( ClockTimer timer(clocks); pages = sizeClass->free(allocation); } - if ((pages > 0) && config::globalConfig().timeAllocations) { + if ((pages > 0) && config::globalConfig.timeAllocations) { // Increment the free time only if the allocation contained // pages in the class. Note that size class indices in the // allocator are not necessarily the same as in the stats. diff --git a/velox/common/memory/tests/MemoryAllocatorTest.cpp b/velox/common/memory/tests/MemoryAllocatorTest.cpp index 990ce4a510dc..d8caae91cf5b 100644 --- a/velox/common/memory/tests/MemoryAllocatorTest.cpp +++ b/velox/common/memory/tests/MemoryAllocatorTest.cpp @@ -54,7 +54,7 @@ class MemoryAllocatorTest : public testing::TestWithParam { protected: static void SetUpTestCase() { TestValue::enable(); - config::globalConfig().memoryLeakCheckEnabled = true; + config::globalConfig.memoryLeakCheckEnabled = true; } void SetUp() override { @@ -650,7 +650,7 @@ TEST_P(MemoryAllocatorTest, stats) { } gflags::FlagSaver flagSaver; - config::globalConfig().timeAllocations = true; + config::globalConfig.timeAllocations = true; for (auto i = 0; i < sizes.size(); ++i) { std::unique_ptr allocation = std::make_unique(); auto size = sizes[i]; @@ -669,7 +669,7 @@ TEST_P(MemoryAllocatorTest, singleAllocation) { return; } gflags::FlagSaver flagSaver; - config::globalConfig().timeAllocations = true; + config::globalConfig.timeAllocations = true; const std::vector& sizes = instance_->sizeClasses(); MachinePageCount capacity = kCapacityPages; for (auto i = 0; i < sizes.size(); ++i) { diff --git a/velox/exec/Operator.cpp b/velox/exec/Operator.cpp index 35c13c5e34ee..e80b3729c30f 100644 --- a/velox/exec/Operator.cpp +++ b/velox/exec/Operator.cpp @@ -650,7 +650,7 @@ void Operator::MemoryReclaimer::enterArbitration() { } Driver* const runningDriver = driverThreadCtx->driverCtx()->driver; - if (!config::globalConfig().memoryPoolCapacityTransferAcrossTasks) { + if (!config::globalConfig.memoryPoolCapacityTransferAcrossTasks) { if (auto opDriver = ensureDriver()) { // NOTE: the current running driver might not be the driver of the // operator that requests memory arbitration. The reason is that an @@ -680,7 +680,7 @@ void Operator::MemoryReclaimer::leaveArbitration() noexcept { return; } Driver* const runningDriver = driverThreadCtx->driverCtx()->driver; - if (!config::globalConfig().memoryPoolCapacityTransferAcrossTasks) { + if (!config::globalConfig.memoryPoolCapacityTransferAcrossTasks) { if (auto opDriver = ensureDriver()) { VELOX_CHECK_EQ( runningDriver->task()->taskId(), diff --git a/velox/exec/fuzzer/FuzzerUtil.cpp b/velox/exec/fuzzer/FuzzerUtil.cpp index 4921e4b9f653..41868e4b16b4 100644 --- a/velox/exec/fuzzer/FuzzerUtil.cpp +++ b/velox/exec/fuzzer/FuzzerUtil.cpp @@ -334,8 +334,8 @@ void setupMemory( int64_t allocatorCapacity, int64_t arbitratorCapacity, bool enableGlobalArbitration) { - config::globalConfig().enableMemoryUsageTrackInDefaultMemoryPool = true; - config::globalConfig().memoryLeakCheckEnabled = true; + config::globalConfig.enableMemoryUsageTrackInDefaultMemoryPool = true; + config::globalConfig.memoryLeakCheckEnabled = true; facebook::velox::memory::SharedArbitrator::registerFactory(); facebook::velox::memory::MemoryManagerOptions options; options.allocatorCapacity = allocatorCapacity; From cf4c989e9f52a44bc09e98095304219b3a2f7be8 Mon Sep 17 00:00:00 2001 From: Deepak Majeti Date: Fri, 6 Dec 2024 07:42:37 -0500 Subject: [PATCH 4/6] add translation function --- velox/buffer/tests/BufferTest.cpp | 6 ++-- velox/common/config/GlobalConfig.h | 4 ++- velox/common/memory/Memory.cpp | 4 +-- .../memory/tests/MemoryAllocatorTest.cpp | 31 ++++++++++++------- .../memory/tests/MemoryCapExceededTest.cpp | 2 ++ .../common/memory/tests/MemoryManagerTest.cpp | 2 ++ .../test/TestIntegerDictionaryEncoder.cpp | 1 + .../dwrf/test/TestStringDictionaryEncoder.cpp | 1 + velox/exec/tests/utils/OperatorTestBase.cpp | 1 + velox/flag_definitions/flags.cpp | 27 ++++++++++++++++ 10 files changed, 62 insertions(+), 17 deletions(-) diff --git a/velox/buffer/tests/BufferTest.cpp b/velox/buffer/tests/BufferTest.cpp index 78486c235abd..d0eac34b7499 100644 --- a/velox/buffer/tests/BufferTest.cpp +++ b/velox/buffer/tests/BufferTest.cpp @@ -18,7 +18,6 @@ #include "folly/Range.h" #include "velox/common/base/tests/GTestUtils.h" -#include "velox/common/config/GlobalConfig.h" #include "velox/common/testutil/TestValue.h" #include "velox/type/StringView.h" @@ -26,6 +25,8 @@ #include #include +DECLARE_bool(velox_enable_memory_usage_track_in_default_memory_pool); + namespace facebook { namespace velox { @@ -38,7 +39,8 @@ static_assert(!Buffer::is_pod_like_v>, ""); class BufferTest : public testing::Test { protected: static void SetUpTestCase() { - config::globalConfig.enableMemoryUsageTrackInDefaultMemoryPool = true; + FLAGS_velox_enable_memory_usage_track_in_default_memory_pool = true; + config::translateFlagsToGlobalConfig(); } void SetUp() override { diff --git a/velox/common/config/GlobalConfig.h b/velox/common/config/GlobalConfig.h index e616838be677..26de8d36124f 100644 --- a/velox/common/config/GlobalConfig.h +++ b/velox/common/config/GlobalConfig.h @@ -22,7 +22,7 @@ namespace facebook::velox::config { struct GlobalConfiguration { /// Number of shared leaf memory pools per process. - int32_t memoryNumSharedLeafPoolsConfig{32}; + int32_t memoryNumSharedLeafPools{32}; /// If true, check fails on any memory leaks in memory pool and memory /// manager. bool memoryLeakCheckEnabled{false}; @@ -47,4 +47,6 @@ struct GlobalConfiguration { extern GlobalConfiguration globalConfig; +void translateFlagsToGlobalConfig(); + } // namespace facebook::velox::config diff --git a/velox/common/memory/Memory.cpp b/velox/common/memory/Memory.cpp index 9920a1de7e71..1d9ac77d4015 100644 --- a/velox/common/memory/Memory.cpp +++ b/velox/common/memory/Memory.cpp @@ -78,7 +78,7 @@ std::vector> createSharedLeafMemoryPools( VELOX_CHECK_EQ(sysPool.name(), kSysRootName); std::vector> leafPools; const size_t numSharedPools = - std::max(1, config::globalConfig.memoryNumSharedLeafPoolsConfig); + std::max(1, config::globalConfig.memoryNumSharedLeafPools); leafPools.reserve(numSharedPools); for (size_t i = 0; i < numSharedPools; ++i) { leafPools.emplace_back( @@ -128,7 +128,7 @@ MemoryManager::MemoryManager(const MemoryManagerOptions& options) sysRoot_->name()); VELOX_CHECK_EQ( sharedLeafPools_.size(), - std::max(1, config::globalConfig.memoryNumSharedLeafPoolsConfig)); + std::max(1, config::globalConfig.memoryNumSharedLeafPools)); } MemoryManager::~MemoryManager() { diff --git a/velox/common/memory/tests/MemoryAllocatorTest.cpp b/velox/common/memory/tests/MemoryAllocatorTest.cpp index d8caae91cf5b..f83b657270f2 100644 --- a/velox/common/memory/tests/MemoryAllocatorTest.cpp +++ b/velox/common/memory/tests/MemoryAllocatorTest.cpp @@ -16,7 +16,6 @@ #include "velox/common/memory/MemoryAllocator.h" #include #include "velox/common/base/tests/GTestUtils.h" -#include "velox/common/config/GlobalConfig.h" #include "velox/common/memory/AllocationPool.h" #include "velox/common/memory/MallocAllocator.h" #include "velox/common/memory/MmapAllocator.h" @@ -35,6 +34,9 @@ #include #endif // linux +DECLARE_bool(velox_memory_leak_check_enabled); +DECLARE_bool(velox_time_allocations); + using namespace facebook::velox::common::testutil; namespace facebook::velox::memory { @@ -54,7 +56,8 @@ class MemoryAllocatorTest : public testing::TestWithParam { protected: static void SetUpTestCase() { TestValue::enable(); - config::globalConfig.memoryLeakCheckEnabled = true; + FLAGS_velox_memory_leak_check_enabled = true; + config::translateFlagsToGlobalConfig(); } void SetUp() override { @@ -649,8 +652,8 @@ TEST_P(MemoryAllocatorTest, stats) { ASSERT_EQ(stats.sizes[i].numAllocations, 0); } - gflags::FlagSaver flagSaver; - config::globalConfig.timeAllocations = true; + FLAGS_velox_time_allocations = true; + config::translateFlagsToGlobalConfig(); for (auto i = 0; i < sizes.size(); ++i) { std::unique_ptr allocation = std::make_unique(); auto size = sizes[i]; @@ -662,14 +665,17 @@ TEST_P(MemoryAllocatorTest, stats) { ASSERT_GE(stats.sizes[i].totalBytes, size * AllocationTraits::kPageSize); ASSERT_GE(stats.sizes[i].numAllocations, 1); } + FLAGS_velox_time_allocations = false; + config::translateFlagsToGlobalConfig(); } TEST_P(MemoryAllocatorTest, singleAllocation) { if (!useMmap_ && enableReservation_) { return; } - gflags::FlagSaver flagSaver; - config::globalConfig.timeAllocations = true; + + FLAGS_velox_time_allocations = true; + config::translateFlagsToGlobalConfig(); const std::vector& sizes = instance_->sizeClasses(); MachinePageCount capacity = kCapacityPages; for (auto i = 0; i < sizes.size(); ++i) { @@ -716,6 +722,8 @@ TEST_P(MemoryAllocatorTest, singleAllocation) { } ASSERT_TRUE(instance_->checkConsistency()); } + FLAGS_velox_time_allocations = false; + config::translateFlagsToGlobalConfig(); } TEST_P(MemoryAllocatorTest, increasingSize) { @@ -1613,6 +1621,11 @@ TEST_P(MemoryAllocatorTest, allocatorCapacityWithThreads) { EXPECT_EQ(instance_->numAllocated(), 0); } +VELOX_INSTANTIATE_TEST_SUITE_P( + MemoryAllocatorTestSuite, + MemoryAllocatorTest, + testing::ValuesIn({0, 1, 2})); + class MmapArenaTest : public testing::Test { public: // 32 MB arena space @@ -1942,11 +1955,6 @@ TEST_P(MemoryAllocatorTest, unmap) { } } -VELOX_INSTANTIATE_TEST_SUITE_P( - MemoryAllocatorTestSuite, - MemoryAllocatorTest, - testing::ValuesIn({0, 1, 2})); - class MmapConfigTest : public testing::Test { public: protected: @@ -1993,5 +2001,4 @@ TEST_F(MmapConfigTest, sizeClasses) { runPages = runPages / 2; } } - } // namespace facebook::velox::memory diff --git a/velox/common/memory/tests/MemoryCapExceededTest.cpp b/velox/common/memory/tests/MemoryCapExceededTest.cpp index ef174b8a765f..a1d4c695882b 100644 --- a/velox/common/memory/tests/MemoryCapExceededTest.cpp +++ b/velox/common/memory/tests/MemoryCapExceededTest.cpp @@ -34,11 +34,13 @@ class MemoryCapExceededTest : public OperatorTestBase, // NOTE: if 'GetParam()' is true, then suppress the verbose error message in // memory capacity exceeded exception. FLAGS_velox_suppress_memory_capacity_exceeding_error_message = GetParam(); + config::translateFlagsToGlobalConfig(); } void TearDown() override { OperatorTestBase::TearDown(); FLAGS_velox_suppress_memory_capacity_exceeding_error_message = false; + config::translateFlagsToGlobalConfig(); } }; diff --git a/velox/common/memory/tests/MemoryManagerTest.cpp b/velox/common/memory/tests/MemoryManagerTest.cpp index c4a7329cf8ef..cbfa8875ff2e 100644 --- a/velox/common/memory/tests/MemoryManagerTest.cpp +++ b/velox/common/memory/tests/MemoryManagerTest.cpp @@ -44,6 +44,7 @@ class MemoryManagerTest : public testing::Test { protected: static void SetUpTestCase() { SharedArbitrator::registerFactory(); + config::translateFlagsToGlobalConfig(); } inline static const std::string arbitratorKind_{"SHARED"}; @@ -370,6 +371,7 @@ TEST_F(MemoryManagerTest, defaultMemoryUsageTracking) { for (bool trackDefaultMemoryUsage : {false, true}) { FLAGS_velox_enable_memory_usage_track_in_default_memory_pool = trackDefaultMemoryUsage; + config::translateFlagsToGlobalConfig(); MemoryManager manager{}; auto defaultPool = manager.addLeafPool("defaultMemoryUsageTracking"); ASSERT_EQ(defaultPool->trackUsage(), trackDefaultMemoryUsage); diff --git a/velox/dwio/dwrf/test/TestIntegerDictionaryEncoder.cpp b/velox/dwio/dwrf/test/TestIntegerDictionaryEncoder.cpp index 8f1e1675c584..16ecca5f3663 100644 --- a/velox/dwio/dwrf/test/TestIntegerDictionaryEncoder.cpp +++ b/velox/dwio/dwrf/test/TestIntegerDictionaryEncoder.cpp @@ -29,6 +29,7 @@ class TestIntegerDictionaryEncoder : public ::testing::Test { protected: static void SetUpTestCase() { FLAGS_velox_enable_memory_usage_track_in_default_memory_pool = true; + config::translateFlagsToGlobalConfig(); memory::MemoryManager::testingSetInstance({}); } }; diff --git a/velox/dwio/dwrf/test/TestStringDictionaryEncoder.cpp b/velox/dwio/dwrf/test/TestStringDictionaryEncoder.cpp index f6dd8d848f03..e547f7d16d9c 100644 --- a/velox/dwio/dwrf/test/TestStringDictionaryEncoder.cpp +++ b/velox/dwio/dwrf/test/TestStringDictionaryEncoder.cpp @@ -28,6 +28,7 @@ class TestStringDictionaryEncoder : public ::testing::Test { protected: static void SetUpTestCase() { FLAGS_velox_enable_memory_usage_track_in_default_memory_pool = true; + config::translateFlagsToGlobalConfig(); memory::MemoryManager::testingSetInstance({}); } }; diff --git a/velox/exec/tests/utils/OperatorTestBase.cpp b/velox/exec/tests/utils/OperatorTestBase.cpp index 69300dafb0b3..33ce9c6603b9 100644 --- a/velox/exec/tests/utils/OperatorTestBase.cpp +++ b/velox/exec/tests/utils/OperatorTestBase.cpp @@ -63,6 +63,7 @@ OperatorTestBase::~OperatorTestBase() { void OperatorTestBase::SetUpTestCase() { FLAGS_velox_enable_memory_usage_track_in_default_memory_pool = true; FLAGS_velox_memory_leak_check_enabled = true; + config::translateFlagsToGlobalConfig(); memory::SharedArbitrator::registerFactory(); resetMemory(); functions::prestosql::registerAllScalarFunctions(); diff --git a/velox/flag_definitions/flags.cpp b/velox/flag_definitions/flags.cpp index ee308964ac7b..17d5aae2dbc6 100644 --- a/velox/flag_definitions/flags.cpp +++ b/velox/flag_definitions/flags.cpp @@ -16,6 +16,8 @@ #include +#include "velox/common/config/GlobalConfig.h" + // Used in velox/common/memory/Memory.cpp DEFINE_int32( @@ -100,3 +102,28 @@ DEFINE_bool( "exception. This is only used by test to control the test error output size"); DEFINE_bool(velox_memory_use_hugepages, true, "Use explicit huge pages"); + +DEFINE_bool( + velox_memory_pool_capacity_transfer_across_tasks, + false, + "Whether allow to memory capacity transfer between memory pools from " + "different tasks, which might happen in use case like Spark-Gluten"); + +namespace facebook::velox::config { +void translateFlagsToGlobalConfig() { + config::globalConfig.memoryNumSharedLeafPools = + FLAGS_velox_memory_num_shared_leaf_pools; + config::globalConfig.memoryLeakCheckEnabled = + FLAGS_velox_memory_leak_check_enabled; + config::globalConfig.memoryPoolDebugEnabled = + FLAGS_velox_memory_pool_debug_enabled; + config::globalConfig.enableMemoryUsageTrackInDefaultMemoryPool = + FLAGS_velox_enable_memory_usage_track_in_default_memory_pool; + config::globalConfig.timeAllocations = FLAGS_velox_time_allocations; + config::globalConfig.memoryUseHugepages = FLAGS_velox_memory_use_hugepages; + config::globalConfig.suppressMemoryCapacityExceedingErrorMessage = + FLAGS_velox_suppress_memory_capacity_exceeding_error_message; + config::globalConfig.memoryPoolCapacityTransferAcrossTasks = + FLAGS_velox_memory_pool_capacity_transfer_across_tasks; +} +} // namespace facebook::velox::config From 0bfa11f36401e07f6851e0c34ef01b41581472c8 Mon Sep 17 00:00:00 2001 From: Deepak Majeti Date: Fri, 6 Dec 2024 16:28:33 -0500 Subject: [PATCH 5/6] address review comments and fix velox_exception --- velox/buffer/tests/BufferTest.cpp | 9 ++++---- velox/buffer/tests/CMakeLists.txt | 1 + velox/common/base/CMakeLists.txt | 10 ++------ velox/common/base/VeloxException.cpp | 13 +++++++---- velox/common/base/VeloxException.h | 1 - velox/common/base/tests/CMakeLists.txt | 1 + velox/common/base/tests/ExceptionTest.cpp | 13 +++++++---- velox/common/config/CMakeLists.txt | 9 ++++---- velox/common/config/Config.cpp | 3 --- velox/common/config/GlobalConfig.cpp | 23 +++++++++++++++++++ velox/common/config/GlobalConfig.h | 12 ++++++++-- .../memory/tests/MemoryAllocatorTest.cpp | 11 +++++---- .../memory/tests/MemoryCapExceededTest.cpp | 5 ++-- .../common/memory/tests/MemoryManagerTest.cpp | 5 ++-- velox/dwio/common/tests/CMakeLists.txt | 1 + .../dwio/common/tests/LoggedExceptionTest.cpp | 13 +++++++---- velox/dwio/dwrf/test/CMakeLists.txt | 1 + .../test/TestIntegerDictionaryEncoder.cpp | 3 ++- .../dwrf/test/TestStringDictionaryEncoder.cpp | 3 ++- velox/exec/tests/utils/CMakeLists.txt | 1 + velox/exec/tests/utils/OperatorTestBase.cpp | 3 ++- velox/flag_definitions/CMakeLists.txt | 3 ++- velox/flag_definitions/flags.cpp | 12 ++++++++-- velox/flag_definitions/flags.h | 19 +++++++++++++++ 24 files changed, 123 insertions(+), 52 deletions(-) create mode 100644 velox/common/config/GlobalConfig.cpp create mode 100644 velox/flag_definitions/flags.h diff --git a/velox/buffer/tests/BufferTest.cpp b/velox/buffer/tests/BufferTest.cpp index d0eac34b7499..551b21585590 100644 --- a/velox/buffer/tests/BufferTest.cpp +++ b/velox/buffer/tests/BufferTest.cpp @@ -19,6 +19,7 @@ #include "folly/Range.h" #include "velox/common/base/tests/GTestUtils.h" #include "velox/common/testutil/TestValue.h" +#include "velox/flag_definitions/flags.h" #include "velox/type/StringView.h" #include @@ -27,8 +28,7 @@ DECLARE_bool(velox_enable_memory_usage_track_in_default_memory_pool); -namespace facebook { -namespace velox { +namespace facebook::velox { static_assert(Buffer::is_pod_like_v, ""); static_assert(Buffer::is_pod_like_v, ""); @@ -40,7 +40,7 @@ class BufferTest : public testing::Test { protected: static void SetUpTestCase() { FLAGS_velox_enable_memory_usage_track_in_default_memory_pool = true; - config::translateFlagsToGlobalConfig(); + translateFlagsToGlobalConfig(); } void SetUp() override { @@ -491,5 +491,4 @@ TEST_F(BufferTest, sliceBooleanBuffer) { Buffer::slice(bufferPtr, 5, 6, nullptr), "Pool must not be null."); } -} // namespace velox -} // namespace facebook +} // namespace facebook::velox diff --git a/velox/buffer/tests/CMakeLists.txt b/velox/buffer/tests/CMakeLists.txt index 097f5ee3bb3f..9e2bfaa4b9fc 100644 --- a/velox/buffer/tests/CMakeLists.txt +++ b/velox/buffer/tests/CMakeLists.txt @@ -20,6 +20,7 @@ target_link_libraries( velox_memory velox_buffer velox_test_util + velox_flag_definitions GTest::gtest GTest::gtest_main GTest::gmock diff --git a/velox/common/base/CMakeLists.txt b/velox/common/base/CMakeLists.txt index dad8128a343f..3bde1734d28f 100644 --- a/velox/common/base/CMakeLists.txt +++ b/velox/common/base/CMakeLists.txt @@ -14,14 +14,8 @@ velox_add_library(velox_exception Exceptions.cpp VeloxException.cpp Exceptions.h) -velox_link_libraries( - velox_exception - PUBLIC velox_flag_definitions - velox_process - Folly::folly - fmt::fmt - gflags::gflags - glog::glog) +velox_link_libraries(velox_exception PUBLIC velox_process Folly::folly fmt::fmt + glog::glog) velox_add_library( velox_common_base diff --git a/velox/common/base/VeloxException.cpp b/velox/common/base/VeloxException.cpp index 82af2ba8a7d4..5d1a6bf45f92 100644 --- a/velox/common/base/VeloxException.cpp +++ b/velox/common/base/VeloxException.cpp @@ -15,6 +15,7 @@ */ #include "velox/common/base/VeloxException.h" +#include "velox/common/config/GlobalConfig.h" #include #include @@ -134,16 +135,18 @@ namespace { bool isStackTraceEnabled(VeloxException::Type type) { using namespace std::literals::chrono_literals; const bool isSysException = type == VeloxException::Type::kSystem; - if ((isSysException && !FLAGS_velox_exception_system_stacktrace_enabled) || - (!isSysException && !FLAGS_velox_exception_user_stacktrace_enabled)) { + if ((isSysException && + !config::globalConfig.exceptionSystemStacktraceEnabled) || + (!isSysException && + !config::globalConfig.exceptionUserStacktraceEnabled)) { // VeloxException stacktraces are disabled. return false; } const int32_t rateLimitMs = isSysException - ? FLAGS_velox_exception_system_stacktrace_rate_limit_ms - : FLAGS_velox_exception_user_stacktrace_rate_limit_ms; - // not static so the gflag can be manipulated at runtime + ? config::globalConfig.exceptionSystemStacktraceRateLimitMs + : config::globalConfig.exceptionUserStacktraceRateLimitMs; + // not static so the global config can be manipulated at runtime if (0 == rateLimitMs) { // VeloxException stacktraces are not rate-limited return true; diff --git a/velox/common/base/VeloxException.h b/velox/common/base/VeloxException.h index 148ae38f06a2..5ed38f645039 100644 --- a/velox/common/base/VeloxException.h +++ b/velox/common/base/VeloxException.h @@ -23,7 +23,6 @@ #include #include #include -#include #include #include "velox/common/process/StackTrace.h" diff --git a/velox/common/base/tests/CMakeLists.txt b/velox/common/base/tests/CMakeLists.txt index 7d85566bf4ed..3032eea49c5a 100644 --- a/velox/common/base/tests/CMakeLists.txt +++ b/velox/common/base/tests/CMakeLists.txt @@ -49,6 +49,7 @@ target_link_libraries( velox_status velox_exception velox_temp_path + velox_flag_definitions Boost::filesystem Boost::headers Folly::folly diff --git a/velox/common/base/tests/ExceptionTest.cpp b/velox/common/base/tests/ExceptionTest.cpp index 9386b8cb672e..0d279caf446f 100644 --- a/velox/common/base/tests/ExceptionTest.cpp +++ b/velox/common/base/tests/ExceptionTest.cpp @@ -19,6 +19,8 @@ #include #include "velox/common/base/Exceptions.h" +#include "velox/common/config/GlobalConfig.h" +#include "velox/flag_definitions/flags.h" using namespace facebook::velox; @@ -83,6 +85,7 @@ void testExceptionTraceCollectionControl(bool userException, bool enabled) { FLAGS_velox_exception_system_stacktrace_enabled = enabled ? true : false; FLAGS_velox_exception_user_stacktrace_enabled = folly::Random::oneIn(2); } + translateFlagsToGlobalConfig(); try { if (userException) { throw VeloxUserError( @@ -109,8 +112,8 @@ void testExceptionTraceCollectionControl(bool userException, bool enabled) { SCOPED_TRACE(fmt::format( "enabled: {}, user flag: {}, sys flag: {}", enabled, - FLAGS_velox_exception_user_stacktrace_enabled, - FLAGS_velox_exception_system_stacktrace_enabled)); + config::globalConfig.exceptionUserStacktraceEnabled, + config::globalConfig.exceptionSystemStacktraceEnabled)); ASSERT_EQ(userException, e.exceptionType() == VeloxException::Type::kUser); ASSERT_EQ(enabled, e.stackTrace() != nullptr); } @@ -123,6 +126,7 @@ void testExceptionTraceCollectionRateControl( // Enable trace rate control in the test. FLAGS_velox_exception_user_stacktrace_enabled = true; FLAGS_velox_exception_system_stacktrace_enabled = true; + translateFlagsToGlobalConfig(); // Set rate control interval to a large value to avoid time related test // flakiness. const int kRateLimitIntervalMs = 4000; @@ -146,6 +150,7 @@ void testExceptionTraceCollectionRateControl( FLAGS_velox_exception_user_stacktrace_rate_limit_ms = folly::Random::rand32(); } + translateFlagsToGlobalConfig(); for (int iter = 0; iter < 3; ++iter) { try { if (userException) { @@ -174,8 +179,8 @@ void testExceptionTraceCollectionRateControl( "userException: {}, hasRateLimit: {}, user limit: {}ms, sys limit: {}ms", userException, hasRateLimit, - FLAGS_velox_exception_user_stacktrace_rate_limit_ms, - FLAGS_velox_exception_system_stacktrace_rate_limit_ms)); + config::globalConfig.exceptionUserStacktraceRateLimitMs, + config::globalConfig.exceptionSystemStacktraceRateLimitMs)); ASSERT_EQ( userException, e.exceptionType() == VeloxException::Type::kUser); ASSERT_EQ(!hasRateLimit || ((iter % 2) == 0), e.stackTrace() != nullptr); diff --git a/velox/common/config/CMakeLists.txt b/velox/common/config/CMakeLists.txt index 7780665a2925..c37336f68686 100644 --- a/velox/common/config/CMakeLists.txt +++ b/velox/common/config/CMakeLists.txt @@ -12,13 +12,14 @@ # See the License for the specific language governing permissions and # limitations under the License. -if (${VELOX_BUILD_TESTING}) +if(${VELOX_BUILD_TESTING}) add_subdirectory(tests) -endif () +endif() + +velox_add_library(velox_global_config OBJECT GlobalConfig.cpp) velox_add_library(velox_common_config Config.cpp) velox_link_libraries( velox_common_config - PUBLIC velox_common_base - velox_exception + PUBLIC velox_common_base velox_global_config velox_exception PRIVATE re2::re2) diff --git a/velox/common/config/Config.cpp b/velox/common/config/Config.cpp index b5cbb914a711..18cc705949cd 100644 --- a/velox/common/config/Config.cpp +++ b/velox/common/config/Config.cpp @@ -17,12 +17,9 @@ #include #include "velox/common/config/Config.h" -#include "velox/common/config/GlobalConfig.h" namespace facebook::velox::config { -GlobalConfiguration globalConfig; - double toBytesPerCapacityUnit(CapacityUnit unit) { switch (unit) { case CapacityUnit::BYTE: diff --git a/velox/common/config/GlobalConfig.cpp b/velox/common/config/GlobalConfig.cpp new file mode 100644 index 000000000000..9dd5e53e14e9 --- /dev/null +++ b/velox/common/config/GlobalConfig.cpp @@ -0,0 +1,23 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "velox/common/config/GlobalConfig.h" + +namespace facebook::velox::config { + +GlobalConfiguration globalConfig; + +} // namespace facebook::velox::config diff --git a/velox/common/config/GlobalConfig.h b/velox/common/config/GlobalConfig.h index 26de8d36124f..1ccd8e2bed27 100644 --- a/velox/common/config/GlobalConfig.h +++ b/velox/common/config/GlobalConfig.h @@ -43,10 +43,18 @@ struct GlobalConfiguration { /// Whether allow to memory capacity transfer between memory pools from /// different tasks, which might happen in use case like Spark-Gluten bool memoryPoolCapacityTransferAcrossTasks{false}; + /// Enable the stacktrace for system type of VeloxException. + bool exceptionSystemStacktraceEnabled{true}; + /// Enable the stacktrace for user type of VeloxException. + bool exceptionUserStacktraceEnabled{false}; + /// Min time interval in milliseconds between stack traces captured in + /// user type of VeloxException; off when set to 0 (the default). + int32_t exceptionUserStacktraceRateLimitMs{0}; + /// Min time interval in milliseconds between stack traces captured in + /// system type of VeloxException; off when set to 0 (the default). + int32_t exceptionSystemStacktraceRateLimitMs{0}; }; extern GlobalConfiguration globalConfig; -void translateFlagsToGlobalConfig(); - } // namespace facebook::velox::config diff --git a/velox/common/memory/tests/MemoryAllocatorTest.cpp b/velox/common/memory/tests/MemoryAllocatorTest.cpp index f83b657270f2..02fcefebf576 100644 --- a/velox/common/memory/tests/MemoryAllocatorTest.cpp +++ b/velox/common/memory/tests/MemoryAllocatorTest.cpp @@ -22,6 +22,7 @@ #include "velox/common/memory/MmapArena.h" #include "velox/common/memory/SharedArbitrator.h" #include "velox/common/testutil/TestValue.h" +#include "velox/flag_definitions/flags.h" #include #include @@ -57,7 +58,7 @@ class MemoryAllocatorTest : public testing::TestWithParam { static void SetUpTestCase() { TestValue::enable(); FLAGS_velox_memory_leak_check_enabled = true; - config::translateFlagsToGlobalConfig(); + translateFlagsToGlobalConfig(); } void SetUp() override { @@ -653,7 +654,7 @@ TEST_P(MemoryAllocatorTest, stats) { } FLAGS_velox_time_allocations = true; - config::translateFlagsToGlobalConfig(); + translateFlagsToGlobalConfig(); for (auto i = 0; i < sizes.size(); ++i) { std::unique_ptr allocation = std::make_unique(); auto size = sizes[i]; @@ -666,7 +667,7 @@ TEST_P(MemoryAllocatorTest, stats) { ASSERT_GE(stats.sizes[i].numAllocations, 1); } FLAGS_velox_time_allocations = false; - config::translateFlagsToGlobalConfig(); + translateFlagsToGlobalConfig(); } TEST_P(MemoryAllocatorTest, singleAllocation) { @@ -675,7 +676,7 @@ TEST_P(MemoryAllocatorTest, singleAllocation) { } FLAGS_velox_time_allocations = true; - config::translateFlagsToGlobalConfig(); + translateFlagsToGlobalConfig(); const std::vector& sizes = instance_->sizeClasses(); MachinePageCount capacity = kCapacityPages; for (auto i = 0; i < sizes.size(); ++i) { @@ -723,7 +724,7 @@ TEST_P(MemoryAllocatorTest, singleAllocation) { ASSERT_TRUE(instance_->checkConsistency()); } FLAGS_velox_time_allocations = false; - config::translateFlagsToGlobalConfig(); + translateFlagsToGlobalConfig(); } TEST_P(MemoryAllocatorTest, increasingSize) { diff --git a/velox/common/memory/tests/MemoryCapExceededTest.cpp b/velox/common/memory/tests/MemoryCapExceededTest.cpp index a1d4c695882b..0518f6da49cd 100644 --- a/velox/common/memory/tests/MemoryCapExceededTest.cpp +++ b/velox/common/memory/tests/MemoryCapExceededTest.cpp @@ -19,6 +19,7 @@ #include "velox/common/memory/MmapAllocator.h" #include "velox/exec/tests/utils/OperatorTestBase.h" #include "velox/exec/tests/utils/PlanBuilder.h" +#include "velox/flag_definitions/flags.h" #include @@ -34,13 +35,13 @@ class MemoryCapExceededTest : public OperatorTestBase, // NOTE: if 'GetParam()' is true, then suppress the verbose error message in // memory capacity exceeded exception. FLAGS_velox_suppress_memory_capacity_exceeding_error_message = GetParam(); - config::translateFlagsToGlobalConfig(); + translateFlagsToGlobalConfig(); } void TearDown() override { OperatorTestBase::TearDown(); FLAGS_velox_suppress_memory_capacity_exceeding_error_message = false; - config::translateFlagsToGlobalConfig(); + translateFlagsToGlobalConfig(); } }; diff --git a/velox/common/memory/tests/MemoryManagerTest.cpp b/velox/common/memory/tests/MemoryManagerTest.cpp index cbfa8875ff2e..cbbf33b389b5 100644 --- a/velox/common/memory/tests/MemoryManagerTest.cpp +++ b/velox/common/memory/tests/MemoryManagerTest.cpp @@ -24,6 +24,7 @@ #include "velox/common/memory/MallocAllocator.h" #include "velox/common/memory/Memory.h" #include "velox/common/memory/SharedArbitrator.h" +#include "velox/flag_definitions/flags.h" DECLARE_int32(velox_memory_num_shared_leaf_pools); DECLARE_bool(velox_enable_memory_usage_track_in_default_memory_pool); @@ -44,7 +45,7 @@ class MemoryManagerTest : public testing::Test { protected: static void SetUpTestCase() { SharedArbitrator::registerFactory(); - config::translateFlagsToGlobalConfig(); + translateFlagsToGlobalConfig(); } inline static const std::string arbitratorKind_{"SHARED"}; @@ -371,7 +372,7 @@ TEST_F(MemoryManagerTest, defaultMemoryUsageTracking) { for (bool trackDefaultMemoryUsage : {false, true}) { FLAGS_velox_enable_memory_usage_track_in_default_memory_pool = trackDefaultMemoryUsage; - config::translateFlagsToGlobalConfig(); + translateFlagsToGlobalConfig(); MemoryManager manager{}; auto defaultPool = manager.addLeafPool("defaultMemoryUsageTracking"); ASSERT_EQ(defaultPool->trackUsage(), trackDefaultMemoryUsage); diff --git a/velox/dwio/common/tests/CMakeLists.txt b/velox/dwio/common/tests/CMakeLists.txt index 6f3a36273c6d..1ee9346155c3 100644 --- a/velox/dwio/common/tests/CMakeLists.txt +++ b/velox/dwio/common/tests/CMakeLists.txt @@ -62,6 +62,7 @@ target_link_libraries( velox_dwio_common_test velox_dwio_common_test_utils velox_temp_path + velox_global_config velox_vector_test_lib Boost::regex velox_link_libs diff --git a/velox/dwio/common/tests/LoggedExceptionTest.cpp b/velox/dwio/common/tests/LoggedExceptionTest.cpp index 6eea7e8b5c78..1ad9ed21a6c6 100644 --- a/velox/dwio/common/tests/LoggedExceptionTest.cpp +++ b/velox/dwio/common/tests/LoggedExceptionTest.cpp @@ -15,6 +15,7 @@ */ #include #include +#include "velox/common/config/GlobalConfig.h" #include "velox/dwio/common/exception/Exception.h" using namespace facebook::velox::dwio::common::exception; @@ -24,13 +25,15 @@ namespace { void testTraceCollectionSwitchControl(bool enabled) { // Logged exception is system type of velox exception. // Disable rate control in the test. - FLAGS_velox_exception_user_stacktrace_rate_limit_ms = 0; - FLAGS_velox_exception_system_stacktrace_rate_limit_ms = 0; + config::globalConfig.exceptionUserStacktraceRateLimitMs = 0; + config::globalConfig.exceptionSystemStacktraceRateLimitMs = 0; - // NOTE: the user flag should not affect the tracing behavior of system type + // NOTE: the user config should not affect the tracing behavior of system type // of exception collection. - FLAGS_velox_exception_user_stacktrace_enabled = folly::Random::oneIn(2); - FLAGS_velox_exception_system_stacktrace_enabled = enabled ? true : false; + config::globalConfig.exceptionUserStacktraceEnabled = folly::Random::oneIn(2); + config::globalConfig.exceptionSystemStacktraceEnabled = + enabled ? true : false; + try { throw LoggedException("Test error message"); } catch (VeloxException& e) { diff --git a/velox/dwio/dwrf/test/CMakeLists.txt b/velox/dwio/dwrf/test/CMakeLists.txt index 3d1711c6c5da..305364c08dae 100644 --- a/velox/dwio/dwrf/test/CMakeLists.txt +++ b/velox/dwio/dwrf/test/CMakeLists.txt @@ -19,6 +19,7 @@ set(TEST_LINK_LIBS velox_dwio_dwrf_reader velox_dwio_dwrf_writer velox_row_fast + velox_flag_definitions gflags::gflags GTest::gtest GTest::gtest_main diff --git a/velox/dwio/dwrf/test/TestIntegerDictionaryEncoder.cpp b/velox/dwio/dwrf/test/TestIntegerDictionaryEncoder.cpp index 16ecca5f3663..83d38e6deb2a 100644 --- a/velox/dwio/dwrf/test/TestIntegerDictionaryEncoder.cpp +++ b/velox/dwio/dwrf/test/TestIntegerDictionaryEncoder.cpp @@ -17,6 +17,7 @@ #include #include #include "velox/dwio/dwrf/writer/IntegerDictionaryEncoder.h" +#include "velox/flag_definitions/flags.h" DECLARE_bool(velox_enable_memory_usage_track_in_default_memory_pool); @@ -29,7 +30,7 @@ class TestIntegerDictionaryEncoder : public ::testing::Test { protected: static void SetUpTestCase() { FLAGS_velox_enable_memory_usage_track_in_default_memory_pool = true; - config::translateFlagsToGlobalConfig(); + translateFlagsToGlobalConfig(); memory::MemoryManager::testingSetInstance({}); } }; diff --git a/velox/dwio/dwrf/test/TestStringDictionaryEncoder.cpp b/velox/dwio/dwrf/test/TestStringDictionaryEncoder.cpp index e547f7d16d9c..3ad8ddfc7361 100644 --- a/velox/dwio/dwrf/test/TestStringDictionaryEncoder.cpp +++ b/velox/dwio/dwrf/test/TestStringDictionaryEncoder.cpp @@ -17,6 +17,7 @@ #include #include #include "velox/dwio/dwrf/writer/StringDictionaryEncoder.h" +#include "velox/flag_definitions/flags.h" DECLARE_bool(velox_enable_memory_usage_track_in_default_memory_pool); @@ -28,7 +29,7 @@ class TestStringDictionaryEncoder : public ::testing::Test { protected: static void SetUpTestCase() { FLAGS_velox_enable_memory_usage_track_in_default_memory_pool = true; - config::translateFlagsToGlobalConfig(); + translateFlagsToGlobalConfig(); memory::MemoryManager::testingSetInstance({}); } }; diff --git a/velox/exec/tests/utils/CMakeLists.txt b/velox/exec/tests/utils/CMakeLists.txt index 0df50966d54b..a322a1d2ae97 100644 --- a/velox/exec/tests/utils/CMakeLists.txt +++ b/velox/exec/tests/utils/CMakeLists.txt @@ -53,6 +53,7 @@ target_link_libraries( velox_tpch_connector velox_presto_serializer velox_functions_prestosql + velox_flag_definitions velox_aggregates) if(${VELOX_BUILD_RUNNER}) diff --git a/velox/exec/tests/utils/OperatorTestBase.cpp b/velox/exec/tests/utils/OperatorTestBase.cpp index 33ce9c6603b9..14ede4c70bf8 100644 --- a/velox/exec/tests/utils/OperatorTestBase.cpp +++ b/velox/exec/tests/utils/OperatorTestBase.cpp @@ -22,6 +22,7 @@ #include "velox/common/memory/SharedArbitrator.h" #include "velox/common/testutil/TestValue.h" #include "velox/exec/tests/utils/LocalExchangeSource.h" +#include "velox/flag_definitions/flags.h" #include "velox/functions/prestosql/aggregates/RegisterAggregateFunctions.h" #include "velox/functions/prestosql/registration/RegistrationFunctions.h" #include "velox/parse/Expressions.h" @@ -63,7 +64,7 @@ OperatorTestBase::~OperatorTestBase() { void OperatorTestBase::SetUpTestCase() { FLAGS_velox_enable_memory_usage_track_in_default_memory_pool = true; FLAGS_velox_memory_leak_check_enabled = true; - config::translateFlagsToGlobalConfig(); + translateFlagsToGlobalConfig(); memory::SharedArbitrator::registerFactory(); resetMemory(); functions::prestosql::registerAllScalarFunctions(); diff --git a/velox/flag_definitions/CMakeLists.txt b/velox/flag_definitions/CMakeLists.txt index 8b551113d735..e008dfcef5a7 100644 --- a/velox/flag_definitions/CMakeLists.txt +++ b/velox/flag_definitions/CMakeLists.txt @@ -12,4 +12,5 @@ # See the License for the specific language governing permissions and # limitations under the License. velox_add_library(velox_flag_definitions flags.cpp) -velox_link_libraries(velox_flag_definitions PRIVATE gflags::gflags) +velox_link_libraries(velox_flag_definitions PRIVATE velox_global_config + gflags::gflags) diff --git a/velox/flag_definitions/flags.cpp b/velox/flag_definitions/flags.cpp index 17d5aae2dbc6..a92e63ebbb36 100644 --- a/velox/flag_definitions/flags.cpp +++ b/velox/flag_definitions/flags.cpp @@ -109,7 +109,7 @@ DEFINE_bool( "Whether allow to memory capacity transfer between memory pools from " "different tasks, which might happen in use case like Spark-Gluten"); -namespace facebook::velox::config { +namespace facebook::velox { void translateFlagsToGlobalConfig() { config::globalConfig.memoryNumSharedLeafPools = FLAGS_velox_memory_num_shared_leaf_pools; @@ -125,5 +125,13 @@ void translateFlagsToGlobalConfig() { FLAGS_velox_suppress_memory_capacity_exceeding_error_message; config::globalConfig.memoryPoolCapacityTransferAcrossTasks = FLAGS_velox_memory_pool_capacity_transfer_across_tasks; + config::globalConfig.exceptionSystemStacktraceEnabled = + FLAGS_velox_exception_system_stacktrace_enabled; + config::globalConfig.exceptionSystemStacktraceRateLimitMs = + FLAGS_velox_exception_system_stacktrace_rate_limit_ms; + config::globalConfig.exceptionUserStacktraceEnabled = + FLAGS_velox_exception_user_stacktrace_enabled; + config::globalConfig.exceptionUserStacktraceRateLimitMs = + FLAGS_velox_exception_user_stacktrace_rate_limit_ms; } -} // namespace facebook::velox::config +} // namespace facebook::velox diff --git a/velox/flag_definitions/flags.h b/velox/flag_definitions/flags.h new file mode 100644 index 000000000000..4b748602adae --- /dev/null +++ b/velox/flag_definitions/flags.h @@ -0,0 +1,19 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +namespace facebook::velox { +void translateFlagsToGlobalConfig(); +} From 4bc9d630325ce7835ad35588960a7f66d09a5243 Mon Sep 17 00:00:00 2001 From: Deepak Majeti Date: Mon, 9 Dec 2024 08:59:41 -0500 Subject: [PATCH 6/6] rebase with main --- velox/common/config/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/velox/common/config/CMakeLists.txt b/velox/common/config/CMakeLists.txt index c37336f68686..b4680557ce96 100644 --- a/velox/common/config/CMakeLists.txt +++ b/velox/common/config/CMakeLists.txt @@ -16,7 +16,7 @@ if(${VELOX_BUILD_TESTING}) add_subdirectory(tests) endif() -velox_add_library(velox_global_config OBJECT GlobalConfig.cpp) +velox_add_library(velox_global_config GlobalConfig.cpp) velox_add_library(velox_common_config Config.cpp) velox_link_libraries(