Adar Dembo has posted comments on this change. Change subject: Memory tracking for result tracker ......................................................................
Patch Set 13: (10 comments) http://gerrit.cloudera.org:8080/#/c/3627/13/src/kudu/rpc/result_tracker.cc File src/kudu/rpc/result_tracker.cc: PS13, Line 78: // Release all the memory for the stuff we'll delete on destruction. : for (auto& client_state : clients_) { : for (auto& completion_record : client_state.second->completion_records) { : mem_tracker_->Release(completion_record.second->memory_footprint()); : } : mem_tracker_->Release(client_state.second->memory_footprint()); : } : mem_tracker_->Release(memory_footprint()); Can we aggregate this and make just one Release() call? This isn't a hot path, but it seems easy enough to do. http://gerrit.cloudera.org:8080/#/c/3627/13/src/kudu/rpc/result_tracker.h File src/kudu/rpc/result_tracker.h: Line 228: return client_state_map_bytes_; Typically memory_footprint() methods are supposed to account for the entirety of the object, including any nested allocations. Since you're just using it for a "shallow" accounting, perhaps call it memory_footprint_excluding_clients, and add a comment explaining that? Ahh, the issue is that the ScopedMemTrackerUpdater expects a method named memory_footprint(). How about renaming all of them to memory_footprint_shallow() instead, and making it clear via comments which "deep" structures they are not accounting for? Line 254: int64_t memory_footprint() const { Likewise, memory_footprint_excluding_rpcs. Line 255: return kudu_malloc_usable_size(this) Does this method need to be synchronized in some way? Can the CompletionRecord be modified while we're executing here? Or is every access expected to hold lock_? If so, maybe add Unlocked() to the name of the method. Line 281: int64_t memory_footprint() const { Likewise, memory_footprint_excluding_completion_records. Line 282: return kudu_malloc_usable_size(this) + completion_record_map_bytes_; Do we need to synchronize access to completion_record_map_bytes_? PS13, Line 327: // Lock that protects access to 'clients_' and to the state contained in each : // ClientState. : // TODO consider a per-ClientState lock if we find this too coarse grained. Does this also protect access to client_state_map_bytes_? If so, should it be taken in memory_footprint()? http://gerrit.cloudera.org:8080/#/c/3627/13/src/kudu/rpc/rpc-stress-test.cc File src/kudu/rpc/rpc-stress-test.cc: Line 259: usleep(100 * 1000); Nit: Can we use SleepFor() instead? It's more idiomatic for Kudu, and makes the units a little clearer. Separately, why do we need to sleep here? http://gerrit.cloudera.org:8080/#/c/3627/9/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: PS9, Line 89: } : : } / > Done re the id. Hmm, OK, I guess that's true. Admittedly a separate memtracker is more of an issue when its parent is the tablet memtracker, as then a server can have hundreds or thousands additional memtrackers, which muddies the memory UI view significantly. http://gerrit.cloudera.org:8080/#/c/3627/13/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: Line 102: result_tracker_(new rpc::ResultTracker(CreateMemTrackerForForResultTracker(mem_tracker_))), How about: result_tracker_(new rpc::ResultTracker(MemTracker::CreateTracker(-1, "result-tracker", mem_tracker_))), Or is that too long? -- 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: 13 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