Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8914 )
Change subject: IMPALA-6193: Track memory of incoming data streams ...................................................................... Patch Set 5: (8 comments) Looking pretty good, remaining comments are mainly about making the accounting as precise as possible. http://gerrit.cloudera.org:8080/#/c/8914/5/be/src/rpc/impala-service-pool.cc File be/src/rpc/impala-service-pool.cc: http://gerrit.cloudera.org:8080/#/c/8914/5/be/src/rpc/impala-service-pool.cc@58 PS5, Line 58: DCHECK(mem_tracker_ != nullptr); nit: extra indentation. http://gerrit.cloudera.org:8080/#/c/8914/5/be/src/runtime/krpc-data-stream-mgr.cc File be/src/runtime/krpc-data-stream-mgr.cc: http://gerrit.cloudera.org:8080/#/c/8914/5/be/src/runtime/krpc-data-stream-mgr.cc@172 PS5, Line 172: incoming_request_tracker_->Release(rpc_context->GetTransferSize()); I generally think it's better to Consume() before Release()ing to avoid undercounting. We could actually avoid the problem I think if we make use of the fact that the two trackers share a parent and use ConsumeLocal() and ReleaseLocal() so that the offsetting increments and decrements don't modify consumption further up in the tree. E.g. DCHECK_EQ(incoming_request_tracker_->parent(), mem_tracker_->parent()); incoming_request_tracker_->ReleaseLocal(rpc_context->GetTransferSize(), mem_tracker_->parent()); mem_tracker_->ConsumeLocal(rpc_context->GetTransferSize(), mem_tracker_->parent()); As a side-effect we'd reduce contention on the MemTrackers further up the tree. http://gerrit.cloudera.org:8080/#/c/8914/5/be/src/runtime/krpc-data-stream-mgr.cc@212 PS5, Line 212: incoming_request_tracker_->Release(rpc_context->GetTransferSize()); Has the memory been released at this point? http://gerrit.cloudera.org:8080/#/c/8914/5/be/src/runtime/krpc-data-stream-mgr.cc@268 PS5, Line 268: incoming_request_tracker_->Release(rpc_context->GetTransferSize()); Has the rpc_context memory been released at this point? http://gerrit.cloudera.org:8080/#/c/8914/5/be/src/runtime/krpc-data-stream-mgr.cc@387 PS5, Line 387: mem_tracker_->Release(ctx->rpc_context->GetTransferSize()); We haven't actually released the memory at this point, have we? Should we be using the DiscardTransfer() method? http://gerrit.cloudera.org:8080/#/c/8914/5/be/src/runtime/krpc-data-stream-mgr.cc@391 PS5, Line 391: mem_tracker_->Release(ctx->rpc_context->GetTransferSize()); Same question here. http://gerrit.cloudera.org:8080/#/c/8914/5/be/src/runtime/krpc-data-stream-recvr.cc File be/src/runtime/krpc-data-stream-recvr.cc: http://gerrit.cloudera.org:8080/#/c/8914/5/be/src/runtime/krpc-data-stream-recvr.cc@410 PS5, Line 410: recvr_->mem_tracker()->Release(ctx->rpc_context->GetTransferSize()); Is the memory released? http://gerrit.cloudera.org:8080/#/c/8914/5/be/src/runtime/krpc-data-stream-recvr.cc@458 PS5, Line 458: recvr_->mem_tracker()->Release(payload->rpc_context->GetTransferSize()); Same here -- To view, visit http://gerrit.cloudera.org:8080/8914 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2df1204d2483313a8a18e5e3be6cec9e402614c4 Gerrit-Change-Number: 8914 Gerrit-PatchSet: 5 Gerrit-Owner: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 17 Jan 2018 22:42:31 +0000 Gerrit-HasComments: Yes
