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