Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10550 )
Change subject: PREVIEW: IMPALA-7078: improve memory consumption of wide Avro scans ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/10550/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10550/2//COMMIT_MSG@18 PS2, Line 18: Cap the maximum row batch queue size at 5 * the number of active : scanner threads. > adding this heuristic on top of the current one seems hard to understand ho I want to get a tighter upper bound on the amount of memory in the queue and some guarantee that scaling down the scanner thread count (automatically or manually) will actually reduce the amount of queued memory. We could leave this out of the change - it's not essential - but it felt like it should include some harder guarantees about queue size to make it easier to reason about. We can usually control memory consumption from queueing batches by automatically limiting num_scanner_threads based on concurrency and availability of reservation (or by setting in manually), but there's no reason, with the current code, that a single scanner thread couldn't fill up the whole queue, e.g. if the consumer thread was off doing some other work for a long time. http://gerrit.cloudera.org:8080/#/c/10550/2//COMMIT_MSG@32 PS2, Line 32: Track the row batch queue memory consumption against a sub-tracker > what does that look like in terms of the memtracker hierarchy? Added the MemTracker dump here. http://gerrit.cloudera.org:8080/#/c/10550/2/be/src/exec/hdfs-avro-scanner.cc File be/src/exec/hdfs-avro-scanner.cc: http://gerrit.cloudera.org:8080/#/c/10550/2/be/src/exec/hdfs-avro-scanner.cc@496 PS2, Line 496: Check : // AtCapacity() at the end of the loop to guarantee that we process at least one row : // so that we mke progress even if the batch starts off with AtCapacity() == true. > that's kinda confusing because it's counter to our usual which is that once Done http://gerrit.cloudera.org:8080/#/c/10550/2/be/src/exec/hdfs-avro-scanner.cc@568 PS2, Line 568: keep_current_chunk = true to > but line 573 passes false. i think you meant false here. For some reason my brain likes reversing things sometimes. http://gerrit.cloudera.org:8080/#/c/10550/2/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/10550/2/be/src/exec/hdfs-scan-node.cc@238 PS2, Line 238: row_batch->SetMemTracker(row_batches_mem_tracker_); > which mem-tracker are these accounted to before this line? They're accounted against ExecNode::mem_tracker_ - see HdfsScanner::ProcessSplit() then transferred to the caller of GetNext() with AcquireState() http://gerrit.cloudera.org:8080/#/c/10550/2/be/src/runtime/mem-tracker.cc File be/src/runtime/mem-tracker.cc: http://gerrit.cloudera.org:8080/#/c/10550/2/be/src/runtime/mem-tracker.cc@243 PS2, Line 243: } > why is it that we need this new interface now but didn't need it in other c The motivation for doing it in this patch specifically is to avoid adding more contention on the global MemTracker consumption counters (process and pool level) in case that started to become more of a bottleneck. I think this is a better solution in general than separately calling Consume() and Release() because it avoids the unnecessary updates of common ancestor counters and also avoids the window of inconsistency where the memory isn't accounted against the query (which can be hard to reason about). We should probably switch to using this in other places where it applies, e.g. the various DataStream* classes in a follow-on. -- To view, visit http://gerrit.cloudera.org:8080/10550 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iebd2600b4784fd19696c9b92eefb7d7e9db0c80b Gerrit-Change-Number: 10550 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 31 May 2018 01:27:59 +0000 Gerrit-HasComments: Yes
