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 <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
