You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2016/05/31 16:41:15 UTC
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Tim Armstrong has uploaded a new change for review.
http://gerrit.cloudera.org:8080/3246
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
IMPALA-3611: track unused Disk IO buffer memory
Track I/O buffers against a separate MemTracker. This gives us better
visibility into memory consumption from the debug webpage and from
MemTracker consumption dumps. The immediate motivation was in trying to
determine whether idle memory consumption of an impalad was caused by a
memory leak.
The previous code "tracked" the buffers against the process-wide
tracker, but it was a no-op outside of ASAN builds since the
process-wide tracker took its value from TCMalloc.
The test code required fixing because it assumed that buffers were
always credited against the DiskIoMgr's tracker. This only made sense
when the DiskIoMgr's tracker is the root process-wide tracker.
Fix backend test logging for disk-io-mgr-test.
Testing:
Ran exhaustive tests, ran local and cluster stress tests.
Change-Id: I8777cf76f04d34a46f53d53005412e0f1d63b5b7
---
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/disk-io-mgr.h
3 files changed, 93 insertions(+), 85 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/46/3246/1
--
To view, visit http://gerrit.cloudera.org:8080/3246
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8777cf76f04d34a46f53d53005412e0f1d63b5b7
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3246
to look at the new patch set (#8).
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
IMPALA-3611: track unused Disk IO buffer memory
Track I/O buffers against separate MemTrackers. This gives us better
visibility into memory consumption from the debug webpage and from
MemTracker consumption dumps. The immediate motivation was in trying to
determine whether idle memory consumption of an impalad was caused by a
memory leak.
We add two trackers: for buffers cached in DiskIoMgr's free list,
and another for clients that don't provide a MemTracker (the only
one is BufferedBlockMgr, which will be removed at some point).
The previous code "tracked" the buffers against the process-wide
tracker, but it was a no-op outside of ASAN builds since the
process-wide tracker took its value from TCMalloc.
The test code required fixing because it assumed that buffers were
always credited against the DiskIoMgr's tracker. This only made sense
when the DiskIoMgr's tracker is the root process-wide tracker.
Fix backend test logging for disk-io-mgr-test.
Testing:
Ran exhaustive tests, ran local and cluster stress tests.
Change-Id: I8777cf76f04d34a46f53d53005412e0f1d63b5b7
---
M be/src/runtime/disk-io-mgr-scan-range.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/disk-io-mgr.h
M be/src/runtime/row-batch.cc
5 files changed, 224 insertions(+), 158 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/46/3246/8
--
To view, visit http://gerrit.cloudera.org:8080/3246
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8777cf76f04d34a46f53d53005412e0f1d63b5b7
Gerrit-PatchSet: 8
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has abandoned this change.
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
Abandoned
Moved to http://gerrit.cloudera.org:8080/3799
--
To view, visit http://gerrit.cloudera.org:8080/3246
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I8777cf76f04d34a46f53d53005412e0f1d63b5b7
Gerrit-PatchSet: 13
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3246
to look at the new patch set (#10).
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
IMPALA-3611: track unused Disk IO buffer memory
Track I/O buffers against separate MemTrackers. This gives us better
visibility into memory consumption from the debug webpage and from
MemTracker consumption dumps. The immediate motivation was in trying to
determine whether idle memory consumption of an impalad was caused by a
memory leak.
We add two trackers: for buffers cached in DiskIoMgr's free list,
and another for clients that don't provide a MemTracker (the only
one is BufferedBlockMgr, which will be removed at some point).
The previous code "tracked" the buffers against the process-wide
tracker, but it was a no-op outside of ASAN builds since the
process-wide tracker took its value from TCMalloc.
The test code required fixing because it assumed that buffers were
always credited against the DiskIoMgr's tracker. This only made sense
when the DiskIoMgr's tracker is the root process-wide tracker.
Fix backend test logging for disk-io-mgr-test.
Testing:
Ran exhaustive tests, ran local and cluster stress tests.
Change-Id: I8777cf76f04d34a46f53d53005412e0f1d63b5b7
---
M be/src/runtime/disk-io-mgr-scan-range.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/disk-io-mgr.h
M be/src/runtime/row-batch.cc
5 files changed, 234 insertions(+), 165 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/46/3246/10
--
To view, visit http://gerrit.cloudera.org:8080/3246
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8777cf76f04d34a46f53d53005412e0f1d63b5b7
Gerrit-PatchSet: 10
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has uploaded a new patch set (#5).
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
IMPALA-3611: track unused Disk IO buffer memory
Track I/O buffers against separate MemTrackers. This gives us better
visibility into memory consumption from the debug webpage and from
MemTracker consumption dumps. The immediate motivation was in trying to
determine whether idle memory consumption of an impalad was caused by a
memory leak.
We add two trackers: for buffers cached in DiskIoMgr's free list,
and another for clients that don't provide a MemTracker (the only
one is BufferedBlockMgr, which will be removed at some point).
The previous code "tracked" the buffers against the process-wide
tracker, but it was a no-op outside of ASAN builds since the
process-wide tracker took its value from TCMalloc.
The test code required fixing because it assumed that buffers were
always credited against the DiskIoMgr's tracker. This only made sense
when the DiskIoMgr's tracker is the root process-wide tracker.
Fix backend test logging for disk-io-mgr-test.
Testing:
Ran exhaustive tests, ran local and cluster stress tests.
Change-Id: I8777cf76f04d34a46f53d53005412e0f1d63b5b7
---
M be/src/runtime/disk-io-mgr-scan-range.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/disk-io-mgr.h
M be/src/runtime/row-batch.cc
5 files changed, 209 insertions(+), 153 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/46/3246/5
--
To view, visit http://gerrit.cloudera.org:8080/3246
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8777cf76f04d34a46f53d53005412e0f1d63b5b7
Gerrit-PatchSet: 5
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has uploaded a new patch set (#7).
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
IMPALA-3611: track unused Disk IO buffer memory
Track I/O buffers against separate MemTrackers. This gives us better
visibility into memory consumption from the debug webpage and from
MemTracker consumption dumps. The immediate motivation was in trying to
determine whether idle memory consumption of an impalad was caused by a
memory leak.
We add two trackers: for buffers cached in DiskIoMgr's free list,
and another for clients that don't provide a MemTracker (the only
one is BufferedBlockMgr, which will be removed at some point).
The previous code "tracked" the buffers against the process-wide
tracker, but it was a no-op outside of ASAN builds since the
process-wide tracker took its value from TCMalloc.
The test code required fixing because it assumed that buffers were
always credited against the DiskIoMgr's tracker. This only made sense
when the DiskIoMgr's tracker is the root process-wide tracker.
Fix backend test logging for disk-io-mgr-test.
Testing:
Ran exhaustive tests, ran local and cluster stress tests.
Change-Id: I8777cf76f04d34a46f53d53005412e0f1d63b5b7
---
M be/src/runtime/disk-io-mgr-scan-range.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/disk-io-mgr.h
M be/src/runtime/row-batch.cc
5 files changed, 224 insertions(+), 158 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/46/3246/7
--
To view, visit http://gerrit.cloudera.org:8080/3246
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8777cf76f04d34a46f53d53005412e0f1d63b5b7
Gerrit-PatchSet: 7
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
Patch Set 3:
(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: BufferedBlockMgr does not expect read buffers to be tracked
: // against its MemTracker
Why is it ?
PS3, Line 691: reader->mem_tracker_ != NULL
They are essentially untracked memory., right ? How about having a dummy tracker for this until we fix IMPALA-3200 ?
PS3, Line 834: buffer_mem_tracker_->AnyLimitExceeded()
Why do we want to cancel the query just because we have too many free buffers ? Is it essentially checking for process memory limit here ? If so, the old code seems more explicit about it.
It seems unfortunate that we still have some query maintenance like code in this module.
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: /// Updates this buffer buffer to be owned by the new tracker. Consumption is
: /// released from the current tracker and added to the new one. If either tracker
: /// is NULL, the Release() or Consume() call for that tracker is skipped. I.e.
: /// if 'tracker' is NULL, the memory is just released from 'mem_tracker_', and if
: /// 'mem_tracker_' is NULL, only consumption of 'tracker' is increased.
This seems a bit complicated. Given there are only two callers (i.e. ReturnFreeBuffer() and RowBatch::AddIoBuffer()), I wonder if it's clearer to do the explicit ownership transfer by calling MemTracker::Release()/Consume() directly at those call sites.
I find reasoning about whether tracker / mem_tracker_ is NULL a bit too convoluted.
--
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: 3
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3246/10/be/src/runtime/disk-io-mgr.h
File be/src/runtime/disk-io-mgr.h:
Line 779: MemTracker* mem_tracker, ScanRange* range, char* buffer, int64_t buffer_size);
> let's group GetBufferDesc and GetFreeBuffer decls together, and group Retur
Did the grouping, but not doing the rename: unfortunately they're not actually totally analogous because of the way cached buffers are handled. GetFreeBuffer() never returns a cached buffer, but ReturnBuffer() handles cached buffer.
--
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: 10
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has uploaded a new patch set (#4).
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
IMPALA-3611: track unused Disk IO buffer memory
Track I/O buffers against separate MemTrackers. This gives us better
visibility into memory consumption from the debug webpage and from
MemTracker consumption dumps. The immediate motivation was in trying to
determine whether idle memory consumption of an impalad was caused by a
memory leak.
We add two trackers: for buffers cached in DiskIoMgr's free list,
and another for clients that don't provide a MemTracker (the only
one is BufferedBlockMgr, which will be removed at some point).
The previous code "tracked" the buffers against the process-wide
tracker, but it was a no-op outside of ASAN builds since the
process-wide tracker took its value from TCMalloc.
The test code required fixing because it assumed that buffers were
always credited against the DiskIoMgr's tracker. This only made sense
when the DiskIoMgr's tracker is the root process-wide tracker.
Fix backend test logging for disk-io-mgr-test.
Testing:
Ran exhaustive tests, ran local and cluster stress tests.
Change-Id: I8777cf76f04d34a46f53d53005412e0f1d63b5b7
---
M be/src/runtime/disk-io-mgr-scan-range.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/disk-io-mgr.h
M be/src/runtime/row-batch.cc
5 files changed, 206 insertions(+), 153 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/46/3246/4
--
To view, visit http://gerrit.cloudera.org:8080/3246
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8777cf76f04d34a46f53d53005412e0f1d63b5b7
Gerrit-PatchSet: 4
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3246/4/be/src/runtime/disk-io-mgr.cc
File be/src/runtime/disk-io-mgr.cc:
Line 781: if (!FLAGS_disable_mem_pools && free_buffers_[idx].size() < FLAGS_max_free_io_buffers) {
> Oh right. It's very unfortunate how intertwined disk-io-mgr and memtracker
If we remove the GC logic from TryConsume(), we may consider doing the GC in a separate maintenance thread instead.
--
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 <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3246/4/be/src/runtime/disk-io-mgr.cc
File be/src/runtime/disk-io-mgr.cc:
Line 781: if (!FLAGS_disable_mem_pools && free_buffers_[idx].size() < FLAGS_max_free_io_buffers) {
> Fixed long line.
Oh right. It's very unfortunate how intertwined disk-io-mgr and memtracker are. That's really error prone.
We should either have a version of TryConsume() which doesn't GC or remove the GC logic from TryConsume() altogether.
--
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 <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3246
to look at the new patch set (#12).
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
IMPALA-3611: track unused Disk IO buffer memory
Track I/O buffers against separate MemTrackers. This gives us better
visibility into memory consumption from the debug webpage and from
MemTracker consumption dumps. The immediate motivation was in trying to
determine whether idle memory consumption of an impalad was caused by a
memory leak.
We add two trackers: for buffers cached in DiskIoMgr's free list,
and another for clients that don't provide a MemTracker (the only
one is BufferedBlockMgr, which will be removed at some point).
The previous code "tracked" the buffers against the process-wide
tracker, but it was a no-op outside of ASAN builds since the
process-wide tracker took its value from TCMalloc.
The test code required fixing because it assumed that buffers were
always credited against the DiskIoMgr's tracker. This only made sense
when the DiskIoMgr's tracker is the root process-wide tracker.
Fix backend test logging for disk-io-mgr-test.
Testing:
Ran exhaustive tests, ran local and cluster stress tests.
Change-Id: I8777cf76f04d34a46f53d53005412e0f1d63b5b7
---
M be/src/runtime/disk-io-mgr-scan-range.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/disk-io-mgr.h
M be/src/runtime/row-batch.cc
5 files changed, 236 insertions(+), 167 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/46/3246/12
--
To view, visit http://gerrit.cloudera.org:8080/3246
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8777cf76f04d34a46f53d53005412e0f1d63b5b7
Gerrit-PatchSet: 12
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
Patch Set 9: Code-Review+1
--
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: 9
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
Patch Set 7:
(2 comments)
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 859: d, we should be able to remove the free lists
> Just checking my understanding here since you claimed in an earlier reply t
If I claimed that I didn't mean to. I looked at using TryConsume() in DiskIoMgr::ReturnFreeBuffer(), but that wasn't safe to do because of the issue with free_buffers_lock_.
http://gerrit.cloudera.org:8080/#/c/3246/7/be/src/runtime/disk-io-mgr.cc
File be/src/runtime/disk-io-mgr.cc:
PS7, Line 749: L
> Isn't long 32-bit according to the standard ? I thought we need LL, no ?
On Linux and Mac OS X long is the machine word size. Maybe you're thinking of Windows? I'll switch to LL since that is more obviously correct.
--
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: 7
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has uploaded a new patch set (#2).
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
IMPALA-3611: track unused Disk IO buffer memory
Track I/O buffers against a separate MemTracker. This gives us better
visibility into memory consumption from the debug webpage and from
MemTracker consumption dumps. The immediate motivation was in trying to
determine whether idle memory consumption of an impalad was caused by a
memory leak.
The previous code "tracked" the buffers against the process-wide
tracker, but it was a no-op outside of ASAN builds since the
process-wide tracker took its value from TCMalloc.
The test code required fixing because it assumed that buffers were
always credited against the DiskIoMgr's tracker. This only made sense
when the DiskIoMgr's tracker is the root process-wide tracker.
Fix backend test logging for disk-io-mgr-test.
Testing:
Ran exhaustive tests, ran local and cluster stress tests.
Change-Id: I8777cf76f04d34a46f53d53005412e0f1d63b5b7
---
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/disk-io-mgr.h
M be/src/runtime/row-batch.cc
4 files changed, 134 insertions(+), 109 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/46/3246/2
--
To view, visit http://gerrit.cloudera.org:8080/3246
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8777cf76f04d34a46f53d53005412e0f1d63b5b7
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/3246/1/be/src/runtime/disk-io-mgr.cc
File be/src/runtime/disk-io-mgr.cc:
Line 700: Consume(buffer_size)
Do you think it makes sense to convert this to TryConsume() in the long run or is it too hard to propagate the failure at this point ? Looks like the caller already checks for the status in the buffer descriptor created below so may be we can somehow take advantage of that.
Also, it seems a bit convoluted to first count the consumption towards the buffer_mem_tracker_ and then transfer it to the reader's mem-tracker in GetBufferDesc(). May be we should outright count the consumption towards the reader and call Release() on buffer_mem_tracker_ explicitly if we are consuming from the free_buffers_ ?
Line 764: SetMemTracker(
This is a bit of a misnomer. Do you think it makes sense to call it TransferMemory() or TransferBufferOwnership() or something along that line ?
Line 829: process_limit_exceeded = buffer_mem_tracker_->AnyLimitExceeded();
I wonder if this check should be moved to ReturnFreeBuffer() ? Also, this is no longer tracking process memory limit so may be this variable should be renamed if it's kept.
--
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: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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/4/be/src/runtime/disk-io-mgr.cc
File be/src/runtime/disk-io-mgr.cc:
PS4, Line 210: Clear(
> Will Reset() be a better name ? This seems to match Reset() below.
Done
PS4, Line 219: mem_tracker_ = NULL;
> nit: Can this be at line 213 so we initialize the fields in the same order
Done here and below.
I think it's better to initialize it, so done.
Line 781: if (!FLAGS_disable_mem_pools && free_buffers_[idx].size() < FLAGS_max_free_io_buffers) {
> Long line.
Fixed long line.
Using TryConsume() here could result in a deadlock since if it hits the process memory limit, it will call GcIoBuffers(), which attempts to grab free_buffers_lock_. Added a comment.
PS4, Line 856: bool any_io_mgr_limit_exceeded = free_buffer_mem_tracker_->AnyLimitExceeded();
> Will it be more obvious if we move this line into ReturnFreeBuffer() instea
See the comment above for why we can't do that. Note that this AnyLimitExceeded() check will call GcIoBuffers() if the process memory limit is exceeded. That isn't the best way to handle it, but fixing it is a bigger task.
--
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 <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
Patch Set 13: Code-Review+2
Carry
--
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: 13
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
Patch Set 9:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/3246/9/be/src/runtime/disk-io-mgr.cc
File be/src/runtime/disk-io-mgr.cc:
Line 246: mem_tracker_->Release(buffer_len_);
it's a bit confusing that the tracking happens both in BufferDescriptor methods (here) and also directly in DiskIoMgr methods. maybe there's no great way to factor the code to keep the accounting all in once class though.
PS9, Line 368: Untracked
this is a little misleading given that they are tracked by this tracker. Why not just make a mem tracker for buffered block manager, and then nothing has to change in this code when the buffered block manager is deleted? is there something that made that tough?
PS9, Line 650: Not a cached buffer. Return the io buffer.
not your comment, but this comment is pretty useless, it just restates what the C++ code says. it'd be better to explain why cached buffers are not returned.
Line 752: delete[] buffer;
would be nice to delete first then Release().
http://gerrit.cloudera.org:8080/#/c/3246/9/be/src/runtime/disk-io-mgr.h
File be/src/runtime/disk-io-mgr.h:
PS9, Line 242: caller should check the memory limit
do callers actually check the mem limit?
or are they assuming that the mem limit is okay because the src and dst mem trackers are within the same hierarchy and dst itself doesn't have a limit?
PS9, Line 271: Non-NULL for non-cached
double negative is hard to parse. maybe say "Can be NULL only for cached"
PS9, Line 774: associated
what does this mean? does this routine consume memory from 'mem_tracker', or does this mean something else?
Line 792: /// max_buffer_size_.
would be good to comment on how mem tracking is affected.
Line 802: /// Returns the buffer in 'desc' (cannot be NULL), and sets desc->buffer_ to NULL.
likewise.
from the comments alone it's not clear what the difference between ReturnBuffer() and ReturnFreeBuffer() are.
--
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: 9
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3246/1/be/src/runtime/disk-io-mgr.cc
File be/src/runtime/disk-io-mgr.cc:
Line 239: if (mem_tracker_ != NULL) mem_tracker_->Consume(buffer_len_);
Not your change but this should really be TryConsume() in the long run too.
--
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: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
Patch Set 11: Code-Review+2
(4 comments)
http://gerrit.cloudera.org:8080/#/c/3246/9/be/src/runtime/disk-io-mgr.cc
File be/src/runtime/disk-io-mgr.cc:
Line 246: mem_tracker_->Release(buffer_len_);
> I feel like this actually makes sense, since the tracking in DiskIoMgr meth
Okay, that's fine.
PS9, Line 368: Untracked
> I originally called it "Other Disk IO Buffers" but changed it as a a result
Okay, fine to leave as is as long as this goes away with buffer pool.
http://gerrit.cloudera.org:8080/#/c/3246/9/be/src/runtime/disk-io-mgr.h
File be/src/runtime/disk-io-mgr.h:
Line 802:
> Renamed to FreeBufferMemory() and updated the comment.
Thanks for the rename.
http://gerrit.cloudera.org:8080/#/c/3246/10/be/src/runtime/disk-io-mgr.h
File be/src/runtime/disk-io-mgr.h:
Line 779: MemTracker* mem_tracker, ScanRange* range, char* buffer, int64_t buffer_size);
let's group GetBufferDesc and GetFreeBuffer decls together, and group ReturnBufferDesc and ReturnBuffer decls, since they have analogous relationships.
And it'd be nice to name them consistently to make this relationship more apparent. i.e. GetBuffer()/GetBufferDesc() or GetFreeBuffer()/GetFreeBufferDesc(), to be analogous to ReturnBuffer()/ReturnBufferDesc(). But okay to ignore this rename if you don't prefer.
--
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: 11
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
Patch Set 7: Code-Review+1
Yes, please switch to LL. I think we already do the same in decimal-util.h
--
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: 7
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
Patch Set 7:
(2 comments)
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 859: d, we should be able to remove the free lists
> It's safe to do that here since we're not holding free_buffers_lock_ at thi
Just checking my understanding here since you claimed in an earlier reply that we stopped calling GcIoBuffers() but that doesn't seem to be the case. Anyway, I guess it's fine either way as fundamentally, it's not ideal to do the limit checks here but we won't fix it in this patch.
http://gerrit.cloudera.org:8080/#/c/3246/7/be/src/runtime/disk-io-mgr.cc
File be/src/runtime/disk-io-mgr.cc:
PS7, Line 749: L
Isn't long 32-bit according to the standard ? I thought we need LL, no ?
--
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: 7
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
Patch Set 4:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/3246/4/be/src/runtime/disk-io-mgr.cc
File be/src/runtime/disk-io-mgr.cc:
PS4, Line 210: Clear(
Will Reset() be a better name ? This seems to match Reset() below.
PS4, Line 219: mem_tracker_ = NULL;
nit: Can this be at line 213 so we initialize the fields in the same order as they are declared ?
Why is it okay to not initialize scan_range_offset_ ? I guess it's always overwritten when the buffer is consumed, right ?
PS4, Line 648: // Not a cached buffer. Return the io buffer.
: ReturnFreeBuffer(buffer_desc);
nit: should fit in one line.
Line 781: if (!FLAGS_disable_mem_pools && free_buffers_[idx].size() < FLAGS_max_free_io_buffers) {
Long line.
Please also see comments below about free_buffer_mem_tracker.
PS4, Line 856: bool any_io_mgr_limit_exceeded = free_buffer_mem_tracker_->AnyLimitExceeded();
Will it be more obvious if we move this line into ReturnFreeBuffer() instead ? That should help reduce chances of failure due to exceeding memory limits, right ?
if (!FLAGS_disable_mem_pools &&
free_buffers_[idx].size() < FLAGS_max_free_io_buffers &&
free_buffer_mem_tracker_->TryConsume(buffer_size)) {
. ...
} else {
...
}
--
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 <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/3246/1/be/src/runtime/disk-io-mgr-test.cc
File be/src/runtime/disk-io-mgr-test.cc:
Line 932: TEST_F(DiskIoMgrTest, Buffers) {
Had to make more more fixes in this test - the version I pushed was missing some fixed I made locally but didn't squash into the patchset correctly.
http://gerrit.cloudera.org:8080/#/c/3246/1/be/src/runtime/disk-io-mgr.cc
File be/src/runtime/disk-io-mgr.cc:
PS1, Line 239: if (mem_tracker_ != NULL) mem_tracker_->Consume(buffer_len_);
> Not your change but this should really be TryConsume() in the long run too.
Agreed, but I think we need to look at the whole lifecycle of the buffers to work out the right technique to throttle the scans when memory is scarce.
PS1, Line 700: Consume(buffer_size)
> Do you think it makes sense to convert this to TryConsume() in the long run
I feel like this will probably change once we rework the scanner memory management anyway, so it may not be worth doing the plumbing at this point to thread through a TryConsume() failure. It looks tricky.
I cleaned up the memory transfer to avoid doing the transfer in GetBufferDesc(). This works out cleaner.
PS1, Line 764: SetMemTracker(
> This is a bit of a misnomer. Do you think it makes sense to call it Transfe
Done
PS1, Line 829: process_limit_exceeded = buffer_mem_tracker_->AnyLimitExceeded();
> I wonder if this check should be moved to ReturnFreeBuffer() ? Also, this i
Changed to any_io_mgr_limit_exceeded. In practice the only limit that applies to the io mgr now is the process limit, but the naming seems more correct in content.
--
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: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3246/4/be/src/runtime/disk-io-mgr.cc
File be/src/runtime/disk-io-mgr.cc:
Line 781: if (!FLAGS_disable_mem_pools && free_buffers_[idx].size() < FLAGS_max_free_io_buffers) {
> If we remove the GC logic from TryConsume(), we may consider doing the GC i
In the interest of avoiding throwaway work, my preference is to wait until after IMPALA-3200. I think the DiskIoMgr free lists will be unnecessary once we switch to allocating these buffers from the buffer pool, since the buffer pool will have its own free lists.
--
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 <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
Patch Set 9:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/3246/9/be/src/runtime/disk-io-mgr.cc
File be/src/runtime/disk-io-mgr.cc:
Line 246: mem_tracker_->Release(buffer_len_);
> it's a bit confusing that the tracking happens both in BufferDescriptor met
I feel like this actually makes sense, since the tracking in DiskIoMgr methods is handling ownership transfer between DiskIoMgr and an external mem tracker, while this is handling transfer between external mem trackers. This method is self-contained.
PS9, Line 368: Untracked
> this is a little misleading given that they are tracked by this tracker. W
I originally called it "Other Disk IO Buffers" but changed it as a a result of previous review comments. My comment then was "Went with untracked. I thought this might be a little confusing, since they are technically tracked, but seems clearer than 'other'.". I haven't been able to come up with better nomenclature.
The tracker is easy to create, but I don't think we want it to hang off the query mem tracker, since it would mean we have temporary 8MB bumps in query mem usage while spilling that could put it over the limit. Doesn't seem worth the potential problems. I could have the buffered block mgr tracker hang off the process mem tracker, which probably works ok.
Do you think that's better? It seems more complicated and confusing to me than the current approach. My preference is to leave as-is and avoid investing further time since it's a temporary solution.
PS9, Line 650: Not a cached buffer. Return the io buffer.
> not your comment, but this comment is pretty useless, it just restates what
Done
Line 752: delete[] buffer;
> would be nice to delete first then Release().
Done
http://gerrit.cloudera.org:8080/#/c/3246/9/be/src/runtime/disk-io-mgr.h
File be/src/runtime/disk-io-mgr.h:
PS9, Line 242: caller should check the memory limit
> do callers actually check the mem limit?
You're right, we don't transfer between queries, so there shouldn't be a net increase in memory usage as a result of this transfer. I was thinking that this would be a case where the QueryMaintenance() memory checks was still necessary, but I don't think it is for that reason.
Agree we need to revisit it, but I don't think there's a clear way to propagate the error, so I left the TODO.
PS9, Line 271: Non-NULL for non-cached
> double negative is hard to parse. maybe say "Can be NULL only for cached"
Done
PS9, Line 774: associated
> what does this mean? does this routine consume memory from 'mem_tracker', o
Updated the comment to reflect that this purely allocates and initializes the descriptor.
Line 792: /// max_buffer_size_.
> would be good to comment on how mem tracking is affected.
Done
Line 802: /// Returns the buffer in 'desc' (cannot be NULL), and sets desc->buffer_ to NULL.
> likewise.
Renamed to FreeBufferMemory() and updated the comment.
--
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: 9
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
Patch Set 6:
(10 comments)
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_tr
We don't want to track this memory, since it's a cached buffer that's owned by the HDFS client in the JVM.
Added a comment.
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.
Done
PS6, Line 368: Other
> Untracked / Unowned ?
Went with untracked. I thought this might be a little confusing, since they are technically tracked, but seems clearer than 'other'.
PS6, Line 702: (1 << idx)
> 1LL to be consistent with the 64-bit-ness above. I think in practice, this
Done (here and elsewhere in this file)
PS6, Line 790: // Note: we can't use TryConsume(), which can indirectly call GcIoBuffers().
> , which can indirectly call GcIoBuffers() and lead to deadlock.
Done
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 "t
Done. Also added a TODO referencing IMPALA-3209, since it's related to making scanners operate in a memory limit.
PS6, Line 859: free_buffer_mem_tracker_->AnyLimitExceeded();
> Regarding your previous comment about not calling GcIoBuffer() here, I am n
It's safe to do that here since we're not holding free_buffers_lock_ at this point in the code.
The change doesn't make a functional difference, it's just for consistency since we are no longer tracking directly against process_mem_tracker_. The pattern generally in the codebase is to interact only with your local MemTracker.
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.
Done
PS6, Line 237: MemTracker* mem_tracker() { return mem_tracker_; }
> Is this used outside of disk-io-mgr at all ?
Not now. Removed it. It was at one point when I tried to move this to RowBatch.
PS6, Line 243: 'tracker'
> 'dst' ? It may be clearer to say transfer from 'mem_tracker_' to 'dst_'. Pl
I think this is more related to IMPALA-3209, since this is part of the cycle of memory transfer between the disk io mgr, scanners, and upstream exec nodes. Added a reference to that JIRA.
I looked at just checking the memory limit and returning a status, but it looked like exiting early from some of the calling functions could leave things in an inconsistent state.
--
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 <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
Patch Set 9: Code-Review+2
Change 1L to 1LL and rebased
--
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: 9
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/3246/4/be/src/runtime/disk-io-mgr.cc
File be/src/runtime/disk-io-mgr.cc:
Line 688:
I'll remove this extra blank line.
http://gerrit.cloudera.org:8080/#/c/3246/4/be/src/runtime/disk-io-mgr.h
File be/src/runtime/disk-io-mgr.h:
Line 232: bool is_cached() { return scan_range_->cached_buffer_ != NULL; }
I added this to make the logic around cached buffers a little more explicit.
Line 256: /// Reset the buffer descriptor to an uninitialized state.
I added this so that we didn't recycle BufferDescriptors in some indeterminate state.
--
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 <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes