Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18798 )
Change subject: IMPALA-6684: Fix untracked memory in KRPC ...................................................................... Patch Set 6: (8 comments) add end-end unit test http://gerrit.cloudera.org:8080/#/c/18798/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18798/6//COMMIT_MSG@22 PS6, Line 22: Passed core tests and ran a single node benchmark which shows no : regression. Ported row-batch-serialize-test and : row-batch-serialize-benchmark to test the new BE using KRPC. Collected : query-profile, heap growth, and memory usage log showing untracked : memory decreased by 1/2. separate each tasks as: Testing: - Passed core tests. - Ran a single node benchmark which shows no regression. - Updated row-batch-serialize-test and row-batch-serialize-benchmark to test the row-batch serialization used by KRPC. - Manually collected query-profile, heap growth, and memory usage log showing untracked memory decreased by 1/2. http://gerrit.cloudera.org:8080/#/c/18798/6/be/src/benchmarks/row-batch-serialize-benchmark.cc File be/src/benchmarks/row-batch-serialize-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/18798/6/be/src/benchmarks/row-batch-serialize-benchmark.cc@437 PS6, Line 437: argc, argv, true add impala::TestInfo::BE_TEST http://gerrit.cloudera.org:8080/#/c/18798/6/be/src/runtime/row-batch.h File be/src/runtime/row-batch.h: http://gerrit.cloudera.org:8080/#/c/18798/6/be/src/runtime/row-batch.h@105 PS6, Line 105: mem_allocator_->Free(reinterpret_cast<uint8_t*>(tuple_data_)) Should we check if tuple_data_ is not nullptr before calling Free()? http://gerrit.cloudera.org:8080/#/c/18798/6/be/src/runtime/row-batch.h@114 PS6, Line 114: inline Status AllocateTraceableBuffer Add a new header file row-ratch.inline.h and put this inline function body into the new header file. http://gerrit.cloudera.org:8080/#/c/18798/6/be/src/runtime/row-batch.h@115 PS6, Line 115: char** buffer_ptr, int64_t* buffer_length, int64_t* buffer_capacity Define two allocation functions to replace AllocateTraceableBuffer(), one for tuple_data, another for compression_scratch so that we don't need to pass parameters except 'size'. http://gerrit.cloudera.org:8080/#/c/18798/6/be/src/runtime/row-batch.cc File be/src/runtime/row-batch.cc: http://gerrit.cloudera.org:8080/#/c/18798/6/be/src/runtime/row-batch.cc@306 PS6, Line 306: // bool full_dedup = UseFullDedup(); delete http://gerrit.cloudera.org:8080/#/c/18798/6/be/src/runtime/row-batch.cc@322 PS6, Line 322: &output_batch->tuple_data_, : &output_batch->tuple_data_length_, &output_batch->tuple_data_capacity_ change function signature so that we don't need to pass member variables as parameters. http://gerrit.cloudera.org:8080/#/c/18798/6/be/src/runtime/row-batch.cc@393 PS6, Line 393: Thrift implementation for Serialization using TRowBatch. : /// Benchmarks (be/src/benchmarks/row-batch-serialize-benchmark.cc) and tests : /// (be/src/runtime/row-batch-serialize-test.cc) for serialization use TRowBatch and : /// Thrift so we need to keep the old implementation so they don't fail. You already replace TRowBatch in those two files, update the comments accordingly. We can clean the Thrift related code later. -- To view, visit http://gerrit.cloudera.org:8080/18798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2ba2b907ce4f275a7a1fb8cf75453c7003eb4b82 Gerrit-Change-Number: 18798 Gerrit-PatchSet: 6 Gerrit-Owner: Omid Shahidi <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Omid Shahidi <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Fri, 12 Aug 2022 04:03:55 +0000 Gerrit-HasComments: Yes
