github-actions[bot] commented on code in PR #61327:
URL: https://github.com/apache/doris/pull/61327#discussion_r2935406114
##########
be/src/load/memtable/memtable_flush_executor.cpp:
##########
@@ -194,17 +194,17 @@ Status FlushToken::_do_flush_memtable(MemTable* memtable,
int32_t segment_id, in
memtable->resource_ctx()->memory_context()->mem_tracker()->write_tracker());
SCOPED_CONSUME_MEM_TRACKER(memtable->mem_tracker());
- DEFER_RELEASE_RESERVED();
+ // DEFER_RELEASE_RESERVED();
Review Comment:
**[Issue: Commented-out code should be deleted or properly reverted]**
Commenting out code is not an acceptable way to disable functionality. Per
Doris coding standards, code should either be fully present (active) or fully
removed. Commented-out code creates confusion about whether it's intentional
and temporary or accidental.
If this memory reservation logic is no longer needed (because PODArray's
allocator now tracks at the Allocator level), these lines should be deleted
entirely. If there's an intent to re-enable later, consider a config flag or at
minimum add a TODO comment explaining the plan.
Also note: commenting out `dec_flushing_task()` without commenting out the
rest of the code that calls `inc_flushing_task()` (in `_try_reserve_memory`)
means the counter logic becomes inconsistent if anyone calls
`_try_reserve_memory` from another path. The functions `_try_reserve_memory`,
`inc_flushing_task`, `dec_flushing_task`, and
`check_and_inc_has_any_flushing_task` in `memtable_flush_executor.h` are now
effectively dead code — they should be cleaned up as well if this is permanent.
##########
be/src/core/pod_array.h:
##########
@@ -489,48 +388,57 @@ class PODArray : public PODArrayBase<sizeof(T),
initial_bytes, TAllocator, pad_r
/// Same as resize, but zeroes new elements.
void resize_fill(size_t n) {
size_t old_size = this->size();
- const auto new_size = this->byte_size(n);
if (n > old_size) {
this->reserve(n);
- this->reset_resident_memory(this->c_start + new_size);
memset(this->c_end, 0, this->byte_size(n - old_size));
}
- this->c_end = this->c_start + new_size;
+ this->c_end = this->c_start + this->byte_size(n);
}
- /// reset the array capacity
- /// fill the new additional elements using the value
void resize_fill(size_t n, const T& value) {
size_t old_size = this->size();
- const auto new_size = this->byte_size(n);
if (n > old_size) {
this->reserve(n);
- this->reset_resident_memory(this->c_start + new_size);
std::fill(t_end(), t_end() + n - old_size, value);
}
- this->c_end = this->c_start + new_size;
+ this->c_end = this->c_start + this->byte_size(n);
}
template <typename U, typename... TAllocatorParams>
void push_back(U&& x, TAllocatorParams&&... allocator_params) {
- if (UNLIKELY(this->c_end + sizeof(T) > this->c_res_mem)) { //
c_res_mem <= c_end_of_storage
- if (UNLIKELY(this->c_end + sizeof(T) > this->c_end_of_storage)) {
-
this->reserve_for_next_size(std::forward<TAllocatorParams>(allocator_params)...);
- }
- this->reset_resident_memory_impl(this->c_end + this->byte_size(1));
+ if (UNLIKELY(this->c_end + sizeof(T) > this->c_end_of_storage)) {
+
this->reserve_for_next_size(std::forward<TAllocatorParams>(allocator_params)...);
}
new (t_end()) T(std::forward<U>(x));
this->c_end += this->byte_size(1);
}
+ template <typename U, typename... TAllocatorParams>
+ void add_num_element(U&& x, uint32_t num, TAllocatorParams&&...
allocator_params) {
+ if (num != 0) {
Review Comment:
**[Issue: Dead code — `add_num_element` has zero callers in the codebase]**
This new function is not called anywhere in the codebase. Adding unused code
increases maintenance burden and contradicts the principle of keeping changes
minimal and focused. If this is intended for future use, it should be added in
the PR that actually uses it. Please either add callers or remove this function.
##########
be/src/core/pod_array.h:
##########
@@ -489,48 +388,57 @@ class PODArray : public PODArrayBase<sizeof(T),
initial_bytes, TAllocator, pad_r
/// Same as resize, but zeroes new elements.
void resize_fill(size_t n) {
size_t old_size = this->size();
- const auto new_size = this->byte_size(n);
if (n > old_size) {
this->reserve(n);
- this->reset_resident_memory(this->c_start + new_size);
memset(this->c_end, 0, this->byte_size(n - old_size));
}
- this->c_end = this->c_start + new_size;
+ this->c_end = this->c_start + this->byte_size(n);
}
- /// reset the array capacity
- /// fill the new additional elements using the value
void resize_fill(size_t n, const T& value) {
size_t old_size = this->size();
- const auto new_size = this->byte_size(n);
if (n > old_size) {
this->reserve(n);
- this->reset_resident_memory(this->c_start + new_size);
std::fill(t_end(), t_end() + n - old_size, value);
}
- this->c_end = this->c_start + new_size;
+ this->c_end = this->c_start + this->byte_size(n);
}
template <typename U, typename... TAllocatorParams>
void push_back(U&& x, TAllocatorParams&&... allocator_params) {
- if (UNLIKELY(this->c_end + sizeof(T) > this->c_res_mem)) { //
c_res_mem <= c_end_of_storage
- if (UNLIKELY(this->c_end + sizeof(T) > this->c_end_of_storage)) {
-
this->reserve_for_next_size(std::forward<TAllocatorParams>(allocator_params)...);
- }
- this->reset_resident_memory_impl(this->c_end + this->byte_size(1));
+ if (UNLIKELY(this->c_end + sizeof(T) > this->c_end_of_storage)) {
+
this->reserve_for_next_size(std::forward<TAllocatorParams>(allocator_params)...);
}
new (t_end()) T(std::forward<U>(x));
this->c_end += this->byte_size(1);
}
+ template <typename U, typename... TAllocatorParams>
+ void add_num_element(U&& x, uint32_t num, TAllocatorParams&&...
allocator_params) {
+ if (num != 0) {
+ const auto growth_size = this->byte_size(num);
+ if (UNLIKELY(this->c_end + growth_size > this->c_end_of_storage)) {
+ this->reserve(this->size() + num);
+ }
+ std::fill(t_end(), t_end() + num, x);
+ this->c_end = this->c_end + growth_size;
+ }
+ }
+
+ template <typename U, typename... TAllocatorParams>
+ void add_num_element_without_reserve(U&& x, uint32_t num,
+ TAllocatorParams&&...
allocator_params) {
Review Comment:
**[Issue: Dead code + minor issues in `add_num_element_without_reserve`]**
1. **Dead code**: Like `add_num_element`, this function has zero callers
anywhere in the codebase. It should be removed or justified.
2. **Missing `num == 0` guard**: Unlike `add_num_element` which guards `if
(num != 0)`, this function will call `std::fill(t_end(), t_end() + 0, x)` with
`num=0`, which is a no-op but inconsistent with the sibling function.
3. **Unused template parameter pack**: `TAllocatorParams&&...
allocator_params` is declared but never used in the function body. This should
be removed to avoid confusion.
4. **Style inconsistency**: Uses `sizeof(T) * num` while the rest of
PODArray consistently uses `this->byte_size(num)` for the same calculation.
Minor but worth keeping consistent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
