Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11103 )
Change subject: IMPALA-7096: restore scanner thread memory heuristics ...................................................................... Patch Set 9: Code-Review+2 (6 comments) Carry +2 http://gerrit.cloudera.org:8080/#/c/11103/7/be/src/runtime/scanner-mem-limiter.h File be/src/runtime/scanner-mem-limiter.h: http://gerrit.cloudera.org:8080/#/c/11103/7/be/src/runtime/scanner-mem-limiter.h@32 PS7, Line 32: limiting the aggregate memory consumpt > is it to limit the number of scanner threads, or the aggregate memory consu Done http://gerrit.cloudera.org:8080/#/c/11103/7/be/src/runtime/scanner-mem-limiter.h@42 PS7, Line 42: as this objec > garbled Done http://gerrit.cloudera.org:8080/#/c/11103/7/be/src/runtime/scanner-mem-limiter.h@43 PS7, Line 43: (i.e. as long as the below methods > is that requirement because the instance of this class happens to also be a Done http://gerrit.cloudera.org:8080/#/c/11103/7/be/src/runtime/scanner-mem-limiter.h@71 PS7, Line 71: /// ClaimMemoryForScannerThread() will not be called. > do we need that? why not just iterate over the map? do we want ordering but I started off with the vector then kept it as an optimisation to allow more efficient iteration in ClaimMemoryForScannerThread. Now that I look at it again I doubt there's a significant enough difference between this and unordered_map to justify the complexity. http://gerrit.cloudera.org:8080/#/c/11103/7/be/src/runtime/scanner-mem-limiter.cc File be/src/runtime/scanner-mem-limiter.cc: http://gerrit.cloudera.org:8080/#/c/11103/7/be/src/runtime/scanner-mem-limiter.cc@75 PS7, Line 75: e a crude heuristic of guessing that the scan : // will > what code? before this change or before the change that removed the origina Added a more concrete description of which commits added/removed the code. http://gerrit.cloudera.org:8080/#/c/11103/7/be/src/runtime/scanner-mem-limiter.cc@81 PS7, Line 81: addtl_consumption += static_cast<int64_t>((consumption * 1.5) / num_threads); : } > I don't really understand that. Why would adding this thread increase consu Yeah exactly. Tried to improve the comment. -- To view, visit http://gerrit.cloudera.org:8080/11103 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib9907fa8c4d2b0b85f67f4f160899c1c258ad82b Gerrit-Change-Number: 11103 Gerrit-PatchSet: 9 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 15 Aug 2018 21:20:58 +0000 Gerrit-HasComments: Yes
