Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8034 )
Change subject: IMPALA-5522:Use tracked memory for DictDecoder and DictEncoder ...................................................................... Patch Set 8: (4 comments) I took a look at it and just wanted to point out some cases where the new code is diverging from best practices in our codebase. http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/exec/hdfs-parquet-scanner.cc@348 PS8, Line 348: CloseAndUnregisterFromParent > I found that MemTracker::Close() is not unregistering with the parent MemTr We should be using Close() here instead of unregistering every tracker (this is documented in the comment for CloseAndUnregisterFromParent()). That may be moot since I don't think we need a special MemTracker for this - it's fine if we just track this against the scan node tracker. There are pros/cons of tracking things at a finer granularity but generally we don't track memory at a very fine granularity. Looking at the JIRA description, I can see it being read as "track the memory against a new MemTracker" but that wasn't what I intended. As-is the behaviour added by this patch is particularly weird because we don't have per-scanner MemTrackers, so you'll end up with tens of these "dict decoder" trackers hanging off the scan node. http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/util/dict-encoding.h File be/src/util/dict-encoding.h: http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/util/dict-encoding.h@133 PS8, Line 133: inline nit: inline here and nearby isn't necessary for a couple of reason. First, the compiler can generally figure out that trivial functions can be inlined with or without a hint. Second, functions defined in C++ class bodies have an implicit inline hint implied anyway: https://isocpp.org/wiki/faq/inline-functions#inline-member-fns-more http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/util/dict-encoding.h@184 PS8, Line 184: if (dict_bytes_cnt_ != 0) { Nit: check isn't necessary - Release() does this for you. http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/util/dict-encoding.h@194 PS8, Line 194: // TODO: AnyLimitExceeded() can be called to check if memory limit has been exceeded. The TODO should be to use TryConsume(). The asynchronous checks are not the direction we want to take things in. Michael Ho had a nice comment here explaining the direction we're trying to take things: https://issues.apache.org/jira/browse/IMPALA-2399 -- To view, visit http://gerrit.cloudera.org:8080/8034 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I02a3b54f6c107d19b62ad9e1c49df94175964299 Gerrit-Change-Number: 8034 Gerrit-PatchSet: 8 Gerrit-Owner: Pranay Singh Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Pranay Singh Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 13 Oct 2017 22:49:17 +0000 Gerrit-HasComments: Yes
