diff --git a/velox/buffer/tests/BufferTest.cpp b/velox/buffer/tests/BufferTest.cpp index 551b2158559..9be2d59c6f8 100644 --- a/velox/buffer/tests/BufferTest.cpp +++ b/velox/buffer/tests/BufferTest.cpp @@ -19,7 +19,6 @@ #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 @@ -28,7 +27,8 @@ DECLARE_bool(velox_enable_memory_usage_track_in_default_memory_pool); -namespace facebook::velox { +namespace facebook { +namespace velox { static_assert(Buffer::is_pod_like_v, ""); static_assert(Buffer::is_pod_like_v, ""); @@ -40,7 +40,6 @@ class BufferTest : public testing::Test { protected: static void SetUpTestCase() { FLAGS_velox_enable_memory_usage_track_in_default_memory_pool = true; - translateFlagsToGlobalConfig(); } void SetUp() override { @@ -491,4 +490,5 @@ TEST_F(BufferTest, sliceBooleanBuffer) { Buffer::slice(bufferPtr, 5, 6, nullptr), "Pool must not be null."); } -} // namespace facebook::velox +} // namespace velox +} // namespace facebook diff --git a/velox/buffer/tests/CMakeLists.txt b/velox/buffer/tests/CMakeLists.txt index 9e2bfaa4b9f..097f5ee3bb3 100644 --- a/velox/buffer/tests/CMakeLists.txt +++ b/velox/buffer/tests/CMakeLists.txt @@ -20,7 +20,6 @@ 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 68f1fa3c79b..a82f1a6734b 100644 --- a/velox/common/base/CMakeLists.txt +++ b/velox/common/base/CMakeLists.txt @@ -14,8 +14,14 @@ velox_add_library(velox_exception Exceptions.cpp VeloxException.cpp Exceptions.h) -velox_link_libraries(velox_exception PUBLIC velox_process Folly::folly fmt::fmt - glog::glog) +velox_link_libraries( + velox_exception + PUBLIC velox_flag_definitions + velox_process + Folly::folly + fmt::fmt + gflags::gflags + glog::glog) velox_add_library( velox_common_base diff --git a/velox/common/base/VeloxException.cpp b/velox/common/base/VeloxException.cpp index 254fcd1f005..82af2ba8a7d 100644 --- a/velox/common/base/VeloxException.cpp +++ b/velox/common/base/VeloxException.cpp @@ -15,7 +15,6 @@ */ #include "velox/common/base/VeloxException.h" -#include "velox/common/config/GlobalConfig.h" #include #include @@ -135,18 +134,16 @@ namespace { bool isStackTraceEnabled(VeloxException::Type type) { using namespace std::literals::chrono_literals; const bool isSysException = type == VeloxException::Type::kSystem; - if ((isSysException && - !config::globalConfig().exceptionSystemStacktraceEnabled) || - (!isSysException && - !config::globalConfig().exceptionUserStacktraceEnabled)) { + if ((isSysException && !FLAGS_velox_exception_system_stacktrace_enabled) || + (!isSysException && !FLAGS_velox_exception_user_stacktrace_enabled)) { // VeloxException stacktraces are disabled. return false; } const int32_t rateLimitMs = isSysException - ? config::globalConfig().exceptionSystemStacktraceRateLimitMs - : config::globalConfig().exceptionUserStacktraceRateLimitMs; - // not static so the global config can be manipulated at runtime + ? 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 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 c86b896c145..2e18952f47d 100644 --- a/velox/common/base/VeloxException.h +++ b/velox/common/base/VeloxException.h @@ -23,6 +23,7 @@ #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 3032eea49c5..7d85566bf4e 100644 --- a/velox/common/base/tests/CMakeLists.txt +++ b/velox/common/base/tests/CMakeLists.txt @@ -49,7 +49,6 @@ 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 4edcaa3b16e..9386b8cb672 100644 --- a/velox/common/base/tests/ExceptionTest.cpp +++ b/velox/common/base/tests/ExceptionTest.cpp @@ -19,8 +19,6 @@ #include #include "velox/common/base/Exceptions.h" -#include "velox/common/config/GlobalConfig.h" -#include "velox/flag_definitions/flags.h" using namespace facebook::velox; @@ -85,7 +83,6 @@ 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( @@ -112,8 +109,8 @@ void testExceptionTraceCollectionControl(bool userException, bool enabled) { SCOPED_TRACE(fmt::format( "enabled: {}, user flag: {}, sys flag: {}", enabled, - config::globalConfig().exceptionUserStacktraceEnabled, - config::globalConfig().exceptionSystemStacktraceEnabled)); + FLAGS_velox_exception_user_stacktrace_enabled, + FLAGS_velox_exception_system_stacktrace_enabled)); ASSERT_EQ(userException, e.exceptionType() == VeloxException::Type::kUser); ASSERT_EQ(enabled, e.stackTrace() != nullptr); } @@ -126,7 +123,6 @@ 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; @@ -150,7 +146,6 @@ void testExceptionTraceCollectionRateControl( FLAGS_velox_exception_user_stacktrace_rate_limit_ms = folly::Random::rand32(); } - translateFlagsToGlobalConfig(); for (int iter = 0; iter < 3; ++iter) { try { if (userException) { @@ -179,8 +174,8 @@ void testExceptionTraceCollectionRateControl( "userException: {}, hasRateLimit: {}, user limit: {}ms, sys limit: {}ms", userException, hasRateLimit, - config::globalConfig().exceptionUserStacktraceRateLimitMs, - config::globalConfig().exceptionSystemStacktraceRateLimitMs)); + FLAGS_velox_exception_user_stacktrace_rate_limit_ms, + FLAGS_velox_exception_system_stacktrace_rate_limit_ms)); ASSERT_EQ( userException, e.exceptionType() == VeloxException::Type::kUser); ASSERT_EQ(!hasRateLimit || ((iter % 2) == 0), e.stackTrace() != nullptr); diff --git a/velox/common/caching/SsdFile.cpp b/velox/common/caching/SsdFile.cpp index f0e416acd79..9db2c5c4f08 100644 --- a/velox/common/caching/SsdFile.cpp +++ b/velox/common/caching/SsdFile.cpp @@ -22,7 +22,6 @@ #include "velox/common/base/SuccinctPrinter.h" #include "velox/common/caching/FileIds.h" #include "velox/common/caching/SsdCache.h" -#include "velox/common/config/GlobalConfig.h" #include "velox/common/memory/Memory.h" #include "velox/common/process/TraceContext.h" @@ -35,6 +34,9 @@ #include #include +DECLARE_bool(velox_ssd_odirect); +DECLARE_bool(velox_ssd_verify_write); + namespace facebook::velox::cache { namespace { @@ -116,7 +118,7 @@ SsdFile::SsdFile(const Config& config) process::TraceContext trace("SsdFile::SsdFile"); filesystems::FileOptions fileOptions; fileOptions.shouldThrowOnFileAlreadyExists = false; - fileOptions.bufferIo = !config::globalConfig().useSsdODirect; + fileOptions.bufferIo = !FLAGS_velox_ssd_odirect; writeFile_ = fs_->openFileForWrite(fileName_, fileOptions); readFile_ = fs_->openFileForRead(fileName_, fileOptions); @@ -415,7 +417,7 @@ void SsdFile::write(std::vector& pins) { checksum = checksumEntry(*entry); } entries_[std::move(key)] = SsdRun(offset, size, checksum); - if (config::globalConfig().verifySsdWrite) { + if (FLAGS_velox_ssd_verify_write) { verifyWrite(*entry, SsdRun(offset, size, checksum)); } offset += size; diff --git a/velox/common/caching/tests/AsyncDataCacheTest.cpp b/velox/common/caching/tests/AsyncDataCacheTest.cpp index 98a556fda09..ad781feb723 100644 --- a/velox/common/caching/tests/AsyncDataCacheTest.cpp +++ b/velox/common/caching/tests/AsyncDataCacheTest.cpp @@ -27,7 +27,6 @@ #include "velox/common/testutil/ScopedTestTime.h" #include "velox/common/testutil/TestValue.h" #include "velox/exec/tests/utils/TempDirectoryPath.h" -#include "velox/flag_definitions/flags.h" #include #include @@ -43,6 +42,9 @@ using namespace facebook::velox::common::testutil; using facebook::velox::memory::MemoryAllocator; +DECLARE_bool(velox_ssd_odirect); +DECLARE_bool(velox_ssd_verify_write); + // Represents a planned load from a file. Many of these constitute a load plan. struct Request { Request(uint64_t _offset, uint64_t _size) : offset(_offset), size(_size) {} @@ -138,7 +140,6 @@ class AsyncDataCacheTest : public ::testing::TestWithParam { if (ssdBytes > 0) { // tmpfs does not support O_DIRECT, so turn this off for testing. FLAGS_velox_ssd_odirect = false; - translateFlagsToGlobalConfig(); // Make a new tempDirectory only if one is not already set. The // second creation of cache must find the checkpoint of the // previous one. @@ -720,7 +721,7 @@ TEST_P(AsyncDataCacheTest, pin) { TEST_P(AsyncDataCacheTest, replace) { constexpr int64_t kMaxBytes = 64 << 20; - config::globalConfig().exceptionUserStacktraceEnabled = false; + FLAGS_velox_exception_user_stacktrace_enabled = false; initializeCache(kMaxBytes); // Load 10x the max size, inject an error every 21 batches. loadLoop(0, kMaxBytes * 10, 21); @@ -738,7 +739,7 @@ TEST_P(AsyncDataCacheTest, replace) { TEST_P(AsyncDataCacheTest, evictAccounting) { constexpr int64_t kMaxBytes = 64 << 20; - config::globalConfig().exceptionUserStacktraceEnabled = false; + FLAGS_velox_exception_user_stacktrace_enabled = false; initializeCache(kMaxBytes); auto pool = manager_->addLeafPool("test"); @@ -762,7 +763,7 @@ TEST_P(AsyncDataCacheTest, evictAccounting) { TEST_P(AsyncDataCacheTest, largeEvict) { constexpr int64_t kMaxBytes = 256 << 20; constexpr int32_t kNumThreads = 24; - config::globalConfig().exceptionUserStacktraceEnabled = false; + FLAGS_velox_exception_user_stacktrace_enabled = false; initializeCache(kMaxBytes); // Load 10x the max size, inject an allocation of 1/8 the capacity every 4 // batches. @@ -841,7 +842,7 @@ TEST_P(AsyncDataCacheTest, DISABLED_ssd) { constexpr uint64_t kRamBytes = 32 << 20; constexpr uint64_t kSsdBytes = 512UL << 20; #endif - config::globalConfig().exceptionUserStacktraceEnabled = false; + FLAGS_velox_exception_user_stacktrace_enabled = false; initializeCache(kRamBytes, kSsdBytes); cache_->setVerifyHook( [&](const AsyncDataCacheEntry& entry) { checkContents(entry); }); @@ -849,7 +850,6 @@ TEST_P(AsyncDataCacheTest, DISABLED_ssd) { // Read back all writes. This increases the chance of writes falling behind // new entry creation. FLAGS_velox_ssd_verify_write = true; - translateFlagsToGlobalConfig(); // We read kSsdBytes worth of data on 16 threads. The same data will be hit by // all threads. The expectation is that most of the data ends up on SSD. All @@ -865,7 +865,7 @@ TEST_P(AsyncDataCacheTest, DISABLED_ssd) { // We allow writes to proceed faster. FLAGS_velox_ssd_verify_write = false; - translateFlagsToGlobalConfig(); + // We read the data back. The verify hook checks correct values. Error every // 13 batch loads. runThreads(16, [&](int32_t /*i*/) { loadLoop(0, kSsdBytes, 13); }); diff --git a/velox/common/caching/tests/SsdFileTest.cpp b/velox/common/caching/tests/SsdFileTest.cpp index 857ada27b8e..bdb8fbdcda9 100644 --- a/velox/common/caching/tests/SsdFileTest.cpp +++ b/velox/common/caching/tests/SsdFileTest.cpp @@ -17,7 +17,6 @@ #include "velox/common/base/tests/GTestUtils.h" #include "velox/common/caching/FileIds.h" #include "velox/common/caching/tests/CacheTestUtil.h" -#include "velox/common/config/GlobalConfig.h" #include "velox/common/file/FileSystems.h" #include "velox/common/file/tests/FaultyFileSystem.h" #include "velox/common/memory/Memory.h" @@ -36,6 +35,9 @@ using namespace facebook::velox::tests::utils; using facebook::velox::memory::MemoryAllocator; +DECLARE_bool(velox_ssd_odirect); +DECLARE_bool(velox_ssd_verify_write); + // Represents an entry written to SSD. struct TestEntry { FileCacheKey key; @@ -74,7 +76,7 @@ class SsdFileTest : public testing::Test { bool disableFileCow = false, bool enableFaultInjection = false) { // tmpfs does not support O_DIRECT, so turn this off for testing. - config::globalConfig().useSsdODirect = false; + FLAGS_velox_ssd_odirect = false; cache_ = AsyncDataCache::create(memory::memoryManager()->allocator()); cacheHelper_ = std::make_unique(cache_.get()); @@ -328,7 +330,7 @@ TEST_F(SsdFileTest, writeAndRead) { constexpr int64_t kSsdSize = 16 * SsdFile::kRegionSize; std::vector allEntries; initializeCache(kSsdSize); - config::globalConfig().verifySsdWrite = true; + FLAGS_velox_ssd_verify_write = true; for (auto startOffset = 0; startOffset <= kSsdSize - SsdFile::kRegionSize; startOffset += SsdFile::kRegionSize) { auto pins = @@ -403,7 +405,7 @@ TEST_F(SsdFileTest, checkpoint) { constexpr int64_t kSsdSize = 16 * SsdFile::kRegionSize; const uint64_t checkpointIntervalBytes = 5 * SsdFile::kRegionSize; const auto fileNameAlt = StringIdLease(fileIds(), "fileInStorageAlt"); - config::globalConfig().verifySsdWrite = true; + FLAGS_velox_ssd_verify_write = true; initializeCache(kSsdSize, checkpointIntervalBytes); std::vector allEntries; @@ -499,7 +501,7 @@ TEST_F(SsdFileTest, checkpoint) { TEST_F(SsdFileTest, fileCorruption) { constexpr int64_t kSsdSize = 16 * SsdFile::kRegionSize; const uint64_t checkpointIntervalBytes = 5 * SsdFile::kRegionSize; - config::globalConfig().verifySsdWrite = true; + FLAGS_velox_ssd_verify_write = true; const auto populateCache = [&](std::vector& entries) { entries.clear(); @@ -571,7 +573,7 @@ TEST_F(SsdFileTest, fileCorruption) { TEST_F(SsdFileTest, recoverFromCheckpointWithChecksum) { constexpr int64_t kSsdSize = 4 * SsdFile::kRegionSize; const uint64_t checkpointIntervalBytes = 3 * SsdFile::kRegionSize; - config::globalConfig().verifySsdWrite = true; + FLAGS_velox_ssd_verify_write = true; // Test if cache data can be recovered with different settings. struct { diff --git a/velox/common/config/CMakeLists.txt b/velox/common/config/CMakeLists.txt index b4680557ce9..7780665a292 100644 --- a/velox/common/config/CMakeLists.txt +++ b/velox/common/config/CMakeLists.txt @@ -12,14 +12,13 @@ # 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() - -velox_add_library(velox_global_config GlobalConfig.cpp) +endif () velox_add_library(velox_common_config Config.cpp) velox_link_libraries( velox_common_config - PUBLIC velox_common_base velox_global_config velox_exception + PUBLIC velox_common_base + velox_exception PRIVATE re2::re2) diff --git a/velox/common/config/GlobalConfig.cpp b/velox/common/config/GlobalConfig.cpp deleted file mode 100644 index 4379492df9f..00000000000 --- a/velox/common/config/GlobalConfig.cpp +++ /dev/null @@ -1,26 +0,0 @@ -/* - * 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() { - static GlobalConfiguration config; - return config; -} - -} // namespace facebook::velox::config diff --git a/velox/common/config/GlobalConfig.h b/velox/common/config/GlobalConfig.h deleted file mode 100644 index f0c3946ac4d..00000000000 --- a/velox/common/config/GlobalConfig.h +++ /dev/null @@ -1,92 +0,0 @@ -/* - * 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 -#include - -namespace facebook::velox::config { - -struct GlobalConfiguration { - /// Number of shared leaf memory pools per process. - int32_t memoryNumSharedLeafPools{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}; - /// 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}; - /// Whether to overwrite queryCtx and force the use of simplified expression - /// evaluation path. - bool forceEvalSimplified{false}; - /// This is an experimental flag only to be used for debugging purposes. If - /// set to true, serializes the input vector data and all the SQL expressions - /// in the ExprSet that is currently executing, whenever a fatal signal is - /// encountered. Enabling this flag makes the signal handler async signal - /// unsafe, so it should only be used for debugging purposes. The vector and - /// SQLs are serialized to files in directories specified by either - /// 'saveInputOnExpressionAnyFailurePath' or - /// 'saveInputOnExpressionSystemFailurePath' - bool experimentalSaveInputOnFatalSignal{false}; - /// Used to enable saving input vector and expression SQL on disk in case - /// of any (user or system) error during expression evaluation. The value - /// specifies a path to a directory where the vectors will be saved. That - /// directory must exist and be writable. - std::string saveInputOnExpressionAnyFailurePath; - /// Used to enable saving input vector and expression SQL on disk in case - /// of a system error during expression evaluation. The value specifies a path - /// to a directory where the vectors will be saved. That directory must exist - /// and be writable. This flag is ignored if - /// saveInputOnExpressionAnyFailurePath flag is set. - std::string saveInputOnExpressionSystemFailurePath; - /// Use O_DIRECT for SSD cache I/O. This allows to bypass Linux Kernel's page - /// cache and can improve performance on some filesystems. Disable if the - /// filesystem does not support it. - bool useSsdODirect{true}; - /// Verify the data written to SSD. Once an entry is written, it is - /// immediately read back and is compared against the entry written. This is - /// helpful to protect against SSD write corruption. - bool verifySsdWrite{false}; -}; - -GlobalConfiguration& globalConfig(); - -} // namespace facebook::velox::config diff --git a/velox/common/memory/CMakeLists.txt b/velox/common/memory/CMakeLists.txt index ecd605ace64..79a707fac30 100644 --- a/velox/common/memory/CMakeLists.txt +++ b/velox/common/memory/CMakeLists.txt @@ -38,9 +38,11 @@ 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 46de0d573ea..ff44791763a 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 (FLAGS_velox_memory_leak_check_enabled) { VELOX_CHECK( ((allocatedBytes_ - reservations_.read()) == 0) && (numAllocated_ == 0) && (numMapped_ == 0), diff --git a/velox/common/memory/MallocAllocator.h b/velox/common/memory/MallocAllocator.h index 5600c0a2578..ade9536f0fb 100644 --- a/velox/common/memory/MallocAllocator.h +++ b/velox/common/memory/MallocAllocator.h @@ -20,6 +20,8 @@ #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/Memory.cpp b/velox/common/memory/Memory.cpp index 85c0376fc47..cd29de95eeb 100644 --- a/velox/common/memory/Memory.cpp +++ b/velox/common/memory/Memory.cpp @@ -20,10 +20,11 @@ #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__"}; @@ -78,7 +79,7 @@ std::vector> createSharedLeafMemoryPools( VELOX_CHECK_EQ(sysPool.name(), kSysRootName); std::vector> leafPools; const size_t numSharedPools = - std::max(1, config::globalConfig().memoryNumSharedLeafPools); + std::max(1, FLAGS_velox_memory_num_shared_leaf_pools); leafPools.reserve(numSharedPools); for (size_t i = 0; i < numSharedPools; ++i) { leafPools.emplace_back( @@ -129,7 +130,7 @@ MemoryManager::MemoryManager(const MemoryManagerOptions& options) sysRoot_->name()); VELOX_CHECK_EQ( sharedLeafPools_.size(), - std::max(1, config::globalConfig().memoryNumSharedLeafPools)); + std::max(1, FLAGS_velox_memory_num_shared_leaf_pools)); } MemoryManager::~MemoryManager() { diff --git a/velox/common/memory/Memory.h b/velox/common/memory/Memory.h index ce63a444eb3..2aa3e5d93f5 100644 --- a/velox/common/memory/Memory.h +++ b/velox/common/memory/Memory.h @@ -25,6 +25,7 @@ #include #include +#include #include #include @@ -35,11 +36,14 @@ #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 @@ -61,18 +65,18 @@ struct MemoryManagerOptions { /// If true, enable memory usage tracking in the default memory pool. bool trackDefaultUsage{ - config::globalConfig().enableMemoryUsageTrackInDefaultMemoryPool}; + FLAGS_velox_enable_memory_usage_track_in_default_memory_pool}; /// 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{FLAGS_velox_memory_leak_check_enabled}; /// 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{FLAGS_velox_memory_pool_debug_enabled}; /// 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 b8180ff9c63..574009d8b1f 100644 --- a/velox/common/memory/MemoryAllocator.cpp +++ b/velox/common/memory/MemoryAllocator.cpp @@ -25,6 +25,8 @@ #include "velox/common/base/BitUtil.h" #include "velox/common/memory/Memory.h" +DECLARE_bool(velox_memory_use_hugepages); + namespace facebook::velox::memory { // static @@ -413,7 +415,7 @@ void MemoryAllocator::useHugePages( const ContiguousAllocation& data, bool enable) { #ifdef linux - if (!config::globalConfig().memoryUseHugepages) { + if (!FLAGS_velox_memory_use_hugepages) { return; } auto maybeRange = data.hugePageRange(); diff --git a/velox/common/memory/MemoryAllocator.h b/velox/common/memory/MemoryAllocator.h index fe574b305ea..4a7ac706c71 100644 --- a/velox/common/memory/MemoryAllocator.h +++ b/velox/common/memory/MemoryAllocator.h @@ -24,12 +24,14 @@ #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 { @@ -94,7 +96,7 @@ struct Stats { template void recordAllocate(int64_t bytes, int32_t count, Op op) { - if (config::globalConfig().timeAllocations) { + if (FLAGS_velox_time_allocations) { auto index = sizeIndex(bytes); velox::ClockTimer timer(sizes[index].allocateClocks); op(); @@ -107,7 +109,7 @@ struct Stats { template void recordFree(int64_t bytes, Op op) { - if (config::globalConfig().timeAllocations) { + if (FLAGS_velox_time_allocations) { 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 10c74de8c01..dbd67dd5a57 100644 --- a/velox/common/memory/MemoryPool.cpp +++ b/velox/common/memory/MemoryPool.cpp @@ -27,6 +27,13 @@ #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 { @@ -964,7 +971,7 @@ std::string MemoryPoolImpl::treeMemoryUsage(bool skipEmptyPool) const { if (parent_ != nullptr) { return parent_->treeMemoryUsage(skipEmptyPool); } - if (config::globalConfig().suppressMemoryCapacityExceedingErrorMessage) { + if (FLAGS_velox_suppress_memory_capacity_exceeding_error_message) { return ""; } std::stringstream out; diff --git a/velox/common/memory/MemoryPool.h b/velox/common/memory/MemoryPool.h index f95e44f30a8..3a84037d9e3 100644 --- a/velox/common/memory/MemoryPool.h +++ b/velox/common/memory/MemoryPool.h @@ -24,11 +24,14 @@ #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" +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; } @@ -151,7 +154,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{FLAGS_velox_memory_pool_debug_enabled}; /// 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 6060b4cd7b7..8ab440e2125 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) && FLAGS_velox_time_allocations) { // 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/CMakeLists.txt b/velox/common/memory/tests/CMakeLists.txt index 6d2ca66b956..3a35c9060b3 100644 --- a/velox/common/memory/tests/CMakeLists.txt +++ b/velox/common/memory/tests/CMakeLists.txt @@ -38,7 +38,6 @@ 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 8091f5e4960..f0a66d0768e 100644 --- a/velox/common/memory/tests/MemoryAllocatorTest.cpp +++ b/velox/common/memory/tests/MemoryAllocatorTest.cpp @@ -22,7 +22,6 @@ #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 @@ -36,7 +35,6 @@ #endif // linux DECLARE_bool(velox_memory_leak_check_enabled); -DECLARE_bool(velox_time_allocations); using namespace facebook::velox::common::testutil; @@ -58,7 +56,6 @@ class MemoryAllocatorTest : public testing::TestWithParam { static void SetUpTestCase() { TestValue::enable(); FLAGS_velox_memory_leak_check_enabled = true; - translateFlagsToGlobalConfig(); } void SetUp() override { @@ -652,8 +649,8 @@ TEST_P(MemoryAllocatorTest, stats) { ASSERT_EQ(stats.sizes[i].numAllocations, 0); } + gflags::FlagSaver flagSaver; FLAGS_velox_time_allocations = true; - translateFlagsToGlobalConfig(); for (auto i = 0; i < sizes.size(); ++i) { std::unique_ptr allocation = std::make_unique(); auto size = sizes[i]; @@ -665,17 +662,14 @@ 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; - translateFlagsToGlobalConfig(); } TEST_P(MemoryAllocatorTest, singleAllocation) { if (!useMmap_ && enableReservation_) { return; } - + gflags::FlagSaver flagSaver; FLAGS_velox_time_allocations = true; - translateFlagsToGlobalConfig(); const std::vector& sizes = instance_->sizeClasses(); MachinePageCount capacity = kCapacityPages; for (auto i = 0; i < sizes.size(); ++i) { @@ -722,8 +716,6 @@ TEST_P(MemoryAllocatorTest, singleAllocation) { } ASSERT_TRUE(instance_->checkConsistency()); } - FLAGS_velox_time_allocations = false; - translateFlagsToGlobalConfig(); } TEST_P(MemoryAllocatorTest, increasingSize) { @@ -1618,11 +1610,6 @@ 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 @@ -1952,6 +1939,11 @@ TEST_P(MemoryAllocatorTest, unmap) { } } +VELOX_INSTANTIATE_TEST_SUITE_P( + MemoryAllocatorTestSuite, + MemoryAllocatorTest, + testing::ValuesIn({0, 1, 2})); + class MmapConfigTest : public testing::Test { public: protected: @@ -1998,4 +1990,5 @@ 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 0518f6da49c..ef174b8a765 100644 --- a/velox/common/memory/tests/MemoryCapExceededTest.cpp +++ b/velox/common/memory/tests/MemoryCapExceededTest.cpp @@ -19,7 +19,6 @@ #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 @@ -35,13 +34,11 @@ 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(); - translateFlagsToGlobalConfig(); } void TearDown() override { OperatorTestBase::TearDown(); FLAGS_velox_suppress_memory_capacity_exceeding_error_message = false; - translateFlagsToGlobalConfig(); } }; diff --git a/velox/common/memory/tests/MemoryManagerTest.cpp b/velox/common/memory/tests/MemoryManagerTest.cpp index bd2d526b3e1..dbfe8475fea 100644 --- a/velox/common/memory/tests/MemoryManagerTest.cpp +++ b/velox/common/memory/tests/MemoryManagerTest.cpp @@ -24,7 +24,6 @@ #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); @@ -45,7 +44,6 @@ class MemoryManagerTest : public testing::Test { protected: static void SetUpTestCase() { SharedArbitrator::registerFactory(); - translateFlagsToGlobalConfig(); } inline static const std::string arbitratorKind_{"SHARED"}; @@ -372,7 +370,6 @@ TEST_F(MemoryManagerTest, defaultMemoryUsageTracking) { for (bool trackDefaultMemoryUsage : {false, true}) { FLAGS_velox_enable_memory_usage_track_in_default_memory_pool = trackDefaultMemoryUsage; - 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 fc819632830..ac6d8ad7d5f 100644 --- a/velox/dwio/common/tests/CMakeLists.txt +++ b/velox/dwio/common/tests/CMakeLists.txt @@ -62,7 +62,6 @@ 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 c1634683565..6eea7e8b5c7 100644 --- a/velox/dwio/common/tests/LoggedExceptionTest.cpp +++ b/velox/dwio/common/tests/LoggedExceptionTest.cpp @@ -15,7 +15,6 @@ */ #include #include -#include "velox/common/config/GlobalConfig.h" #include "velox/dwio/common/exception/Exception.h" using namespace facebook::velox::dwio::common::exception; @@ -25,24 +24,21 @@ namespace { void testTraceCollectionSwitchControl(bool enabled) { // Logged exception is system type of velox exception. // Disable rate control in the test. - config::globalConfig().exceptionUserStacktraceRateLimitMs = 0; - config::globalConfig().exceptionSystemStacktraceRateLimitMs = 0; + FLAGS_velox_exception_user_stacktrace_rate_limit_ms = 0; + FLAGS_velox_exception_system_stacktrace_rate_limit_ms = 0; - // NOTE: the user config should not affect the tracing behavior of system type + // NOTE: the user flag should not affect the tracing behavior of system type // of exception collection. - config::globalConfig().exceptionUserStacktraceEnabled = - folly::Random::oneIn(2); - config::globalConfig().exceptionSystemStacktraceEnabled = - enabled ? true : false; - + FLAGS_velox_exception_user_stacktrace_enabled = folly::Random::oneIn(2); + FLAGS_velox_exception_system_stacktrace_enabled = enabled ? true : false; try { throw LoggedException("Test error message"); } catch (VeloxException& e) { SCOPED_TRACE(fmt::format( "enabled: {}, user flag: {}, sys flag: {}", enabled, - config::globalConfig().exceptionUserStacktraceEnabled, - config::globalConfig().exceptionSystemStacktraceEnabled)); + FLAGS_velox_exception_user_stacktrace_enabled, + FLAGS_velox_exception_system_stacktrace_enabled)); ASSERT_TRUE(e.exceptionType() == VeloxException::Type::kSystem); ASSERT_EQ(enabled, e.stackTrace() != nullptr); } diff --git a/velox/dwio/dwrf/test/CMakeLists.txt b/velox/dwio/dwrf/test/CMakeLists.txt index e78c0b0b78f..40bd1622684 100644 --- a/velox/dwio/dwrf/test/CMakeLists.txt +++ b/velox/dwio/dwrf/test/CMakeLists.txt @@ -19,7 +19,6 @@ set(TEST_LINK_LIBS velox_dwio_dwrf_reader velox_dwio_dwrf_writer velox_row_fast - velox_flag_definitions GTest::gtest GTest::gtest_main GTest::gmock diff --git a/velox/dwio/dwrf/test/CacheInputTest.cpp b/velox/dwio/dwrf/test/CacheInputTest.cpp index 41899aa882d..b24bf6152e6 100644 --- a/velox/dwio/dwrf/test/CacheInputTest.cpp +++ b/velox/dwio/dwrf/test/CacheInputTest.cpp @@ -20,7 +20,6 @@ #include "velox/common/base/tests/GTestUtils.h" #include "velox/common/caching/FileIds.h" #include "velox/common/caching/tests/CacheTestUtil.h" -#include "velox/common/config/GlobalConfig.h" #include "velox/common/file/FileSystems.h" #include "velox/common/io/IoStatistics.h" #include "velox/common/io/Options.h" @@ -41,6 +40,7 @@ using facebook::velox::common::Region; using memory::MemoryAllocator; using IoStatisticsPtr = std::shared_ptr; +DECLARE_bool(velox_ssd_odirect); class CacheTest : public ::testing::Test { protected: @@ -98,7 +98,7 @@ class CacheTest : public ::testing::Test { std::unique_ptr ssd; if (ssdBytes > 0) { - config::globalConfig().useSsdODirect = false; + FLAGS_velox_ssd_odirect = false; tempDirectory_ = exec::test::TempDirectoryPath::create(); const SsdCache::Config config( fmt::format("{}/cache", tempDirectory_->getPath()), diff --git a/velox/dwio/dwrf/test/TestIntegerDictionaryEncoder.cpp b/velox/dwio/dwrf/test/TestIntegerDictionaryEncoder.cpp index 8ca887c9924..bfb4177dc84 100644 --- a/velox/dwio/dwrf/test/TestIntegerDictionaryEncoder.cpp +++ b/velox/dwio/dwrf/test/TestIntegerDictionaryEncoder.cpp @@ -17,7 +17,6 @@ #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); @@ -30,7 +29,6 @@ class TestIntegerDictionaryEncoder : public ::testing::Test { protected: static void SetUpTestCase() { FLAGS_velox_enable_memory_usage_track_in_default_memory_pool = true; - translateFlagsToGlobalConfig(); memory::MemoryManager::testingSetInstance({}); } }; diff --git a/velox/dwio/dwrf/test/TestStringDictionaryEncoder.cpp b/velox/dwio/dwrf/test/TestStringDictionaryEncoder.cpp index 3ad8ddfc736..f6dd8d848f0 100644 --- a/velox/dwio/dwrf/test/TestStringDictionaryEncoder.cpp +++ b/velox/dwio/dwrf/test/TestStringDictionaryEncoder.cpp @@ -17,7 +17,6 @@ #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); @@ -29,7 +28,6 @@ class TestStringDictionaryEncoder : public ::testing::Test { protected: static void SetUpTestCase() { FLAGS_velox_enable_memory_usage_track_in_default_memory_pool = true; - translateFlagsToGlobalConfig(); memory::MemoryManager::testingSetInstance({}); } }; diff --git a/velox/exec/Operator.cpp b/velox/exec/Operator.cpp index 35c13c5e34e..5ee37bd2981 100644 --- a/velox/exec/Operator.cpp +++ b/velox/exec/Operator.cpp @@ -17,7 +17,6 @@ #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" @@ -650,7 +649,7 @@ void Operator::MemoryReclaimer::enterArbitration() { } Driver* const runningDriver = driverThreadCtx->driverCtx()->driver; - if (!config::globalConfig().memoryPoolCapacityTransferAcrossTasks) { + if (!FLAGS_velox_memory_pool_capacity_transfer_across_tasks) { 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 +679,7 @@ void Operator::MemoryReclaimer::leaveArbitration() noexcept { return; } Driver* const runningDriver = driverThreadCtx->driverCtx()->driver; - if (!config::globalConfig().memoryPoolCapacityTransferAcrossTasks) { + if (!FLAGS_velox_memory_pool_capacity_transfer_across_tasks) { 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 30327b74ff2..1f436299200 100644 --- a/velox/exec/fuzzer/FuzzerUtil.cpp +++ b/velox/exec/fuzzer/FuzzerUtil.cpp @@ -16,7 +16,6 @@ #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" @@ -345,8 +344,8 @@ void setupMemory( int64_t allocatorCapacity, int64_t arbitratorCapacity, bool enableGlobalArbitration) { - config::globalConfig().enableMemoryUsageTrackInDefaultMemoryPool = true; - config::globalConfig().memoryLeakCheckEnabled = true; + FLAGS_velox_enable_memory_usage_track_in_default_memory_pool = true; + FLAGS_velox_memory_leak_check_enabled = true; facebook::velox::memory::SharedArbitrator::registerFactory(); facebook::velox::memory::MemoryManagerOptions options; options.allocatorCapacity = allocatorCapacity; diff --git a/velox/exec/tests/utils/CMakeLists.txt b/velox/exec/tests/utils/CMakeLists.txt index c1d1f83129f..6539c155e55 100644 --- a/velox/exec/tests/utils/CMakeLists.txt +++ b/velox/exec/tests/utils/CMakeLists.txt @@ -54,7 +54,6 @@ 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 14ede4c70bf..69300dafb0b 100644 --- a/velox/exec/tests/utils/OperatorTestBase.cpp +++ b/velox/exec/tests/utils/OperatorTestBase.cpp @@ -22,7 +22,6 @@ #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" @@ -64,7 +63,6 @@ OperatorTestBase::~OperatorTestBase() { void OperatorTestBase::SetUpTestCase() { FLAGS_velox_enable_memory_usage_track_in_default_memory_pool = true; FLAGS_velox_memory_leak_check_enabled = true; - translateFlagsToGlobalConfig(); memory::SharedArbitrator::registerFactory(); resetMemory(); functions::prestosql::registerAllScalarFunctions(); diff --git a/velox/expression/Expr.cpp b/velox/expression/Expr.cpp index 779da36937d..81b0fa59d3f 100644 --- a/velox/expression/Expr.cpp +++ b/velox/expression/Expr.cpp @@ -21,7 +21,6 @@ #include "velox/common/base/Exceptions.h" #include "velox/common/base/Fs.h" #include "velox/common/base/SuccinctPrinter.h" -#include "velox/common/config/GlobalConfig.h" #include "velox/common/process/ThreadDebugInfo.h" #include "velox/common/testutil/TestValue.h" #include "velox/core/Expressions.h" @@ -36,8 +35,12 @@ #include "velox/vector/SelectivityVector.h" #include "velox/vector/VectorSaver.h" -namespace facebook::velox::exec { +DECLARE_string(velox_save_input_on_expression_any_failure_path); +DECLARE_string(velox_save_input_on_expression_system_failure_path); +DECLARE_bool(force_eval_simplified); +DECLARE_bool(velox_experimental_save_input_on_fatal_signal); +namespace facebook::velox::exec { folly::Synchronized>>& exprSetListeners() { static folly::Synchronized>> @@ -218,8 +221,8 @@ void Expr::computeMetadata() { } // (1) Compute deterministic_. - // An expression is deterministic if it is a deterministic function call or a - // special form, and all its inputs are also deterministic. + // An expression is deterministic if it is a deterministic function call or + // a special form, and all its inputs are also deterministic. if (vectorFunction_) { deterministic_ = vectorFunctionMetadata_.deterministic; } else { @@ -248,9 +251,9 @@ void Expr::computeMetadata() { // cast, constant and fieldReference expressions act as vector functions // with default null behavior. - // If the function has default null behavior, the Expr propagates nulls if - // the set of fields null-propagating arguments depend on is a superset of - // the fields non null-propagating arguments depend on. + // If the function has default null behavior, the Expr propagates nulls + // if the set of fields null-propagating arguments depend on is a + // superset of the fields non null-propagating arguments depend on. std::unordered_set nullPropagating, nonNullPropagating; for (auto& input : inputs_) { if (input->propagatesNulls_) { @@ -372,8 +375,8 @@ bool Expr::evalArgsDefaultNulls( auto newErrors = context.errors(); assert(newErrors); // lint if (flatNulls) { - // There are both nulls and errors. Only a null with no error removes - // a row. + // There are both nulls and errors. Only a null with no error + // removes a row. auto errorNulls = newErrors->errorFlags(); auto rowBits = rows.mutableRows().asMutableRange().bits(); auto nwords = bits::nwords(rows.rows().end()); @@ -532,8 +535,8 @@ class ExprExceptionContext { /// Persist data and sql on disk. Data will be persisted in $basePath/vector /// and sql will be persisted in $basePath/sql void persistDataAndSql(const char* basePath) { - // Exception already persisted or failed to persist. We don't persist again - // in this situation. + // Exception already persisted or failed to persist. We don't persist + // again in this situation. if (!dataPath_.empty()) { return; } @@ -634,8 +637,8 @@ class ExprExceptionContext { std::string sqlPath_{""}; /// Path of the file storing the SQL for all expressions in the ExprSet that - /// was executing this expression. Useful if the bug that caused the error was - /// encountered due to some mutation from running the other expressions. + /// was executing this expression. Useful if the bug that caused the error + /// was encountered due to some mutation from running the other expressions. std::string allExprSqlPath_{"N/A"}; }; @@ -660,10 +663,9 @@ std::string onTopLevelException(VeloxException::Type exceptionType, void* arg) { auto* context = static_cast(arg); const char* basePath = - config::globalConfig().saveInputOnExpressionAnyFailurePath.c_str(); + FLAGS_velox_save_input_on_expression_any_failure_path.c_str(); if (strlen(basePath) == 0 && exceptionType == VeloxException::Type::kSystem) { - basePath = - config::globalConfig().saveInputOnExpressionSystemFailurePath.c_str(); + basePath = FLAGS_velox_save_input_on_expression_system_failure_path.c_str(); } if (strlen(basePath) == 0) { return fmt::format("Top-level Expression: {}", context->expr()->toString()); @@ -734,8 +736,8 @@ void Expr::evalFlatNoNullsImpl( inputValues_.resize(inputs_.size()); for (int32_t i = 0; i < inputs_.size(); ++i) { if (constantInputs_[i]) { - // No need to re-evaluate constant expression. Simply move constant values - // from constantInputs_. + // No need to re-evaluate constant expression. Simply move constant + // values from constantInputs_. inputValues_[i] = std::move(constantInputs_[i]); inputValues_[i]->resize(rows.end()); } else { @@ -766,8 +768,8 @@ void Expr::eval( return; } - // Make sure to include current expression in the error message in case of an - // exception. + // Make sure to include current expression in the error message in case of + // an exception. ExprExceptionContext exprExceptionContext{this, context.row(), parentExprSet}; ExceptionContextSetter exceptionContext( {.messageFunc = parentExprSet ? onTopLevelException : onException, @@ -791,17 +793,18 @@ void Expr::eval( // peeling and wrapping in the sub-nodes. // // Also load fields referenced by shared sub expressions to ensure that if - // there is an encoding on the loaded vector, then it is always peeled before - // evaluating sub-expression. Otherwise, the first call to - // evaluateSharedSubexpr might pass rows before peeling and the next one pass - // rows after peeling. + // there is an encoding on the loaded vector, then it is always peeled + // before evaluating sub-expression. Otherwise, the first call to + // evaluateSharedSubexpr might pass rows before peeling and the next one + // pass rows after peeling. // // Finally, for non-null propagating expressions, load multiply referenced - // inputs unconditionally as it is hard to keep track of the superset of rows - // that would end up being evaluated among all its children (and hence need to - // be loaded). This is because any of the children might have null propagating - // expressions that end up operating on a reduced set of rows. So, one sub - // tree might need only a subset, whereas other might need a different subset. + // inputs unconditionally as it is hard to keep track of the superset of + // rows that would end up being evaluated among all its children (and hence + // need to be loaded). This is because any of the children might have null + // propagating expressions that end up operating on a reduced set of rows. + // So, one sub tree might need only a subset, whereas other might need a + // different subset. // // TODO: Re-work the logic of deciding when to load which field. if (!hasConditionals_ || distinctFields_.size() == 1 || @@ -813,10 +816,10 @@ void Expr::eval( } } else if ( !propagatesNulls_ && !evaluatesArgumentsOnNonIncreasingSelection()) { - // Load multiply-referenced fields at common parent expr with "rows". Delay - // loading fields that are not in multiplyReferencedFields_. In case - // evaluatesArgumentsOnNonIncreasingSelection() is true, this is delayed - // until we process the inputs of ConjunctExpr. + // Load multiply-referenced fields at common parent expr with "rows". + // Delay loading fields that are not in multiplyReferencedFields_. In + // case evaluatesArgumentsOnNonIncreasingSelection() is true, this is + // delayed until we process the inputs of ConjunctExpr. for (const auto& field : multiplyReferencedFields_) { context.ensureFieldLoaded(field->index(context), rows); } @@ -919,8 +922,8 @@ void Expr::evaluateSharedSubexpr( VELOX_DCHECK(missingRows->hasSelections()); // Fix finalSelection to avoid losing values outside missingRows. - // Final selection of rows need to include sharedSubexprRows_, missingRows and - // current final selection of rows if set. + // Final selection of rows need to include sharedSubexprRows_, missingRows + // and current final selection of rows if set. LocalSelectivityVector newFinalSelectionHolder(context, *sharedSubexprRows); auto* newFinalSelection = newFinalSelectionHolder.get(); newFinalSelection->select(*missingRows); @@ -971,9 +974,9 @@ Expr::PeelEncodingsResult Expr::peelEncodings( // Prepare the rows and vectors to peel. - // Use finalSelection to generate peel to ensure those rows can be translated - // and ensure consistent peeling across multiple calls to this expression if - // its a shared subexpression. + // Use finalSelection to generate peel to ensure those rows can be + // translated and ensure consistent peeling across multiple calls to this + // expression if its a shared subexpression. const auto& rowsToPeel = context.isFinalSelection() ? rows : *context.finalSelection(); [[maybe_unused]] auto numFields = context.row()->childrenSize(); @@ -1116,8 +1119,8 @@ bool Expr::removeSureNulls( } if (result) { result->updateBounds(); - // Default-null behavior has taken place if some sure nulls has been removed - // from rows. + // Default-null behavior has taken place if some sure nulls has been + // removed from rows. if (result->countSelected() < rows.countSelected()) { stats_.defaultNullRowsSkipped = true; return true; @@ -1174,11 +1177,11 @@ void Expr::evalWithNulls( } // Optimization that attempts to cache results for inputs that are dictionary -// encoded and use the same base vector between subsequent input batches. Since -// this hold onto a reference to the base vector and the cached results, it can -// be memory intensive. Therefore in order to reduce this consumption and ensure -// it is only employed for cases where it can be useful, it only starts caching -// result after it encounters the same base at least twice. +// encoded and use the same base vector between subsequent input batches. +// Since this hold onto a reference to the base vector and the cached results, +// it can be memory intensive. Therefore in order to reduce this consumption +// and ensure it is only employed for cases where it can be useful, it only +// starts caching result after it encounters the same base at least twice. void Expr::evalWithMemo( const SelectivityVector& rows, EvalCtx& context, @@ -1405,8 +1408,8 @@ void Expr::evalAllImpl( bool tryPeelArgs = deterministic_ ? true : false; bool defaultNulls = vectorFunctionMetadata_.defaultNullBehavior; - // Tracks what subset of rows shall un-evaluated inputs and current expression - // evaluates. Initially points to rows. + // Tracks what subset of rows shall un-evaluated inputs and current + // expression evaluates. Initially points to rows. MutableRemainingRows remainingRows(rows, context); if (defaultNulls) { if (!evalArgsDefaultNulls( @@ -1481,9 +1484,9 @@ bool Expr::applyFunctionWithPeeling( peeledVectors.clear(); // Translate the relevant rows. - // Note: We do not need to translate final selection since at this stage those - // rows are not used but isFinalSelection() is only used to check whether - // pre-existing rows need to be preserved. + // Note: We do not need to translate final selection since at this stage + // those rows are not used but isFinalSelection() is only used to check + // whether pre-existing rows need to be preserved. auto newRows = peeledEncoding->translateToInnerRows(applyRows, newRowsHolder); withContextSaver([&](ContextSaver& saver) { @@ -1498,9 +1501,9 @@ bool Expr::applyFunctionWithPeeling( this->type(), context.pool(), peeledResult, applyRows); context.moveOrCopyResult(wrappedResult, applyRows, result); - // Recycle peeledResult if it's not owned by the result vector. Examples of - // when this can happen is when the result is a primitive constant vector, - // or when moveOrCopyResult copies wrappedResult content. + // Recycle peeledResult if it's not owned by the result vector. Examples + // of when this can happen is when the result is a primitive constant + // vector, or when moveOrCopyResult copies wrappedResult content. context.releaseVector(peeledResult); }); @@ -1671,8 +1674,8 @@ common::Subfield extractSubfield( for (;;) { if (auto* ref = expr->as()) { const auto& name = ref->name(); - // When the field name is empty string, it typically means that the field - // name was not set in the parent type. + // When the field name is empty string, it typically means that the + // field name was not set in the parent type. if (name == "") { expr = expr->inputs()[0].get(); continue; @@ -1850,10 +1853,9 @@ void printInputAndExprs( const BaseVector* vector, const std::vector>& exprs) { const char* basePath = - config::globalConfig().saveInputOnExpressionAnyFailurePath.c_str(); + FLAGS_velox_save_input_on_expression_any_failure_path.c_str(); if (strlen(basePath) == 0) { - basePath = - config::globalConfig().saveInputOnExpressionSystemFailurePath.c_str(); + basePath = FLAGS_velox_save_input_on_expression_system_failure_path.c_str(); } if (strlen(basePath) == 0) { return; @@ -1913,7 +1915,7 @@ void ExprSet::eval( context.ensureFieldLoaded(field->index(context), rows); } - if (config::globalConfig().experimentalSaveInputOnFatalSignal) { + if (FLAGS_velox_experimental_save_input_on_fatal_signal) { auto other = process::GetThreadDebugInfo(); process::ThreadDebugInfo debugInfo; if (other) { @@ -1977,7 +1979,7 @@ std::unique_ptr makeExprSetFromFlag( std::vector&& source, core::ExecCtx* execCtx) { if (execCtx->queryCtx()->queryConfig().exprEvalSimplified() || - config::globalConfig().forceEvalSimplified) { + FLAGS_force_eval_simplified) { return std::make_unique(std::move(source), execCtx); } return std::make_unique(std::move(source), execCtx); diff --git a/velox/expression/tests/ExprEncodingsTest.cpp b/velox/expression/tests/ExprEncodingsTest.cpp index db2ad732350..0cb706072ee 100644 --- a/velox/expression/tests/ExprEncodingsTest.cpp +++ b/velox/expression/tests/ExprEncodingsTest.cpp @@ -133,7 +133,7 @@ class ExprEncodingsTest void SetUp() override { // This test throws a lot of exceptions, so turn off stack trace capturing. - config::globalConfig().exceptionUserStacktraceEnabled = false; + FLAGS_velox_exception_user_stacktrace_enabled = false; functions::prestosql::registerAllScalarFunctions(); parse::registerTypeResolver(); diff --git a/velox/expression/tests/ExprTest.cpp b/velox/expression/tests/ExprTest.cpp index 3784d7adb15..d2d2edab315 100644 --- a/velox/expression/tests/ExprTest.cpp +++ b/velox/expression/tests/ExprTest.cpp @@ -42,6 +42,9 @@ #include "velox/vector/tests/TestingAlwaysThrowsFunction.h" #include "velox/vector/tests/utils/VectorTestBase.h" +DECLARE_string(velox_save_input_on_expression_any_failure_path); +DECLARE_string(velox_save_input_on_expression_system_failure_path); + namespace facebook::velox::test { namespace { class ExprTest : public testing::Test, public VectorTestBase { @@ -2408,8 +2411,8 @@ TEST_P(ParameterizedExprTest, exceptionContext) { registerFunction( {"always_throws"}); - config::globalConfig().saveInputOnExpressionAnyFailurePath = ""; - config::globalConfig().saveInputOnExpressionSystemFailurePath = ""; + FLAGS_velox_save_input_on_expression_any_failure_path = ""; + FLAGS_velox_save_input_on_expression_system_failure_path = ""; try { evaluate("always_throws(c0) + c1", data); @@ -2443,7 +2446,7 @@ TEST_P(ParameterizedExprTest, exceptionContext) { // Enable saving vector and expression SQL for system errors only. auto tempDirectory = exec::test::TempDirectoryPath::create(); - config::globalConfig().saveInputOnExpressionSystemFailurePath = + FLAGS_velox_save_input_on_expression_system_failure_path = tempDirectory->getPath(); try { @@ -2479,9 +2482,9 @@ TEST_P(ParameterizedExprTest, exceptionContext) { } // Enable saving vector and expression SQL for all errors. - config::globalConfig().saveInputOnExpressionAnyFailurePath = + FLAGS_velox_save_input_on_expression_any_failure_path = tempDirectory->getPath(); - config::globalConfig().saveInputOnExpressionSystemFailurePath = ""; + FLAGS_velox_save_input_on_expression_system_failure_path = ""; try { evaluate("always_throws(c0) + c1", data); diff --git a/velox/flag_definitions/CMakeLists.txt b/velox/flag_definitions/CMakeLists.txt index e008dfcef5a..8b551113d73 100644 --- a/velox/flag_definitions/CMakeLists.txt +++ b/velox/flag_definitions/CMakeLists.txt @@ -12,5 +12,4 @@ # 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 velox_global_config - gflags::gflags) +velox_link_libraries(velox_flag_definitions PRIVATE gflags::gflags) diff --git a/velox/flag_definitions/flags.cpp b/velox/flag_definitions/flags.cpp index bd58c820df4..01b44ead8dd 100644 --- a/velox/flag_definitions/flags.cpp +++ b/velox/flag_definitions/flags.cpp @@ -16,8 +16,6 @@ #include -#include "velox/common/config/GlobalConfig.h" - // Used in velox/common/memory/Memory.cpp DEFINE_int32( @@ -122,12 +120,6 @@ DEFINE_bool( 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"); - DEFINE_int32( cache_prefetch_min_pct, 80, @@ -139,39 +131,3 @@ DEFINE_bool( velox_ssd_verify_write, false, "Read back data after writing to SSD"); - -namespace facebook::velox { -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().useSsdODirect = FLAGS_velox_ssd_odirect; - config::globalConfig().verifySsdWrite = FLAGS_velox_ssd_verify_write; - 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; - config::globalConfig().forceEvalSimplified = FLAGS_force_eval_simplified; - config::globalConfig().experimentalSaveInputOnFatalSignal = - FLAGS_velox_experimental_save_input_on_fatal_signal; - config::globalConfig().saveInputOnExpressionAnyFailurePath = - FLAGS_velox_save_input_on_expression_any_failure_path; - config::globalConfig().saveInputOnExpressionSystemFailurePath = - FLAGS_velox_save_input_on_expression_system_failure_path; -} -} // namespace facebook::velox diff --git a/velox/flag_definitions/flags.h b/velox/flag_definitions/flags.h deleted file mode 100644 index a9d3a1e4c3f..00000000000 --- a/velox/flag_definitions/flags.h +++ /dev/null @@ -1,29 +0,0 @@ -/* - * 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(); -} - -/// Use O_DIRECT for SSD cache I/O. This allows to bypass Linux Kernel's page -/// cache and can improve performance on some filesystems. Disable if the -/// filesystem does not support it. -DECLARE_bool(velox_ssd_odirect); - -/// Verify the data written to SSD. Once an entry is written, it is immediately -/// read back and is compared against the entry written. -/// This is helpful to protect against SSD write corruption. -DECLARE_bool(velox_ssd_verify_write);