morningman commented on code in PR #9301:
URL: https://github.com/apache/incubator-doris/pull/9301#discussion_r861422597
##########
be/src/olap/tablet_manager.cpp:
##########
@@ -74,13 +74,14 @@
DEFINE_GAUGE_METRIC_PROTOTYPE_5ARG(tablet_meta_mem_consumption, MetricUnit::BYTE
mem_consumption, Labels({{"type",
"tablet_meta"}}));
TabletManager::TabletManager(int32_t tablet_map_lock_shard_size)
- : _mem_tracker(MemTracker::create_tracker(-1, "TabletManager", nullptr,
+ : _mem_tracker(MemTracker::create_tracker(-1, "TabletMeta", nullptr,
Review Comment:
Why change this name?
##########
be/src/olap/tablet_schema.cpp:
##########
@@ -309,16 +309,21 @@ void TabletColumn::init_from_pb(const ColumnPB& column) {
_has_default_value = column.has_default_value();
if (_has_default_value) {
_default_value = column.default_value();
+ } else {
+ _default_value = "";
Review Comment:
How to express `null` and `empty string`?
##########
be/src/service/internal_service.cpp:
##########
@@ -168,6 +167,7 @@ void
PInternalServiceImpl<T>::tablet_writer_add_batch(google::protobuf::RpcContr
// exhausted, so we put this to a local thread pool to process
int64_t submit_task_time_ns = MonotonicNanos();
_tablet_worker_pool.offer([cntl_base, request, response, done,
submit_task_time_ns, this]() {
+ SCOPED_SWITCH_BTHREAD();
Review Comment:
Why add here?
I think the thread in `_tablet_worker_pool` is not bthread?
##########
be/src/runtime/mem_tracker.cpp:
##########
@@ -318,7 +326,7 @@ bool MemTracker::gc_memory(int64_t max_consumption) {
if (pre_gc_consumption < max_consumption) return false;
int64_t curr_consumption = pre_gc_consumption;
- const int64_t EXTRA_BYTES_TO_FREE = 4L * 1024L * 1024L * 1024L; //
TODO(zxy) Consider as config
+ const int64_t EXTRA_BYTES_TO_FREE = 4L * 1024L * 1024L * 1024L;
Review Comment:
Add comment for this magic number
##########
be/src/olap/task/engine_alter_tablet_task.cpp:
##########
@@ -29,7 +29,7 @@ EngineAlterTabletTask::EngineAlterTabletTask(const
TAlterTabletReqV2& request)
: _alter_tablet_req(request) {
_mem_tracker = MemTracker::create_tracker(
config::memory_limitation_per_thread_for_schema_change_bytes,
- fmt::format("EngineAlterTabletTask: {}-{}",
+ fmt::format("EngineAlterTabletTask:baseTabletId={}-newTabletId{}",
Review Comment:
```suggestion
fmt::format("EngineAlterTabletTask:baseTabletId={}-newTabletId={}",
```
##########
be/src/exec/tablet_sink.cpp:
##########
@@ -50,7 +50,7 @@ NodeChannel::NodeChannel(OlapTableSink* parent, IndexChannel*
index_channel, int
_tuple_data_buffer_ptr = &_tuple_data_buffer;
}
_node_channel_tracker =
- MemTracker::create_tracker(-1, "NodeChannel" +
tls_ctx()->thread_id_str());
+ MemTracker::create_tracker(-1, "NodeChannel:" +
std::to_string(_index_channel->_index_id));
Review Comment:
Why using `_index_channel->_index_id`, this will cause all
`_node_channel_tracker` using same name.
##########
be/src/http/action/compaction_action.h:
##########
@@ -42,7 +42,7 @@ class CompactionAction : public HttpHandler {
CompactionAction(CompactionActionType type) : _type(type) {
_compaction_mem_tracker =
type == RUN_COMPACTION ? MemTracker::create_tracker(-1,
"ManualCompaction", nullptr,
-
MemTrackerLevel::TASK)
+
MemTrackerLevel::INSTANCE)
Review Comment:
What is different between TASK and INSTANCE?
##########
be/src/olap/task/engine_batch_load_task.cpp:
##########
@@ -54,7 +54,9 @@ EngineBatchLoadTask::EngineBatchLoadTask(TPushReq& push_req,
std::vector<TTablet
_res_status(res_status) {
_download_status = Status::OK();
_mem_tracker = MemTracker::create_tracker(
- -1, fmt::format("{}: {}", _push_req.push_type,
std::to_string(_push_req.tablet_id)),
+ -1,
+ fmt::format("EngineBatchLoadTask:pushType{}-tabletId{}",
_push_req.push_type,
Review Comment:
```suggestion
fmt::format("EngineBatchLoadTask:pushType: {}-tabletId: {}",
_push_req.push_type,
```
##########
be/src/runtime/mem_tracker.h:
##########
@@ -97,8 +104,9 @@ class MemTracker {
// Gets a shared_ptr to the "process" tracker, creating it if necessary.
static std::shared_ptr<MemTracker> get_process_tracker();
static MemTracker* get_raw_process_tracker();
- // Gets a shared_ptr to the "brpc server" tracker, creating it if
necessary.
- static std::shared_ptr<MemTracker> get_brpc_server_tracker();
+ // Get a temporary tracker with a specified label, and the tracker will be
created when the label is first get.
+ // Temporary trackers are not automatically destructed, which is usually
used for debugging.
+ static std::shared_ptr<MemTracker> get_temporary_mem_tracker(const
std::string& label);
Review Comment:
where is `get_brpc_server_tracker`?
##########
be/src/olap/tablet_manager.h:
##########
@@ -205,6 +205,9 @@ class TabletManager {
// trace the memory use by meta of tablet
std::shared_ptr<MemTracker> _mem_tracker;
+ // The logical memory given by sizeof is less than the actual memory
allocated by tcmalloc,
+ // Because the minimum memory allocation unit of tcmalloc is page, memory
fragmentation will occur.
Review Comment:
I don't understand this logic.
Could you explain more?
##########
be/src/runtime/mem_pool.cpp:
##########
@@ -41,7 +41,7 @@ const int MemPool::MAX_CHUNK_SIZE;
const int MemPool::DEFAULT_ALIGNMENT;
uint32_t MemPool::k_zero_length_region_ alignas(std::max_align_t) =
MEM_POOL_POISON;
-MemPool::MemPool(MemTracker* mem_tracker)
+MemPool::MemPool(const std::shared_ptr<MemTracker>& mem_tracker)
Review Comment:
Why changing mem_tracker from raw ptr to shared_ptr?
##########
be/src/runtime/tcmalloc_hook.h:
##########
@@ -23,32 +23,40 @@
#include "runtime/thread_context.h"
-// Notice: modify the command in New/Delete Hook should be careful enough!,
-// and should be as simple as possible, otherwise it may cause weird errors.
E.g:
-// 1. The first New Hook call of the process may be before some variables of
-// the process are initialized.
-// 2. Allocating memory in the Hook command causes the Hook to be entered
again,
-// infinite recursion.
-// 3. TCMalloc hook will be triggered during the process of
initializing/Destructor
-// memtracker shared_ptr, Using the object pointed to by this memtracker
shared_ptr
-// in TCMalloc hook may cause crash.
-// 4. Modifying additional thread local variables in ThreadContext
construction and
-// destructor to control the behavior of consume can lead to unexpected
behavior,
-// like this: if (LIKELY(doris::start_thread_mem_tracker)) {
-void new_hook(const void* ptr, size_t size) {
- doris::tls_ctx()->consume_mem(tc_nallocx(size, 0));
-}
-
-void delete_hook(const void* ptr) {
- doris::tls_ctx()->release_mem(tc_malloc_size(const_cast<void*>(ptr)));
-}
-
-void init_hook() {
- MallocHook::AddNewHook(&new_hook);
- MallocHook::AddDeleteHook(&delete_hook);
-}
-
-void destroy_hook() {
- MallocHook::RemoveNewHook(&new_hook);
- MallocHook::RemoveDeleteHook(&delete_hook);
-}
+namespace doris {
+
+class TcmallocHook {
+public:
+ // Notice: modify the command in New/Delete Hook should be careful enough!,
+ // and should be as simple as possible, otherwise it may cause weird
errors. E.g:
+ // 1. The first New Hook call of the process may be before some variables
of
+ // the process are initialized.
+ // 2. Allocating memory in the Hook command causes the Hook to be entered
again,
+ // infinite recursion.
+ // 3. TCMalloc hook will be triggered during the process of
initializing/Destructor
+ // memtracker shared_ptr, Using the object pointed to by this memtracker
shared_ptr
+ // in TCMalloc hook may cause crash.
+ // 4. Modifying additional thread local variables in ThreadContext
construction and
+ // destructor to control the behavior of consume can lead to unexpected
behavior,
+ // like this: if (LIKELY(doris::start_thread_mem_tracker)) {
+ static void new_hook(const void* ptr, size_t size) {
Review Comment:
Why change to static?
##########
be/src/olap/tablet_schema.cpp:
##########
@@ -408,8 +428,8 @@ void TabletSchema::init_from_pb(const TabletSchemaPB&
schema) {
_num_null_columns = 0;
_cols.clear();
_field_name_to_index.clear();
+ TabletColumn column;
for (auto& column_pb : schema.column()) {
Review Comment:
You use `std::move(column)` below, so you have to put `TabletColumn colum`
in the loop.
--
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: commits-unsubscr...@doris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org
