-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
[Feature](multi-catalog) Add memory tracker for orc reader/writer and arrow parquet writer。 #37234
[Feature](multi-catalog) Add memory tracker for orc reader/writer and arrow parquet writer。 #37234
Conversation
Thank you for your contribution to Apache Doris. Since 2024-03-18, the Document has been moved to doris-website. |
run buildall |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
|
||
#include <map> | ||
|
||
#include "orc/MemoryPool.hh" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: 'orc/MemoryPool.hh' file not found [clang-diagnostic-error]
#include "orc/MemoryPool.hh"
^
6bb4b3f
to
61ac8ab
Compare
run buildall |
TPC-H: Total hot run time: 40162 ms
|
TPC-DS: Total hot run time: 175352 ms
|
ClickBench: Total hot run time: 30.89 s
|
61ac8ab
to
671de96
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
#endif // #if defined(USE_JEMALLOC) && defined(USE_MEM_TRACKER) | ||
} | ||
|
||
void free(char* p) override { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: pointer parameter 'p' can be pointer to const [readability-non-const-parameter]
void free(char* p) override { | |
void free(const char* p) override { |
run buildall |
TPC-H: Total hot run time: 41072 ms
|
TPC-DS: Total hot run time: 172392 ms
|
ClickBench: Total hot run time: 30.95 s
|
671de96
to
1fa9e9f
Compare
run buildall |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
@@ -84,7 +84,7 @@ class VOrcTransformer final : public VFileFormatTransformer { | |||
TFileCompressType::type compression, | |||
const iceberg::Schema* iceberg_schema = nullptr); | |||
|
|||
~VOrcTransformer() = default; | |||
~VOrcTransformer(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: annotate this function with 'override' or (rarely) 'final' [modernize-use-override]
~VOrcTransformer(); | |
~VOrcTransformer() override; |
TPC-H: Total hot run time: 39593 ms
|
TPC-DS: Total hot run time: 173515 ms
|
ClickBench: Total hot run time: 30.69 s
|
1fa9e9f
to
6679a62
Compare
run buildall |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
std::mutex RecordSizeMemoryAllocator::_mutex; | ||
|
||
template <bool clear_memory_, bool mmap_populate, bool use_mmap, typename MemoryAllocator> | ||
void Allocator<clear_memory_, mmap_populate, use_mmap, MemoryAllocator>::sys_memory_check( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: function 'sys_memory_check' exceeds recommended size/complexity thresholds [readability-function-size]
void Allocator<clear_memory_, mmap_populate, use_mmap, MemoryAllocator>::sys_memory_check(
^
Additional context
be/src/vec/common/allocator.cpp:46: 112 lines including whitespace and comments (threshold 80)
void Allocator<clear_memory_, mmap_populate, use_mmap, MemoryAllocator>::sys_memory_check(
^
@@ -23,6 +23,10 @@ | |||
// TODO: Readable | |||
|
|||
#include <fmt/format.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: 'fmt/format.h' file not found [clang-diagnostic-error]
#include <fmt/format.h>
^
be/src/vec/common/allocator.h
Outdated
|
||
void* alloc(size_t size) { | ||
if (size <= N) { | ||
if constexpr (Base::clear_memory) memset(stack_memory, 0, N); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: statement should be inside braces [readability-braces-around-statements]
if constexpr (Base::clear_memory) memset(stack_memory, 0, N); | |
if constexpr (Base::clear_memory) { memset(stack_memory, 0, N); | |
} |
be/src/vec/common/allocator.h
Outdated
if (size > N) Base::free(buf, size); | ||
} | ||
void free(void* buf, size_t size) { | ||
if (size > N) Base::free(buf, size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: statement should be inside braces [readability-braces-around-statements]
if (size > N) Base::free(buf, size); | |
if (size > N) { Base::free(buf, size); | |
} |
be/src/vec/common/allocator.h
Outdated
if (new_size <= N) return buf; | ||
void* realloc(void* buf, size_t old_size, size_t new_size) { | ||
/// Was in stack_memory, will remain there. | ||
if (new_size <= N) return buf; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: statement should be inside braces [readability-braces-around-statements]
if (new_size <= N) return buf; | |
if (new_size <= N) { return buf; | |
} |
be/src/vec/common/allocator.h
Outdated
/// Already was big enough to not fit in stack_memory. | ||
if (old_size > N) return Base::realloc(buf, old_size, new_size, Alignment); | ||
/// Already was big enough to not fit in stack_memory. | ||
if (old_size > N) return Base::realloc(buf, old_size, new_size, Alignment); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: statement should be inside braces [readability-braces-around-statements]
if (old_size > N) return Base::realloc(buf, old_size, new_size, Alignment); | |
if (old_size > N) { return Base::realloc(buf, old_size, new_size, Alignment); | |
} |
return arrow::Status::OK(); | ||
} | ||
|
||
void ArrowAllocator::deallocate_aligned(uint8_t* ptr, int64_t size, int64_t alignment) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: pointer parameter 'ptr' can be pointer to const [readability-non-const-parameter]
void ArrowAllocator::deallocate_aligned(uint8_t* ptr, int64_t size, int64_t alignment) { | |
void ArrowAllocator::deallocate_aligned(const uint8_t* ptr, int64_t size, int64_t alignment) { |
be/src/vec/exec/format/parquet/arrow_memory_pool.h:47:
- void deallocate_aligned(uint8_t* ptr, int64_t size, int64_t alignment);
+ void deallocate_aligned(const uint8_t* ptr, int64_t size, int64_t alignment);
|
||
#pragma once | ||
|
||
#include "arrow/memory_pool.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: 'arrow/memory_pool.h' file not found [clang-diagnostic-error]
#include "arrow/memory_pool.h"
^
6679a62
to
fe66e7f
Compare
run buildall |
fe66e7f
to
ad0e5b4
Compare
run buildall |
PR approved by anyone and no changes requested. |
766cffa
to
1bce0fe
Compare
run buildall |
1bce0fe
to
af452ad
Compare
run buildall |
TPC-H: Total hot run time: 39933 ms
|
TPC-DS: Total hot run time: 173202 ms
|
ClickBench: Total hot run time: 30.8 s
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
PR approved by at least one committer and no changes requested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
… arrow parquet writer。 (#37234) ## Proposed changes [Feature] (multi-catalog) Add memory tracker for orc reader/writer and arrow parquet writer。 ## Future work - Since the parquet reader is written by ourself and does not use the arrow third-party library, some memory usage needs to be added to the memory track. - Added read and write operator-level memory tracker to the profile.
Fix allocator.h compiling failed on mac which introduced by #37234.
Fix allocator.h compiling failed on mac which introduced by apache#37234.
Fix allocator.h compiling failed on mac which introduced by apache#37234.
Fix allocator.h compiling failed on mac which introduced by #37234.
Fix allocator.h compiling failed on mac which introduced by #37234.
Proposed changes
[Feature] (multi-catalog) Add memory tracker for orc reader/writer and arrow parquet writer。
Future work