Omid Shahidi has posted comments on this change. ( http://gerrit.cloudera.org:8080/18798 )
Change subject: IMPALA-6684: Fix untracked memory in KRPC ...................................................................... Patch Set 6: (12 comments) > Patch Set 6: > > (8 comments) > > add end-end unit test added end-to-end test 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@138 PS6, Line 138: uint8_t* input = const_cast<uint8_t*>( > const_cast shouldn't be needed here Done http://gerrit.cloudera.org:8080/#/c/18798/6/be/src/benchmarks/row-batch-serialize-benchmark.cc@140 PS6, Line 140: uint8_t* compressed_output = const_cast<uint8_t*>( > const_cast shouldn't be needed here Done 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 Done http://gerrit.cloudera.org:8080/#/c/18798/6/be/src/runtime/krpc-data-stream-sender.h File be/src/runtime/krpc-data-stream-sender.h: http://gerrit.cloudera.org:8080/#/c/18798/6/be/src/runtime/krpc-data-stream-sender.h@30 PS6, Line 30: #include "exec/data-sink.h" > Move this back where it was before Done 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@71 PS6, Line 71: std::unique_ptr<FreePool> free_pool_; > Is unique_lock actually required here? Seem the locking here is fairly simp >From our offline discussion, we can use lock_guard which has zero overhead and >only maintains one state 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()? free-pool.h:99 checks this for us in the free pool free() function 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 Done 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 f Changed function signature which now gets passed the size and a boolean flag (for_compression) and changes the correct buffer and its length and capacity accordingly 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@299 PS6, Line 299: int64_t* tuple_data_stats_counter, int64_t* compression_scratch_stats_counter, bool full_dedup) { > line too long (101 > 90) Done http://gerrit.cloudera.org:8080/#/c/18798/6/be/src/runtime/row-batch.cc@306 PS6, Line 306: // bool full_dedup = UseFullDedup(); > delete Done 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 Done 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 accor Done -- 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 <omid.shahidi.2...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Omid Shahidi <omid.shahidi.2...@gmail.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Wed, 17 Aug 2022 03:01:41 +0000 Gerrit-HasComments: Yes
