impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Date Wed, 08 Jun 2016 21:41:16 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................


Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/3246/3/be/src/runtime/disk-io-mgr.cc
File be/src/runtime/disk-io-mgr.cc:

PS3, Line 688: 
             :   buffer_desc->Reset(reader
> Why is it ?
Possibly a mistake in the original code, but consuming additional memory from the block mgrs
MemTracker has consequences for buffer reservations, spilling, etc, so it seems best not to
open that can of worms.


PS3, Line 691: 
> They are essentially untracked memory., right ? How about having a dummy tr
It would be tracked against the process mem-tracker (aside from in ASAN), which uses TCMalloc
stats, which is the old behaviour. Using the dummy tracker seems to simplify the code, so
I did that.


PS3, Line 834: ve the reader so that another disk thre
> Why do we want to cancel the query just because we have too many free buffe
The process memory limit check calls GcIoBuffers() if it's over the process limit, so that
scenario is avoided.

The behaviour is equivalent, since we dont' set a limit on buffer_mem_tracker_. It just seemed
strange to explicitly go up one level in the MemTracker hierarchy to check the mem-limit.


http://gerrit.cloudera.org:8080/#/c/3246/3/be/src/runtime/disk-io-mgr.h
File be/src/runtime/disk-io-mgr.h:

PS3, Line 240:    int64_t scan_range_offset() const { return scan_range_offset_; }
             : 
             :     /// Transfer ownership of buffer memory between two MemTrackers.
             :     /// 'mem_tracker_' and 'tracker' must be non-NULL
             :     void TransferOwnership(MemTracker* dst);
> This seems a bit complicated. Given there are only two callers (i.e. Return
I simplified this so that it only has to handle the non-NULL cases for RowBatch. I didn't
want RowBatch to have to directly manipulate the trackers, because I think DiskIoMgr should
be responsible for keeping the descriptor in a consistent state. I.e. RowBatch shouldn't enforce
BufferDescriptor's invariants.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8777cf76f04d34a46f53d53005412e0f1d63b5b7
Gerrit-PatchSet: 4
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message