Adar Dembo has posted comments on this change. Change subject: Only release memory to OS when overhead is unacceptable ......................................................................
Patch Set 1: (4 comments) That code was copied from Impala's memtracker implementation; I presume you've changed it there in some way as well? http://gerrit.cloudera.org:8080/#/c/1711/1/src/kudu/util/mem_tracker.cc File src/kudu/util/mem_tracker.cc: Line 56: DEFINE_int32(tcmalloc_free_bytes_percentage, 10, Hide the definition behind TCMALLOC_ENABLED? Not 100% sure about this; I feel a little weird about --dumpxml producing different output depending on how we compile. But maybe it already does, and certainly tcmalloc metrics found in metric schema dumps are conditioned on TCMALLOC_ENABLED. Line 59: TAG_FLAG(tcmalloc_free_bytes_percentage, advanced); Nit: is "tcmalloc" the flag namespace here? Or do you view this as more of a global flag? It doesn't really matter either way, but I've been chewing on "memory_tcmalloc_free_bytes_percentage" (i.e. lump it into the "memory" flag namespace). Line 482: // not in use) Nit: terminate with period. Line 490: "generic.current_allocated_bytes", &bytes_used); Maybe reuse GetTCMallocCurrentAllocatedBytes()? Might be weird without adding an equivalent for pageheap_free_bytes. -- To view, visit http://gerrit.cloudera.org:8080/1711 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I184c9b7c64a57d4cd178fe7cfb2c548e323edecb Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
