impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Date Fri, 10 Jun 2016 19:20:34 GMT
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 <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