Tim Armstrong has posted comments on this change. Change subject: IMPALA-3201: reservation implementation for new buffer pool ......................................................................
Patch Set 4: (8 comments) http://gerrit.cloudera.org:8080/#/c/3993/4/be/src/bufferpool/reservation-tracker.h File be/src/bufferpool/reservation-tracker.h: Line 50: /// * A tracker's reservation is at most its reservation limit. > does 'reservation' mean 'remaining reservation' or 'what has been deducted Maybe I misunderstood the point. I don't think it's either of those. Reservation means the total amount of buffer pool memory that the subtree is entitled to use. Rather than the amount it's entitled to use minus the amount that it's used minus the amount it's children have reserved. I.e. using a reservation means: usage_ += amount; And the invariant is: usage + child_reservations <= reservation <= limit And memory consumption on the MemTracker is: reservation If I understood your first alternative correctly, you mean: Using a reservation requires: usage += amount; reservation -= amount; And the (slightly weaker) invariant is: usage + child_reservations + reservation <= limit (it's slightly weaker because it can't detect after-the-fact if usage was increased without getting a reservation - you need a DCHECK_LE(amount, reservation) before every increment of usage_) Memory consumption on the MemTracker is: usage + child_reservations + reservation Line 51: /// * A tracker's reservation is at least the sum of its childrens' reservations plus > however, that would contradict this sentence (because my children eat into I'm not sure I understand this comment (mabe a consequence of not understanding the first one). I added some more explicit notation from the above comment since it seems worthwhile anyway. Line 59: /// as consumption because reserved memory is committed. An existing reservation can > we need to be careful to differentiate actual consumption vs. used-up reser I plan to reflect usage, reservation, and limit in profile counters for diagnostic purposes (I have had a draft follow-up patch for this hanging around for ages but didn't want to expand the scope of this patch further). Line 60: /// therefore always be used without increasing consumption and with no risk of exceeding > this seems to contradict what you said earlier. I don't think I understand. This point is that a call to IncreaseUsage() (e.g. from allocating a buffer) can't push a client over a memory limit. (since the having enough unused reservation is a precondition for calling IncreaseUsage()). Line 70: void InitRootTracker(MemTracker* mem_tracker, int64_t reservation); > could the root be expressed as having parent == null? Definitely. This approach allows stronger enforcement of invariants though. Otherwise if you had a bug in Prepare() that didn't initialize a tracker, you could end up with a NULL parent tracker, a broken tracker hierarchy and consequent bizarre behaviour. Line 74: /// children will be counted as consumption against 'mem_tracker'. > if the reservation is really counted immediately against a memtracker (with The MemTracker also tracks other memory that was allocated outside of the buffer pool and has no corresponding reservation. It doesn't track how much of the reservation is used (i.e. only deals with limit/consumption versus limit/reservation/usage). It also can't enforce limits on spillable buffer pool memory (which we need at the query and process level). Line 103: /// Increase the usage. The tracker must have at least 'amount' remaining in its > i find this terminology confusing, as explained above. do you really mean ' I don't think I understand this comment. I don't think this was your point, but I changed this comment to use the more consistent term of 'unused reservation'. Line 118: int64_t GetUsage(); > i find this term ambiguous as well, because it seems to imply the memory is Not sure if you were just suggesting a terminological change or suggesting to track usage externally of ReservationTracker. Usage here does mean "usage of the reservation" not "usage of memory". Although they're equal. I think we need to track usage internally otherwise it's difficult to force meaningful invariants. E.g. no way to enforce reservation + usage <= limit. -- To view, visit http://gerrit.cloudera.org:8080/3993 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
