Huaisi Xu has submitted this change and it was merged. Change subject: IMPALA-3034: Verify all consumed memory of a MemTracker is always released at destruction time. ......................................................................
IMPALA-3034: Verify all consumed memory of a MemTracker is always released at destruction time. By adding a DCHECK at the destructor of MemTracker to verify that all its consumed memory has been released, various problems are found in the code. This change fixes all these issues. The first problem has to do with a memory leak in DataStreamRecvr. DataStreamRecvr::Close() is responsible for shutting down all the sender queues and all the row batches in them. However, it tears down the queues in the wrong order. Specifically, it calls DataStreamRecvr::SenderQueue::Close() on each sender queue before unregistering them. There exists a window between the shut down of the queues and their unregistration which allows other threads to enqueue into already closed queues. This change fixes the problem by first unregistering the sender queues before shutting them down. This guarantees that no new row batch will be created once it has been unregistered so DataStreamRecvr::SenderQueue::Close() will not leak any row batches. The second problem is an accounting problem in which RuntimeFilter fails to call Release() on query_mem_tracker_ for all the memory it has consumed. Specifically, if ProcessBuildInput() fails in ConstructBuildSide() of a PHJ node, the outstanding bloom filters allocated in AllocatedRuntimeFilters() will not be added to produced_filters_ set. Since RunFilterBank::Close() only calls MemTracker::Release() on filters in produced_filters_ and consumed_filters_ sets, it will miss these 'orphaned' filters. Fortunately, all allocated filters are kept in the object pool so there is no memory leak. Since the amount of memory consumed is already tracked by 'memory_allocated_', this change fixes the problem by calling MemTracker::Release() with 'memory_allocated_' in RuntimeFilterbank::Close(). The third problem is that DataStreamSender::Send() frees local allocations before the expression evaluation instead of after the evaluation. This leaves some local allocations hanging around longer than needed. This change moves the location in which local allocations are freed. In addition, this change also fixes the order of two fields 'runtime_state_' and 'sink_' in PlanFragmentExecutor. The wrong order can lead lead to freed memory being accessed in the destructor of MemTracker and potentially other places. Change-Id: Ie87fd4cd10a7c7da806ed95eeb262ed51e74ec7b Reviewed-on: http://gerrit.cloudera.org:8080/2269 Reviewed-by: Henry Robinson <[email protected]> Tested-by: Internal Jenkins (cherry picked from commit 903920ef66fd7c209dacc970c7ef0321c6180093) Reviewed-on: http://gerrit.cloudera.org:8080/2423 Reviewed-by: Juan Yu <[email protected]> Tested-by: Huaisi Xu <[email protected]> --- M be/src/runtime/data-stream-recvr.cc M be/src/runtime/data-stream-sender.cc M be/src/runtime/data-stream-test.cc M be/src/runtime/mem-tracker-test.cc M be/src/runtime/mem-tracker.cc M be/src/runtime/plan-fragment-executor.h 6 files changed, 32 insertions(+), 15 deletions(-) Approvals: Huaisi Xu: Verified Juan Yu: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/2423 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ie87fd4cd10a7c7da806ed95eeb262ed51e74ec7b Gerrit-PatchSet: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.2.0_5.4.x Gerrit-Owner: Huaisi Xu <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Huaisi Xu <[email protected]> Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Juan Yu <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]>
