Michael Ho has posted comments on this change. Change subject: IMPALA-3611: track unused Disk IO buffer memory ......................................................................
Patch Set 6: (13 comments) It's good that IMPALA-3200 will get rid of this mess in disk io-mgr. Please see some more comments below. http://gerrit.cloudera.org:8080/#/c/3246/6/be/src/runtime/disk-io-mgr-scan-range.cc File be/src/runtime/disk-io-mgr-scan-range.cc: PS6, Line 441: NULL Is there a reason to not use "reader->mem_tracker_ != NULL ? reader->mem_tracker_ : unowned_buffer_mem_tracker_.get();" ? http://gerrit.cloudera.org:8080/#/c/3246/6/be/src/runtime/disk-io-mgr.cc File be/src/runtime/disk-io-mgr.cc: PS6, Line 224: nit: wrong indentation. PS6, Line 368: Other Untracked / Unowned ? PS6, Line 702: (1 << idx) 1LL to be consistent with the 64-bit-ness above. I think in practice, this is bound by 8MB, but let's be consistent here. PS6, Line 790: // Note: we can't use TryConsume(), which can indirectly call GcIoBuffers(). , which can indirectly call GcIoBuffers() and lead to deadlock. Would you mind adding one more TODO here about the imminent removal of the free buffer list in disk io-mgr once IMPALA-3200 is fixed ? PS6, Line 857: // TODO: we can do a lot better here. The reader can likely make progress : // with fewer io buffers. Can you please replace this TODO statement with one which indicates that "the disk io-mgr will no longer maintain a free list once IMPALA-3200 is fixed so these memory limit checks should be moved to GetFreeBuffer();" PS6, Line 859: free_buffer_mem_tracker_->AnyLimitExceeded(); Regarding your previous comment about not calling GcIoBuffer() here, I am not sure that's true. AnyLimitExceeded() will go through the calling mem-tracker and all its ancestors. The ancestor of free_buffer_mem_tracker_ still includes process_mem_tracker, right ? It's still not very clear to me what this line of change will achieve. http://gerrit.cloudera.org:8080/#/c/3246/6/be/src/runtime/disk-io-mgr.h File be/src/runtime/disk-io-mgr.h: PS6, Line 232: bool is_cached() { return scan_range_->cached_buffer_ != NULL; } This can be private. PS6, Line 237: MemTracker* mem_tracker() { return mem_tracker_; } Is this used outside of disk-io-mgr at all ? PS6, Line 243: 'tracker' 'dst' ? It may be clearer to say transfer from 'mem_tracker_' to 'dst_'. Please also state that the memory limits are not enforced in this function so callers are expected to handle the limit check somewhere until IMPALA-3200 is fixed. PS6, Line 787: buffer_size and max_buffer_size_ nit: 'buffer_size' and 'max_buffer_size_' PS6, Line 789: *buffer_size 'buffer_size' PS6, Line 790: max_buffer_size_ 'max_buffer_size' -- 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: 6 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
