-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[feature](file cache) Support in-memory filecache for no-disk/slow-disk system #38811
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
Conversation
|
Thank you for your contribution to Apache Doris. Since 2024-03-18, the Document has been moved to doris-website. |
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
|
|
||
| namespace doris::io { | ||
|
|
||
| MemFileCacheStorage::~MemFileCacheStorage() {} |
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: use '= default' to define a trivial destructor [modernize-use-equals-default]
| MemFileCacheStorage::~MemFileCacheStorage() {} | |
| MemFileCacheStorage::~MemFileCacheStorage() = default; |
| namespace doris::io { | ||
|
|
||
| struct MemBlock { | ||
| std::shared_ptr<char> addr; |
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.
why not just a std::string
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.
It is a memory address pointing to a char array (a continous memory block), so I think it should be std::shared_ptr<char[]> instead?
|
TODO: config: for extensibility, use another json field instead of 'memory' in path |
|
Sry for this hanging WIP PR, I've been dealing with more urgent tasks. I will return to this ASAP! |
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
| LOG_ERROR("").tag("file cache path", cache_base_path).tag("error", strerror(errno)); | ||
| return Status::IOError("{} statfs error {}", cache_base_path, strerror(errno)); | ||
| } | ||
| size_t disk_capacity = static_cast<size_t>( |
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: use auto when initializing with a cast to avoid duplicating the type name [modernize-use-auto]
| size_t disk_capacity = static_cast<size_t>( | |
| auto disk_capacity = static_cast<size_t>( |
| } | ||
| } | ||
|
|
||
| void test_file_cache_memory_storage(io::FileCacheType cache_type) { |
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 'test_file_cache_memory_storage' exceeds recommended size/complexity thresholds [readability-function-size]
void test_file_cache_memory_storage(io::FileCacheType cache_type) {
^Additional context
be/test/io/cache/block_file_cache_test.cpp:659: 397 lines including whitespace and comments (threshold 80)
void test_file_cache_memory_storage(io::FileCacheType cache_type) {
^|
|
||
| TEST_F(BlockFileCacheTest, ttl_modify_memory_storage) { | ||
| test_file_cache_memory_storage(io::FileCacheType::NORMAL); | ||
| auto sp = SyncPoint::get_instance(); |
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: 'auto sp' can be declared as 'auto *sp' [readability-qualified-auto]
| auto sp = SyncPoint::get_instance(); | |
| L);auto * |
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
| } | ||
| } | ||
|
|
||
| void test_file_cache_memory_storage(io::FileCacheType cache_type) { |
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 'test_file_cache_memory_storage' exceeds recommended size/complexity thresholds [readability-function-size]
void test_file_cache_memory_storage(io::FileCacheType cache_type) {
^Additional context
be/test/io/cache/block_file_cache_test.cpp:659: 389 lines including whitespace and comments (threshold 80)
void test_file_cache_memory_storage(io::FileCacheType cache_type) {
^|
run buildall |
1 similar comment
|
run buildall |
TPC-H: Total hot run time: 38581 ms |
TPC-DS: Total hot run time: 187710 ms |
ClickBench: Total hot run time: 31.52 s |
|
TeamCity be ut coverage result: |
a70c120 to
342316b
Compare
|
run buildall |
TPC-H: Total hot run time: 41403 ms |
|
run buildall |
gavinchou
left a comment
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. |
TPC-DS: Total hot run time: 191044 ms |
ClickBench: Total hot run time: 32.17 s |
TODO: 1. 优化 内存池 2. 写入、读取数据内存零拷贝 3. 原始一些代码的重构(很多fs相关的内容散布在 block_file_cache.cpp 里, 需要进行额外的清理) Signed-off-by: freemandealer <freeman.zhang1992@gmail.com>
Signed-off-by: freemandealer <freeman.zhang1992@gmail.com>
Signed-off-by: freemandealer <freeman.zhang1992@gmail.com>
Signed-off-by: freemandealer <freeman.zhang1992@gmail.com>
Signed-off-by: freemandealer <freeman.zhang1992@gmail.com>
Signed-off-by: freemandealer <freeman.zhang1992@gmail.com>
2b80049 to
b78918a
Compare
|
run buildall |
|
PR approved by at least one committer and no changes requested. |
|
TeamCity be ut coverage result: |
TPC-H: Total hot run time: 41701 ms |
TPC-DS: Total hot run time: 191397 ms |
ClickBench: Total hot run time: 32.42 s |
Hastyshell
left a comment
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
…sk system (#38811) User can set file_cache_path=[{..., "storage":"memory"}] to use RAM as file cache storage.
User can set file_cache_path=[{..., "storage":"memory"}] to use RAM as file cache storage.