impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <>
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:


It's good that IMPALA-3200 will get rid of this mess in disk io-mgr. Please see some more
comments below.
File be/src/runtime/

PS6, Line 441: NULL
Is there a reason to not use "reader->mem_tracker_ != NULL ? reader->mem_tracker_ :
unowned_buffer_mem_tracker_.get();" ?
File be/src/runtime/

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.
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

PS6, Line 790: max_buffer_size_

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I8777cf76f04d34a46f53d53005412e0f1d63b5b7
Gerrit-PatchSet: 6
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message