Tim Armstrong has posted comments on this change. Change subject: IMPALA-5713: always reserve memory for preaggs ......................................................................
Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/7739/3/fe/src/main/java/org/apache/impala/planner/AggregationNode.java File fe/src/main/java/org/apache/impala/planner/AggregationNode.java: PS3, Line 327: 16b > should we add a static_assert to backend to update this if needed, or you t I guess the number of buckets doesn't matter too much from the frontend's perspective - I could just omit the calculation and say 64kb per hash table. Line 330: perInstanceMinReservation = (bufferSize + 64 * 1024) * PARTITION_FANOUT; > this means we end up reserving around the same amount of memory for streami Yeah it was. That's one of the reasons I don't think this change is a win in all situations. I think that is less important though with the new buffer pool since the minimum requirements with 2mb buffers is more sane (34mb versus 264mb to spill). The memory reservation here is min(34mb, estimated size of all groups in memory), which seems like a good amount of memory to reserve for a mid-sized aggregation if memory isn't too scarce. This breaks down if the estimates are off and/or memory requirements are very tight (and we don't care about the degraded performance). And yeah, THP is a concern - preaggregations actually benefited the most from it. -- To view, visit http://gerrit.cloudera.org:8080/7739 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2b544f9ec1a906719e2bbb074743926b95a3a573 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
