David Ribeiro Alves has posted comments on this change. Change subject: Memory tracking for result tracker ......................................................................
Patch Set 9: (5 comments) http://gerrit.cloudera.org:8080/#/c/3627/9/src/kudu/rpc/result_tracker.cc File src/kudu/rpc/result_tracker.cc: Line 75: mem_tracker_->Consume(sizeof(ClientState)); > If we want to make ClientState memory tracking accurate, we need to account Done Line 85: mem_tracker_->Consume(sizeof(CompletionRecord)); > Same issues with CompletionRecord and its nested vector of OngoingRpcs (you Done Line 234: mem_tracker_->Consume(response->ByteSize()); > Should use SpaceUsed(), I think. Done Line 255: mem_tracker_->Release(sizeof(OnGoingRpcInfo)); > We've found that Release() in a loop is generally less efficient than addin I've changed how this works. as a side effect the mem tracker now gets updated only once. Not that it matters in this case since in 99.999% of the cases we'll only seen 1 of these. http://gerrit.cloudera.org:8080/#/c/3627/9/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: PS9, Line 89: if (id != 0) { : StrAppend(&id_str, " ", id); : } > Don't need this; each of these memtrackers is already "disambiguated" by vi Done re the id. don't you need a separate memtracker to be able to follow it's specific memory consumption? knowing how much memory is being used by the result tracker specifically seems pretty important to be able to tweak gc times. -- To view, visit http://gerrit.cloudera.org:8080/3627 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625 Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
