Fix static initialization order fiasco in velox (facebookincubator#12204

) Summary: X-link: prestodb/presto#24458 Replace global statics with local to avoid non-determinism https://en.cppreference.com/w/cpp/language/siof Differential Revision: D68857074
mkatsevVR · Jan 31, 2025 · 836c36e · 836c36e
1 parent dcafd32
commit 836c36e
Show file tree

Hide file tree

Showing 20 changed files with 68 additions and 64 deletions.
diff --git a/velox/common/base/VeloxException.cpp b/velox/common/base/VeloxException.cpp
@@ -136,16 +136,16 @@ bool isStackTraceEnabled(VeloxException::Type type) {
   using namespace std::literals::chrono_literals;
   const bool isSysException = type == VeloxException::Type::kSystem;
   if ((isSysException &&
-       !config::globalConfig.exceptionSystemStacktraceEnabled) ||
+       !config::globalConfig().exceptionSystemStacktraceEnabled) ||
       (!isSysException &&
-       !config::globalConfig.exceptionUserStacktraceEnabled)) {
+       !config::globalConfig().exceptionUserStacktraceEnabled)) {
     // VeloxException stacktraces are disabled.
     return false;
   }
 
   const int32_t rateLimitMs = isSysException
-      ? config::globalConfig.exceptionSystemStacktraceRateLimitMs
-      : config::globalConfig.exceptionUserStacktraceRateLimitMs;
+      ? 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

diff --git a/velox/common/base/tests/ExceptionTest.cpp b/velox/common/base/tests/ExceptionTest.cpp
@@ -112,8 +112,8 @@ void testExceptionTraceCollectionControl(bool userException, bool enabled) {
     SCOPED_TRACE(fmt::format(
         "enabled: {}, user flag: {}, sys flag: {}",
         enabled,
-        config::globalConfig.exceptionUserStacktraceEnabled,
-        config::globalConfig.exceptionSystemStacktraceEnabled));
+        config::globalConfig().exceptionUserStacktraceEnabled,
+        config::globalConfig().exceptionSystemStacktraceEnabled));
     ASSERT_EQ(userException, e.exceptionType() == VeloxException::Type::kUser);
     ASSERT_EQ(enabled, e.stackTrace() != nullptr);
   }
@@ -179,8 +179,8 @@ void testExceptionTraceCollectionRateControl(
           "userException: {}, hasRateLimit: {}, user limit: {}ms, sys limit: {}ms",
           userException,
           hasRateLimit,
-          config::globalConfig.exceptionUserStacktraceRateLimitMs,
-          config::globalConfig.exceptionSystemStacktraceRateLimitMs));
+          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/caching/tests/AsyncDataCacheTest.cpp b/velox/common/caching/tests/AsyncDataCacheTest.cpp
@@ -718,7 +718,7 @@ TEST_P(AsyncDataCacheTest, pin) {
 
 TEST_P(AsyncDataCacheTest, replace) {
   constexpr int64_t kMaxBytes = 64 << 20;
-  config::globalConfig.exceptionUserStacktraceEnabled = false;
+  config::globalConfig().exceptionUserStacktraceEnabled = false;
   initializeCache(kMaxBytes);
   // Load 10x the max size, inject an error every 21 batches.
   loadLoop(0, kMaxBytes * 10, 21);
@@ -736,7 +736,7 @@ TEST_P(AsyncDataCacheTest, replace) {
 
 TEST_P(AsyncDataCacheTest, evictAccounting) {
   constexpr int64_t kMaxBytes = 64 << 20;
-  config::globalConfig.exceptionUserStacktraceEnabled = false;
+  config::globalConfig().exceptionUserStacktraceEnabled = false;
   initializeCache(kMaxBytes);
   auto pool = manager_->addLeafPool("test");
 
@@ -760,7 +760,7 @@ TEST_P(AsyncDataCacheTest, evictAccounting) {
 TEST_P(AsyncDataCacheTest, largeEvict) {
   constexpr int64_t kMaxBytes = 256 << 20;
   constexpr int32_t kNumThreads = 24;
-  config::globalConfig.exceptionUserStacktraceEnabled = false;
+  config::globalConfig().exceptionUserStacktraceEnabled = false;
   initializeCache(kMaxBytes);
   // Load 10x the max size, inject an allocation of 1/8 the capacity every 4
   // batches.
@@ -839,7 +839,7 @@ TEST_P(AsyncDataCacheTest, DISABLED_ssd) {
   constexpr uint64_t kRamBytes = 32 << 20;
   constexpr uint64_t kSsdBytes = 512UL << 20;
 #endif
-  config::globalConfig.exceptionUserStacktraceEnabled = false;
+  config::globalConfig().exceptionUserStacktraceEnabled = false;
   initializeCache(kRamBytes, kSsdBytes);
   cache_->setVerifyHook(
       [&](const AsyncDataCacheEntry& entry) { checkContents(entry); });

diff --git a/velox/common/config/GlobalConfig.cpp b/velox/common/config/GlobalConfig.cpp
@@ -18,6 +18,9 @@
 
 namespace facebook::velox::config {
 
-GlobalConfiguration globalConfig;
+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
@@ -79,6 +79,6 @@ struct GlobalConfiguration {
   std::string saveInputOnExpressionSystemFailurePath;
 };
 
-extern GlobalConfiguration globalConfig;
+GlobalConfiguration& globalConfig();
 
 } // namespace facebook::velox::config
diff --git 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
@@ -78,7 +78,7 @@ std::vector<std::shared_ptr<MemoryPool>> createSharedLeafMemoryPools(
   VELOX_CHECK_EQ(sysPool.name(), kSysRootName);
   std::vector<std::shared_ptr<MemoryPool>> leafPools;
   const size_t numSharedPools =
-      std::max(1, config::globalConfig.memoryNumSharedLeafPools);
+      std::max(1, config::globalConfig().memoryNumSharedLeafPools);
   leafPools.reserve(numSharedPools);
   for (size_t i = 0; i < numSharedPools; ++i) {
     leafPools.emplace_back(
@@ -129,7 +129,7 @@ MemoryManager::MemoryManager(const MemoryManagerOptions& options)
       sysRoot_->name());
   VELOX_CHECK_EQ(
       sharedLeafPools_.size(),
-      std::max(1, config::globalConfig.memoryNumSharedLeafPools));
+      std::max(1, config::globalConfig().memoryNumSharedLeafPools));
 }
 
 MemoryManager::~MemoryManager() {

diff --git 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
@@ -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
@@ -94,7 +94,7 @@ struct Stats {
 
   template <typename Op>
   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 <typename Op>
   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
@@ -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
@@ -151,7 +151,7 @@ class MemoryPool : public std::enable_shared_from_this<MemoryPool> {
 
     /// 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
@@ -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/dwio/common/tests/LoggedExceptionTest.cpp b/velox/dwio/common/tests/LoggedExceptionTest.cpp
@@ -25,13 +25,14 @@ 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;
+  config::globalConfig().exceptionUserStacktraceRateLimitMs = 0;
+  config::globalConfig().exceptionSystemStacktraceRateLimitMs = 0;
 
   // NOTE: the user config should not affect the tracing behavior of system type
   // of exception collection.
-  config::globalConfig.exceptionUserStacktraceEnabled = folly::Random::oneIn(2);
-  config::globalConfig.exceptionSystemStacktraceEnabled =
+  config::globalConfig().exceptionUserStacktraceEnabled =
+      folly::Random::oneIn(2);
+  config::globalConfig().exceptionSystemStacktraceEnabled =
       enabled ? true : false;
 
   try {
@@ -40,8 +41,8 @@ void testTraceCollectionSwitchControl(bool enabled) {
     SCOPED_TRACE(fmt::format(
         "enabled: {}, user flag: {}, sys flag: {}",
         enabled,
-        config::globalConfig.exceptionUserStacktraceEnabled,
-        config::globalConfig.exceptionSystemStacktraceEnabled));
+        config::globalConfig().exceptionUserStacktraceEnabled,
+        config::globalConfig().exceptionSystemStacktraceEnabled));
     ASSERT_TRUE(e.exceptionType() == VeloxException::Type::kSystem);
     ASSERT_EQ(enabled, e.stackTrace() != nullptr);
   }

diff --git 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
@@ -342,8 +342,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;

diff --git a/velox/expression/Expr.cpp b/velox/expression/Expr.cpp
@@ -660,10 +660,10 @@ std::string onTopLevelException(VeloxException::Type exceptionType, void* arg) {
   auto* context = static_cast<ExprExceptionContext*>(arg);
 
   const char* basePath =
-      config::globalConfig.saveInputOnExpressionAnyFailurePath.c_str();
+      config::globalConfig().saveInputOnExpressionAnyFailurePath.c_str();
   if (strlen(basePath) == 0 && exceptionType == VeloxException::Type::kSystem) {
     basePath =
-        config::globalConfig.saveInputOnExpressionSystemFailurePath.c_str();
+        config::globalConfig().saveInputOnExpressionSystemFailurePath.c_str();
   }
   if (strlen(basePath) == 0) {
     return fmt::format("Top-level Expression: {}", context->expr()->toString());
@@ -1850,10 +1850,10 @@ void printInputAndExprs(
     const BaseVector* vector,
     const std::vector<std::shared_ptr<Expr>>& exprs) {
   const char* basePath =
-      config::globalConfig.saveInputOnExpressionAnyFailurePath.c_str();
+      config::globalConfig().saveInputOnExpressionAnyFailurePath.c_str();
   if (strlen(basePath) == 0) {
     basePath =
-        config::globalConfig.saveInputOnExpressionSystemFailurePath.c_str();
+        config::globalConfig().saveInputOnExpressionSystemFailurePath.c_str();
   }
   if (strlen(basePath) == 0) {
     return;
@@ -1913,7 +1913,7 @@ void ExprSet::eval(
     context.ensureFieldLoaded(field->index(context), rows);
   }
 
-  if (config::globalConfig.experimentalSaveInputOnFatalSignal) {
+  if (config::globalConfig().experimentalSaveInputOnFatalSignal) {
     auto other = process::GetThreadDebugInfo();
     process::ThreadDebugInfo debugInfo;
     if (other) {
@@ -1977,7 +1977,7 @@ std::unique_ptr<ExprSet> makeExprSetFromFlag(
     std::vector<core::TypedExprPtr>&& source,
     core::ExecCtx* execCtx) {
   if (execCtx->queryCtx()->queryConfig().exprEvalSimplified() ||
-      config::globalConfig.forceEvalSimplified) {
+      config::globalConfig().forceEvalSimplified) {
     return std::make_unique<ExprSetSimplified>(std::move(source), execCtx);
   }
   return std::make_unique<ExprSet>(std::move(source), execCtx);

diff --git 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;
+    config::globalConfig().exceptionUserStacktraceEnabled = false;
 
     functions::prestosql::registerAllScalarFunctions();
     parse::registerTypeResolver();

diff --git a/velox/expression/tests/ExprTest.cpp b/velox/expression/tests/ExprTest.cpp
@@ -2408,8 +2408,8 @@ TEST_P(ParameterizedExprTest, exceptionContext) {
   registerFunction<TestingAlwaysThrowsFunction, int32_t, int32_t>(
       {"always_throws"});
 
-  config::globalConfig.saveInputOnExpressionAnyFailurePath = "";
-  config::globalConfig.saveInputOnExpressionSystemFailurePath = "";
+  config::globalConfig().saveInputOnExpressionAnyFailurePath = "";
+  config::globalConfig().saveInputOnExpressionSystemFailurePath = "";
 
   try {
     evaluate("always_throws(c0) + c1", data);
@@ -2443,7 +2443,7 @@ TEST_P(ParameterizedExprTest, exceptionContext) {
 
   // Enable saving vector and expression SQL for system errors only.
   auto tempDirectory = exec::test::TempDirectoryPath::create();
-  config::globalConfig.saveInputOnExpressionSystemFailurePath =
+  config::globalConfig().saveInputOnExpressionSystemFailurePath =
       tempDirectory->getPath();
 
   try {
@@ -2479,9 +2479,9 @@ TEST_P(ParameterizedExprTest, exceptionContext) {
   }
 
   // Enable saving vector and expression SQL for all errors.
-  config::globalConfig.saveInputOnExpressionAnyFailurePath =
+  config::globalConfig().saveInputOnExpressionAnyFailurePath =
       tempDirectory->getPath();
-  config::globalConfig.saveInputOnExpressionSystemFailurePath = "";
+  config::globalConfig().saveInputOnExpressionSystemFailurePath = "";
 
   try {
     evaluate("always_throws(c0) + c1", data);

diff --git a/velox/flag_definitions/flags.cpp b/velox/flag_definitions/flags.cpp
@@ -135,34 +135,34 @@ DEFINE_int32(
 
 namespace facebook::velox {
 void translateFlagsToGlobalConfig() {
-  config::globalConfig.memoryNumSharedLeafPools =
+  config::globalConfig().memoryNumSharedLeafPools =
       FLAGS_velox_memory_num_shared_leaf_pools;
-  config::globalConfig.memoryLeakCheckEnabled =
+  config::globalConfig().memoryLeakCheckEnabled =
       FLAGS_velox_memory_leak_check_enabled;
-  config::globalConfig.memoryPoolDebugEnabled =
+  config::globalConfig().memoryPoolDebugEnabled =
       FLAGS_velox_memory_pool_debug_enabled;
-  config::globalConfig.enableMemoryUsageTrackInDefaultMemoryPool =
+  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 =
+  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 =
+  config::globalConfig().memoryPoolCapacityTransferAcrossTasks =
       FLAGS_velox_memory_pool_capacity_transfer_across_tasks;
-  config::globalConfig.exceptionSystemStacktraceEnabled =
+  config::globalConfig().exceptionSystemStacktraceEnabled =
       FLAGS_velox_exception_system_stacktrace_enabled;
-  config::globalConfig.exceptionSystemStacktraceRateLimitMs =
+  config::globalConfig().exceptionSystemStacktraceRateLimitMs =
       FLAGS_velox_exception_system_stacktrace_rate_limit_ms;
-  config::globalConfig.exceptionUserStacktraceEnabled =
+  config::globalConfig().exceptionUserStacktraceEnabled =
       FLAGS_velox_exception_user_stacktrace_enabled;
-  config::globalConfig.exceptionUserStacktraceRateLimitMs =
+  config::globalConfig().exceptionUserStacktraceRateLimitMs =
       FLAGS_velox_exception_user_stacktrace_rate_limit_ms;
-  config::globalConfig.forceEvalSimplified = FLAGS_force_eval_simplified;
-  config::globalConfig.experimentalSaveInputOnFatalSignal =
+  config::globalConfig().forceEvalSimplified = FLAGS_force_eval_simplified;
+  config::globalConfig().experimentalSaveInputOnFatalSignal =
       FLAGS_velox_experimental_save_input_on_fatal_signal;
-  config::globalConfig.saveInputOnExpressionAnyFailurePath =
+  config::globalConfig().saveInputOnExpressionAnyFailurePath =
       FLAGS_velox_save_input_on_expression_any_failure_path;
-  config::globalConfig.saveInputOnExpressionSystemFailurePath =
+  config::globalConfig().saveInputOnExpressionSystemFailurePath =
       FLAGS_velox_save_input_on_expression_system_failure_path;
 }
 } // namespace facebook::velox