impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5522:Use tracked memory for DictDecoder and DictEncoder
Date Fri, 13 Oct 2017 22:49:17 GMT
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.vig@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Oct 2017 22:49:17 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message