[Profile] Make running profile clearer and more intuitive to improve usability #3405
Conversation
… usability
This CL mainly made the following modifications:
1. Delete Invalid MemoryUsed Counter and Add PeakMemUsage in each exec node and datastreamsender
2. Add intent in child execnode profile，make it is easily to know the relationship between execnode
3. Del _is_result_order we not support any more in olap_scan_node.h and olap_scan_node.cpp
4. Add scan_disk method to olap_scanner to fix the counter _num_disks_accessed_counter
5. Now we do not use buffer pool to read and write disk, so annotation eadio counter and writeio counter code
#ISSUE 3365
| } | ||
|
|
||
| if (!node->_children.empty()) { | ||
| node->runtime_profile()->add_child(node->_children[0]->runtime_profile(), false, NULL); |
There was a problem hiding this comment.
It seem that the false here is to distinguish child[0] and other children.
Are you sure this is more readable? You can put the result screenshots here to explain.
There was a problem hiding this comment.
after change the true, we can eaily know olap_scan_node is child of aggregation_node.
be/src/exec/olap_scan_node.cpp
Outdated
| _olap_scanners.push_back(scanner); | ||
|
|
||
| if (disk_set.find(scanner->scan_disk()) == disk_set.end()) { | ||
| disk_set.insert(scanner->scan_disk()); |
There was a problem hiding this comment.
disk_set is a set, so no need to find and insert, just insert is OK.
There was a problem hiding this comment.
yes，i will change the code。
| } | ||
| } | ||
| } | ||
| COUNTER_SET(_num_disks_accessed_counter, static_cast<int64_t>(disk_set.size())); |
There was a problem hiding this comment.
What is this metric used for? If this metric is not useful, just remove it
There was a problem hiding this comment.
The metric help we now how many disk accessed in olap scan node scan data
There was a problem hiding this comment.
What can it be used for when you know how many disks accessed?
There was a problem hiding this comment.
We can know whether the tablet of a query is allocated on different disks to know whether is IO bottleneck in disk
| _rows_returned_counter = | ||
| ADD_COUNTER(_runtime_profile, "RowsReturned", TUnit::UNIT); | ||
| _memory_used_counter = | ||
| ADD_COUNTER(_runtime_profile, "MemoryUsed", TUnit::BYTES); |
There was a problem hiding this comment.
You'd better remove this field if it's not used. Btw, _memory_used_counter seems to be used by HashJoinNode::close, is it really ok to remove it?
There was a problem hiding this comment.
Hi, as we can see. this counter _memory_used_counter is only used for count mem used for _tuple_pool and _hash_tbl.
But the _tuple_pool and _hash_tbl was already track the mem used by MemTracker and counted mem used in PeakMemoryUsage counter. We do really need this counter _memory_used_counter?
So, I think keep the _memory_used_counter named MemoryUsed is confusing.
I have two different suggestions
- Delete all code use
_memory_used_counterin the future, all mem used is tracked byMemTracker, we show peakmemusage it by the counter inMemTracker. - Keep the counter
_memory_used_counter, but change the nameMemoryUsedtoMemoryUsedByHashBTAndTuplePool.
Which suggestion do you think is more suitable？@gaodayue
There was a problem hiding this comment.
Delete all code use _memory_used_counter in the future, all mem used is tracked by MemTracker, we show peakmemusage it by the counter in MemTracker.
I prefer this.
|
|
||
| /// Total number of write I/O operations issued. | ||
| RuntimeProfile::Counter* write_io_ops; | ||
| //RuntimeProfile::Counter* write_io_ops; |
There was a problem hiding this comment.
not used profile can be removed
| counters_.read_io_ops = ADD_COUNTER(child_profile, "ReadIoOps", TUnit::UNIT); | ||
| // counters_.read_wait_time = ADD_TIMER(child_profile, "ReadIoWaitTime"); | ||
| // counters_.read_io_ops = ADD_COUNTER(child_profile, "ReadIoOps", TUnit::UNIT); | ||
| counters_.bytes_read = ADD_COUNTER(child_profile, "ReadIoBytes", TUnit::BYTES); |
There was a problem hiding this comment.
not used profile can be removed.
2.delete the MemUsed counter in exec node
…he#35083 apache#35557 (apache#3405) [fix](memory) Fix BE memory info compatible with Cgroup (apache#35412) [fix](memory) Avoid frequently refresh cgroup memory info (apache#35083) [fix](libjdk) Revert support loading libjvm at runtime (apache#35557)
6 participants
This CL mainly made the following modifications:
1. Delete Invalid MemoryUsed Counter and Add PeakMemUsage in each exec node and datastreamsender
2. Add intent in child execnode profile，make it is easily to know the relationship between execnode
3. Del _is_result_order we not support any more in olap_scan_node.h and olap_scan_node.cpp
4. Add scan_disk method to olap_scanner to fix the counter _num_disks_accessed_counter
5. Now we do not use buffer pool to read and write disk, so comment out readio counter and writeio counter code
ISSUE #3365