[Impala-ASF-CR] Draft: Add JSON output to MemTracker::LogUsage()

["Csaba Ringhofer (Code Review)" <ge...@cloudera.org>]

Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/8955 )

Change subject: Draft: Add JSON output to MemTracker::LogUsage()
......................................................................


Patch Set 1:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/8955/1/be/src/runtime/mem-tracker.h
File be/src/runtime/mem-tracker.h:

http://gerrit.cloudera.org:8080/#/c/8955/1/be/src/runtime/mem-tracker.h@339
PS1, Line 339:   void LogUsage(int max_recursive_depth, std::stringstream* ss, const std::string& prefix,
             :       rapidjson::Value* val, rapidjson::Document* doc, int64_t* logged_consumption);
             :   /// Convenience wrapper around the previous declaration to make it easier to use in
             :   /// DCHECKs.
             :   std::string LogUsage(int max_recursive_depth);
I saw a place where the old interface is used with prefix and logged_consumption: https://github.com/apache/impala/blob/3302abaa32cf3290cb038fbc1dbd0a821f5cbbc7/be/src/runtime/bufferpool/reservation-tracker-test.cc#L543


http://gerrit.cloudera.org:8080/#/c/8955/1/be/src/runtime/mem-tracker.cc
File be/src/runtime/mem-tracker.cc:

http://gerrit.cloudera.org:8080/#/c/8955/1/be/src/runtime/mem-tracker.cc@256
PS1, Line 256: void MemTracker::LogUsage(int max_recursive_depth, std::stringstream* ss,
I think that this became a bit too complicated for a log function. My idea for an alternative solution is creating a struct with the variables we want to log, and split this function to two somewhat less complex ones: one that walks the tree and collects these structs (essentially creating a snapshot), and one that serializes them into string/json. While this would mean some extra copies, a potential performance benefit could be that string formatting would happen when no child_trackers_lock_ is locked. If the structs would have a depth field, then it would be enough to collect them to a vector (without loosing the tree structure).


http://gerrit.cloudera.org:8080/#/c/8955/1/be/src/runtime/mem-tracker.cc@301
PS1, Line 301:   val->AddMember("gauges", gauges, doc->GetAllocator());
if (val != nullptr)


http://gerrit.cloudera.org:8080/#/c/8955/1/be/src/runtime/mem-tracker.cc@309
PS1, Line 309:   string child_trackers_usage;
This variable is no longer used.


http://gerrit.cloudera.org:8080/#/c/8955/1/be/src/runtime/mem-tracker.cc@315
PS1, Line 315:     val->AddMember("children", children, doc->GetAllocator());
if (val != nullptr)


http://gerrit.cloudera.org:8080/#/c/8955/1/be/src/runtime/mem-tracker.cc@340
PS1, Line 340:   vector<string> usage_strings;
This variable is no longer used.



-- 
To view, visit http://gerrit.cloudera.org:8080/8955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0a59c39b0a85c78f1e8db10a93cbb814dc6adfa0
Gerrit-Change-Number: 8955
Gerrit-PatchSet: 1
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Mar 2018 13:24:02 +0000
Gerrit-HasComments: Yes