You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by mi...@apache.org on 2023/12/15 18:33:25 UTC
(impala) 02/02: Revert "IMPALA-11805: Fix LLVM memory manager bytes allocated"
This is an automated email from the ASF dual-hosted git repository.
michaelsmith pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit 6cc8114813eea18810c4fc4109ef06e63a23e077
Author: Michael Smith <mi...@cloudera.com>
AuthorDate: Thu Dec 14 23:57:29 2023 +0000
Revert "IMPALA-11805: Fix LLVM memory manager bytes allocated"
This reverts commit 8874ea07b68c09cee3c4e300b70706f3c5ea40af.
Reason for revert: other work on IMPALA-11805 will stop using the memory
manager's size for the codegen cache, and changing this had broader
implications on query memory tracking that need to be accounted for. It
started causing memory reservation failures in tests.
Change-Id: I7f7a6b21b3a9fc7c9e675c8d3349725c4863f744
Reviewed-on: http://gerrit.cloudera.org:8080/20796
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Michael Smith <mi...@cloudera.com>
---
be/src/codegen/llvm-codegen-cache-test.cc | 30 ++++++++++++-------------
be/src/codegen/llvm-codegen-test.cc | 6 -----
be/src/codegen/mcjit-mem-mgr.h | 10 +++++++--
be/src/thirdparty/llvm/SectionMemoryManager.cpp | 10 ---------
be/src/thirdparty/llvm/SectionMemoryManager.h | 6 -----
5 files changed, 22 insertions(+), 40 deletions(-)
diff --git a/be/src/codegen/llvm-codegen-cache-test.cc b/be/src/codegen/llvm-codegen-cache-test.cc
index d8be5a399..e1976eeca 100644
--- a/be/src/codegen/llvm-codegen-cache-test.cc
+++ b/be/src/codegen/llvm-codegen-cache-test.cc
@@ -18,7 +18,6 @@
#include "codegen/llvm-codegen-cache.h"
#include <boost/thread/thread.hpp>
#include <llvm/ExecutionEngine/ExecutionEngine.h>
-#include "llvm/Support/Process.h"
#include "codegen/mcjit-mem-mgr.h"
#include "common/object-pool.h"
#include "runtime/fragment-state.h"
@@ -37,9 +36,6 @@ namespace impala {
class LlvmCodeGenCacheTest : public testing::Test {
public:
virtual void SetUp() {
- // Using single shard makes the logic of scenarios simple for capacity and
- // eviction-related behavior.
- FLAGS_cache_force_single_shard = true;
FLAGS_codegen_cache_capacity = "0";
metrics_.reset(new MetricGroup("codegen-cache-test"));
profile_ = RuntimeProfile::Create(&obj_pool_, "codegen-cache-test");
@@ -52,11 +48,9 @@ class LlvmCodeGenCacheTest : public testing::Test {
PlanFragmentCtxPB* fragment_ctx = qs->obj_pool()->Add(new PlanFragmentCtxPB());
fragment_state_ =
qs->obj_pool()->Add(new FragmentState(qs, *fragment, *fragment_ctx));
- page_size_ = llvm::sys::Process::getPageSize();
}
virtual void TearDown() {
- FLAGS_cache_force_single_shard = false;
fragment_state_->ReleaseResources();
fragment_state_ = nullptr;
codegen_cache_.reset();
@@ -143,7 +137,6 @@ class LlvmCodeGenCacheTest : public testing::Test {
shared_ptr<LlvmExecutionEngineWrapper> exec_engine_wrapper_;
TQueryOptions query_options_;
TCodeGenOptLevel::type opt_level_ = TCodeGenOptLevel::O2;
- unsigned page_size_;
};
void LlvmCodeGenCacheTest::AddLlvmCodegenEcho(LlvmCodeGen* codegen) {
@@ -275,10 +268,19 @@ int64_t LlvmCodeGenCacheTest::GetMemCharge(
/// Test the situation that the codegen cache hits the limit of capacity, in this case,
/// eviction is needed when new insertion comes.
void LlvmCodeGenCacheTest::TestAtCapacity(TCodeGenCacheMode::type mode) {
- // LLVM allocates memory in pages, and for small examples allocates 3 pages.
- int64_t codegen_cache_capacity = 3 * page_size_ +
- sizeof(CodeGenCacheEntry) + CodeGenCacheKey::OptimalKeySize + sizeof(std::string);
+ int64_t codegen_cache_capacity = 196; // 196B
bool is_normal_mode = !CodeGenCacheModeAnalyzer::is_optimal(mode);
+ // 150B for optimal mode, or 164B on ARM systems
+ if (!is_normal_mode) {
+#ifdef __aarch64__
+ codegen_cache_capacity = 164;
+#else
+ codegen_cache_capacity = 150;
+#endif
+ }
+ // Using single shard makes the logic of scenarios simple for capacity and
+ // eviction-related behavior.
+ FLAGS_cache_force_single_shard = true;
// Create two LlvmCodeGen objects containing a different codegen function separately.
scoped_ptr<LlvmCodeGen> codegen;
@@ -486,9 +488,7 @@ void LlvmCodeGenCacheTest::TestSwitchModeHelper(TCodeGenCacheMode::type mode, st
// Test to switch among different modes.
TEST_F(LlvmCodeGenCacheTest, SwitchMode) {
- // Storage for 2 entries
- int64_t codegen_cache_capacity = 2 * (3 * page_size_ +
- sizeof(CodeGenCacheEntry) + CodeGenCacheKey::OptimalKeySize + sizeof(std::string));
+ int64_t codegen_cache_capacity = 512; // 512B
codegen_cache_.reset(new CodeGenCache(metrics_.get()));
EXPECT_OK(codegen_cache_->Init(codegen_cache_capacity));
string key = "key";
@@ -579,9 +579,7 @@ void LlvmCodeGenCacheTest::TestConcurrentStore(int num_threads) {
}
TEST_F(LlvmCodeGenCacheTest, ConcurrentStore) {
- // Storage for 2 entries
- int64_t codegen_cache_capacity = 2 * (3 * page_size_ +
- sizeof(CodeGenCacheEntry) + CodeGenCacheKey::OptimalKeySize + sizeof(std::string));
+ int64_t codegen_cache_capacity = 512; // 512B
codegen_cache_.reset(new CodeGenCache(metrics_.get()));
EXPECT_OK(codegen_cache_->Init(codegen_cache_capacity));
TestConcurrentStore(8);
diff --git a/be/src/codegen/llvm-codegen-test.cc b/be/src/codegen/llvm-codegen-test.cc
index 2b87267a0..3f6f70daa 100644
--- a/be/src/codegen/llvm-codegen-test.cc
+++ b/be/src/codegen/llvm-codegen-test.cc
@@ -39,8 +39,6 @@
using std::unique_ptr;
-DECLARE_bool(cache_force_single_shard);
-
namespace impala {
class CodegenFnPtrTest : public testing:: Test {
@@ -581,9 +579,6 @@ class LlvmOptTest :
TQueryOptions query_opts_;
virtual void SetUp() {
- // Using single shard makes the logic of scenarios simple for capacity and
- // eviction-related behavior.
- FLAGS_cache_force_single_shard = true;
metrics_.reset(new MetricGroup("codegen-test"));
test_env_.reset(new TestEnv());
ASSERT_OK(test_env_->Init());
@@ -591,7 +586,6 @@ class LlvmOptTest :
}
virtual void TearDown() {
- FLAGS_cache_force_single_shard = false;
fragment_state_->ReleaseResources();
fragment_state_ = nullptr;
test_env_.reset();
diff --git a/be/src/codegen/mcjit-mem-mgr.h b/be/src/codegen/mcjit-mem-mgr.h
index 5844cb6f7..6002b6087 100644
--- a/be/src/codegen/mcjit-mem-mgr.h
+++ b/be/src/codegen/mcjit-mem-mgr.h
@@ -36,7 +36,7 @@ namespace impala {
/// We also use it to track how much memory is allocated for compiled code.
class ImpalaMCJITMemoryManager : public SectionMemoryManager {
public:
- ImpalaMCJITMemoryManager() : bytes_tracked_(0) {}
+ ImpalaMCJITMemoryManager() : bytes_allocated_(0), bytes_tracked_(0){}
virtual uint64_t getSymbolAddress(const std::string& name) override {
if (name == "__dso_handle") return reinterpret_cast<uint64_t>(&__dso_handle);
@@ -45,23 +45,29 @@ class ImpalaMCJITMemoryManager : public SectionMemoryManager {
virtual uint8_t* allocateCodeSection(uintptr_t size, unsigned alignment,
unsigned section_id, llvm::StringRef section_name) override {
+ bytes_allocated_ += size;
return SectionMemoryManager::allocateCodeSection(
size, alignment, section_id, section_name);
}
virtual uint8_t* allocateDataSection(uintptr_t size, unsigned alignment,
unsigned section_id, llvm::StringRef section_name, bool is_read_only) override {
+ bytes_allocated_ += size;
return SectionMemoryManager::allocateDataSection(
size, alignment, section_id, section_name, is_read_only);
}
+ int64_t bytes_allocated() const { return bytes_allocated_; }
int64_t bytes_tracked() const { return bytes_tracked_; }
void set_bytes_tracked(int64_t bytes_tracked) {
- DCHECK_LE(bytes_tracked, bytes_allocated());
+ DCHECK_LE(bytes_tracked, bytes_allocated_);
bytes_tracked_ = bytes_tracked;
}
private:
+ /// Total bytes allocated for the compiled code.
+ int64_t bytes_allocated_;
+
/// Total bytes already tracked by MemTrackers. <= 'bytes_allocated_'.
/// Needed to release the correct amount from the MemTracker when done.
int64_t bytes_tracked_;
diff --git a/be/src/thirdparty/llvm/SectionMemoryManager.cpp b/be/src/thirdparty/llvm/SectionMemoryManager.cpp
index 825843c0a..5964275ad 100644
--- a/be/src/thirdparty/llvm/SectionMemoryManager.cpp
+++ b/be/src/thirdparty/llvm/SectionMemoryManager.cpp
@@ -311,16 +311,6 @@ void SectionMemoryManager::invalidateInstructionCache() {
sys::Memory::InvalidateInstructionCache(Block.base(), Block.size());
}
-// Impala: added to track actual allocation
-int64_t SectionMemoryManager::bytes_allocated() const {
- int64_t total = 0;
- for (const MemoryGroup *Group : {&CodeMem, &RWDataMem, &RODataMem}) {
- for (const sys::MemoryBlock &Block : Group->AllocatedMem)
- total += Block.size();
- }
- return total;
-}
-
SectionMemoryManager::~SectionMemoryManager() {
for (MemoryGroup *Group : {&CodeMem, &RWDataMem, &RODataMem}) {
for (sys::MemoryBlock &Block : Group->AllocatedMem)
diff --git a/be/src/thirdparty/llvm/SectionMemoryManager.h b/be/src/thirdparty/llvm/SectionMemoryManager.h
index fdfcb917a..23ba5de28 100644
--- a/be/src/thirdparty/llvm/SectionMemoryManager.h
+++ b/be/src/thirdparty/llvm/SectionMemoryManager.h
@@ -105,12 +105,6 @@ public:
/// This method is called from finalizeMemory.
virtual void invalidateInstructionCache();
- /// \brief Returns number of bytes allocated by the memory manager.
- ///
- /// Impala: added to track actual memory allocation (which is page-aligned)
- /// rather than just requested memory.
- int64_t bytes_allocated() const;
-
private:
struct FreeMemBlock {
// The actual block of free memory