Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add Global Config in place of gflags #11745

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions velox/buffer/tests/BufferTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 <glog/logging.h>
Expand All @@ -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<int64_t>, "");
static_assert(Buffer::is_pod_like_v<StringView>, "");
Expand All @@ -40,6 +40,7 @@ class BufferTest : public testing::Test {
protected:
static void SetUpTestCase() {
FLAGS_velox_enable_memory_usage_track_in_default_memory_pool = true;
translateFlagsToGlobalConfig();
}

void SetUp() override {
Expand Down Expand Up @@ -490,5 +491,4 @@ TEST_F(BufferTest, sliceBooleanBuffer) {
Buffer::slice<bool>(bufferPtr, 5, 6, nullptr), "Pool must not be null.");
}

} // namespace velox
} // namespace facebook
} // namespace facebook::velox
1 change: 1 addition & 0 deletions velox/buffer/tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ target_link_libraries(
velox_memory
velox_buffer
velox_test_util
velox_flag_definitions
GTest::gtest
GTest::gtest_main
GTest::gmock
Expand Down
10 changes: 2 additions & 8 deletions velox/common/base/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 8 additions & 5 deletions velox/common/base/VeloxException.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/

#include "velox/common/base/VeloxException.h"
#include "velox/common/config/GlobalConfig.h"

#include <folly/synchronization/AtomicStruct.h>
#include <exception>
Expand Down Expand Up @@ -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;
Expand Down
1 change: 0 additions & 1 deletion velox/common/base/VeloxException.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
#include <folly/FixedString.h>
#include <folly/String.h>
#include <folly/synchronization/CallOnce.h>
#include <gflags/gflags.h>
#include <glog/logging.h>

#include "velox/common/process/StackTrace.h"
Expand Down
1 change: 1 addition & 0 deletions velox/common/base/tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ target_link_libraries(
velox_status
velox_exception
velox_temp_path
velox_flag_definitions
Boost::filesystem
Boost::headers
Folly::folly
Expand Down
13 changes: 9 additions & 4 deletions velox/common/base/tests/ExceptionTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
#include <gtest/gtest.h>

#include "velox/common/base/Exceptions.h"
#include "velox/common/config/GlobalConfig.h"
#include "velox/flag_definitions/flags.h"

using namespace facebook::velox;

Expand Down Expand Up @@ -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(
Expand All @@ -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);
}
Expand All @@ -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;
Expand All @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down
9 changes: 5 additions & 4 deletions velox/common/config/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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 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)
23 changes: 23 additions & 0 deletions velox/common/config/GlobalConfig.cpp
Original file line number Diff line number Diff line change
@@ -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
60 changes: 60 additions & 0 deletions velox/common/config/GlobalConfig.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/*
* 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 <cstdint>

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};
};

extern GlobalConfiguration globalConfig;

} // namespace facebook::velox::config
2 changes: 0 additions & 2 deletions velox/common/memory/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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)
2 changes: 1 addition & 1 deletion velox/common/memory/MallocAllocator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
2 changes: 0 additions & 2 deletions velox/common/memory/MallocAllocator.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
7 changes: 3 additions & 4 deletions velox/common/memory/Memory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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__"};
Expand Down Expand Up @@ -79,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, FLAGS_velox_memory_num_shared_leaf_pools);
std::max(1, config::globalConfig.memoryNumSharedLeafPools);
leafPools.reserve(numSharedPools);
for (size_t i = 0; i < numSharedPools; ++i) {
leafPools.emplace_back(
Expand Down Expand Up @@ -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.memoryNumSharedLeafPools));
}

MemoryManager::~MemoryManager() {
Expand Down
12 changes: 4 additions & 8 deletions velox/common/memory/Memory.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@

#include <fmt/format.h>
#include <folly/Synchronized.h>
#include <gflags/gflags.h>
#include <glog/logging.h>

#include <velox/common/base/Exceptions.h>
Expand All @@ -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
Expand All @@ -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};
Expand Down
4 changes: 1 addition & 3 deletions velox/common/memory/MemoryAllocator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -415,7 +413,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();
Expand Down
Loading
Loading