Tim Armstrong has posted comments on this change. Change subject: IMPALA-3201: headers and reservation logic for new buffer pool ......................................................................
Patch Set 17: (12 comments) I didn't end up adding a separate Buffer class. I looked at it but it would require adding more code for the same functionality. Hopefully the new header comments make it clear how to use the buffer pool for that purpose. I don't think it works out to be complex in practice. http://gerrit.cloudera.org:8080/#/c/2569/17/be/src/bufferpool/buffer-allocator.h File be/src/bufferpool/buffer-allocator.h: Line 22: /// Memory allocator for buffers that are multiples of the minimum buffer length. > unclear what this is: who uses it, what is the minimum buffer length? It's just a stub at the moment that calls malloc. I elaborated slightly but it feels like it's getting redundant with the class/subdirectory names and method comments. http://gerrit.cloudera.org:8080/#/c/2569/17/be/src/bufferpool/reservation-tracker.h File be/src/bufferpool/reservation-tracker.h: Line 37: /// nodes. To increase its reservation, a tracker must consume some of its parent's > in l35 consumption refers to actual memory usage, not the reservation. now It's now "usage" instead of consumption. Line 49: /// * A tracker's consumption is at least the sum of its childrens' reservations. > that doesn't make sense to me if consumption = memory consumption. I separated out child reservations from consumption for bookkeeping purposes. I don't think it changes the logic to any real extent. Line 52: /// Each ReservationTracker can optionally have an associated MemTracker. Any > why optionally? if this class tracks memory consumption, i'd expect that to To avoid double-counting. I reworked the mechanism so that it avoids double-counting if the hierarchy is linked at multiple levels. It got a bit more complicated but isn't too bad. In any case linking the process-level trackers shouldn't be necessary. Line 57: /// MemTracker, so reservations count against both the exec node and the query MemTracker. > i don't understand that either, this seems to conflate concepts (reservatio I think they should be conflated for most purposes, aside from internally in the buffer pool and reservation trackers. Aside from that, the only case that you would really care about the distinction is in debugging memory management issues, e.g. an operator reserving more memory than it needs. If an operator isn't using all of it's reservation, it's similar to allocating a 2mb buffer and only using part of it, or wastage from MemPool chunks. Line 60: /// one level. Otherwise reservations will be double-counted against MemTrackers. > should the linking be in the other direction: a memtracker can have an asso I don't think the other direction works, since you need to check the reservation limits and memory limits when increasing a reservation, but only need to check memory limits when increasing consumption against the memory tracker. Why would a MemTracker need to check a reservation? Non-spillable memory won't have a reservation in the short to medium term so those allocations don't interact with reservation trackers at all. PS17, Line 67: s > don't refer to internal state (reservations_) in a function comment. the fu I reworded it a bit to remove the reference. Hopefully it didn't make it significantly more ambiguous. Line 73: /// counted as consumption against 'mem_tracker'. > i don't understand why a reservation is reflected somewhere as consumption. This is explained in the class comment. The reservation is committed memory so must be counted towards all memory limits. If you don't count it as consumption, you easily get into a situation where using your reservation would put you over a memory limit. Line 99: /// Decrease tracker's reservation by amount. This tracker must have used at least > standardize your terminology (here it's 'used', elsewhere 'consumed'). It's usage everywhere now Line 103: /// Consumes the given amount of the reservation. The tracker must have at least 'amount' > long line Done Line 108: void Release(int64_t amount); > release doesn't sound like the opposite of consume (in fact, i'd think it m It's Release()/Consume() in MemTracker. I renamed to Increase/DecreaseUsage(). Line 111: int64_t Reservation(); > Get...() (and the next 2) Done -- To view, visit http://gerrit.cloudera.org:8080/2569 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6 Gerrit-PatchSet: 17 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk 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
