Avoid SerDe for aggregation query with object pool #1854

kangkaisen · 2019-09-24T07:40:29Z

For #1610 "avoid serialize and deserialize from Scannode to Aggnode".

For aggregation query, we directly pass agg object from scan node to Agg node and avoid unnecessary serialize and deserialize.

morningman · 2019-09-24T08:07:44Z

be/src/olap/aggregate_func.h

+        auto* hll = new HyperLogLog((const uint8_t*) src_slice->data);
+        dst_slice->data = reinterpret_cast<char*>(hll);
+
+        arena->track_memory(sizeof(HyperLogLog));


I think sizeof(HyperLogLog) does not reflect the real size of HLL, and even has great difference

I agree with you.
But this should be another issue, This PR don't change this point.
We need a better way to estimate the HLL and Bitmap memory usage.

imay · 2019-09-24T10:44:21Z

be/src/runtime/row_batch.cpp

    std::swap(_has_in_flight_row, other->_has_in_flight_row);
    std::swap(_num_rows, other->_num_rows);
    std::swap(_capacity, other->_capacity);
+    _agg_object_pool.swap(other->_agg_object_pool);


This swap function seems not be used, you can delete this function.

imay · 2019-09-24T10:48:16Z

be/src/runtime/row_batch.cpp

 void RowBatch::transfer_resource_ownership(RowBatch* dest) {
    dest->_auxiliary_mem_usage += _tuple_data_pool->total_allocated_bytes();
    dest->_tuple_data_pool->acquire_data(_tuple_data_pool.get(), false);
+    dest->_agg_object_pool.swap(_agg_object_pool);


The semantics is a transfer, which is not a swap logic.
If dest is empty this is right, if not this will be wrong

imay · 2019-09-24T10:53:13Z

be/src/runtime/plan_fragment_executor.cpp

            find_with_default(params.per_node_scan_ranges, scan_node->id(), no_scan_ranges);
        scan_node->set_scan_ranges(scan_ranges);
        VLOG(1) << "scan_node_Id=" << scan_node->id() << " size=" << scan_ranges.size();
+        scan_node->set_is_agg_node_child(has_agg_node);


This will not works for SCAN1 if the plan tree looks like

JOIN | ---------- | | SCAN1 AGG | SCAN2

There are other problems.
For example, if there are some HLL operation happen between agg and scan, it may cause problem.
Why not do like this?

ScanNode alway return object representation

All Functions or type handler can handle two representation.

If we do like that, we will not depend how plan tree is like.

imay · 2019-09-24T10:55:37Z

be/src/olap/schema_change.cpp

    row_cursor.allocate_memory_for_string_type(_tablet->tablet_schema());
    while (_heap.size() > 0) {
-        init_row_with_others(&row_cursor, *(_heap.top().row_cursor), arena.get());
+        init_row_with_others(&row_cursor, *(_heap.top().row_cursor), arena.get(), agg_object_pool.get());


reset object pool like arena?

imay · 2019-09-24T10:55:58Z

be/src/olap/task/engine_checksum_task.cpp

    uint32_t row_checksum = 0;
    while (true) {
-        OLAPStatus res = reader.next_row_with_aggregation(&row, arena.get(), &eof);
+        OLAPStatus res = reader.next_row_with_aggregation(&row, arena.get(), agg_object_pool.get(), &eof);


reset object pool like arena

imay · 2019-09-24T11:00:13Z

be/src/olap/row_cursor.h

    size_t _variable_len;

+    // for agg query, we don't need to finalize when scan agg object data
+    bool need_agg_finalize = true;


Is it a good option to put this flag here? This struct only a row, it should not care how to assemble itself

imay · 2019-09-24T11:02:06Z

be/src/olap/reader.cpp

    _merged_rows += merged_count;
-    agg_finalize_row(_value_cids, row_cursor, arena);
+    // For agg query, we don't need finalize agg object and directly pass agg object to agg node
+    if (row_cursor->is_need_agg_finalize()) {


better to put flag in ReaderParams

imay · 2019-09-24T11:04:20Z

be/src/olap/merger.cpp

    int64_t output_rows = 0;
    while (true) {
        Arena arena;
+        ObjectPool objectPool;


Wow, that's bad. we will allocate a new arena and object pool for each row we read. This is very bad for performance.

I will fix this issue in another PR.

imay · 2019-09-25T14:57:53Z

@kangkaisen

I suddenly remembered that you need to modify HLL related functions in be/src/exprs/aggregate_functions.cpp. Otherwise, it will cause BE Crash during the upgrade process.

kangkaisen · 2019-09-26T02:46:47Z

@kangkaisen

I suddenly remembered that you need to modify HLL related functions in be/src/exprs/aggregate_functions.cpp. Otherwise, it will cause BE Crash during the upgrade process.

Thanks for your reminder. update.

imay · 2019-09-26T03:00:38Z

be/src/udf/udf.cpp

+    // zero size means the src input is a HyperLogLog object
+    if (other.len == 0) {
+        auto* hll = reinterpret_cast<doris::HyperLogLog*>(other.ptr);
+        uint8_t* ptr = ctx->allocate(doris::HLL_COLUMN_DEFAULT_LEN);


Because there is a private member variable named ptr and len, better to rename this ptr to other_ptr.

imay

LGTM

morningman

LGTM

morningman requested changes Sep 24, 2019

View reviewed changes

imay requested changes Sep 24, 2019

View reviewed changes

Avoid SerDe for aggregation query with object pool

c0c283b

kangkaisen force-pushed the avoid-serde branch from d1a9144 to c0c283b Compare September 25, 2019 13:11

Fix compatibility issue

cc37df3

imay reviewed Sep 26, 2019

View reviewed changes

rename

5a8a4fc

imay approved these changes Sep 26, 2019

View reviewed changes

morningman approved these changes Sep 26, 2019

View reviewed changes

morningman merged commit b246d93 into apache:master Sep 26, 2019

swjtu-zhanglei pushed a commit to swjtu-zhanglei/incubator-doris that referenced this pull request Jul 25, 2023

(selectdb-cloud) Fix compilation error of cache UT (apache#1854)

937b984

Avoid SerDe for aggregation query with object pool #1854

Avoid SerDe for aggregation query with object pool #1854

Uh oh!

Conversation

kangkaisen commented Sep 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imay commented Sep 25, 2019

Uh oh!

kangkaisen commented Sep 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imay left a comment

Choose a reason for hiding this comment

Uh oh!

morningman left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants