Tim Armstrong has posted comments on this change. Change subject: IMPALA-3201: headers and reservation logic for new buffer pool ......................................................................
Patch Set 15: (43 comments) http://gerrit.cloudera.org:8080/#/c/2569/15/be/src/bufferpool/buffer-pool.cc File be/src/bufferpool/buffer-pool.cc: PS15, Line 138: DCHECK > DCHECK_EQ Done http://gerrit.cloudera.org:8080/#/c/2569/15/be/src/bufferpool/buffer-pool.h File be/src/bufferpool/buffer-pool.h: PS15, Line 39: and > nit: end sentence and remove "and" Done Line 47: /// > is the reservation tracker hierarchy also used to enforce max aggregate res Yes, that's the way I intend to use it. PS15, Line 70: it will be : /// allocated from the buffer pool > what does this mean? before who allocates what? what does "allocate" mean Reworded to be clearer. PS15, Line 75: If a client pins the same page multiple time > have you found that a client will want to do that? It's important for the case we have multiple clients, and it makes the invariants around Pin()/Unpin() and reservations extremely simple - if you Pin(), you consume reservation, if you Unpin(), you release it. It greatly simplifies reservation management when a client has multiple iterators over a sequence of unpinned pages. It guarantees that when you want to pin the next page, you'll get back enough reservation from unpinning the previous page to do so. Otherwise if two iterators have the same page pinned, only one page's worth of reservations are actually consumed, and the client has to ensure that it doesn't accidentally consume the remainder. PS15, Line 99: buffer > page? Done PS15, Line 101: buffers > page? Done PS15, Line 101: buffer > page? Done PS15, Line 124: capacity > any requirements on this being an exact multiple of min_page_len, or no? Not really. We won't be able to allocate the last capacity % min_page_len bytes, but it will behave as expected. Line 128: /// arguments are invalid. > explicitly document the arguments. Done PS15, Line 135: len > 'len' here and else were (helps parse the comments IMO). Done PS15, Line 137: If the preconditions are met > this kind of makes it sound like it's okay to call with the preconditions n Done. Clarified that it's enforced with a DCHECK, since I think that's not obvious even with that removed. Line 144: /// page is accessible through either handle until the handle is closed. > document parameters explicitly. Done. Did you want me to document 'client'? Every operation happens in the context of a client so that may be redundant. PS15, Line 147: Close > what does this mean? maybe define the "close" operation in the class comme Done PS15, Line 148: handles for a > maybe say "If this was the last handle for this page, then the page is clea Done. Clarified "cleaned up". You can close multiple handles for the same page if you duplicated them. PS15, Line 153: they have > it has Done PS15, Line 154: If the preconditions are : /// met > delete Done PS15, Line 160: is > becomes? Done PS15, Line 160: not > no longer Done PS15, Line 162: not invalid > shoudl this say "not valid" or "invalid"? Done Line 180: /// Same as Unpin(), except the page's lock must be already held by the caller. > document parameters (here and else where) Done Line 189: /// Returns true and decrease 'remaining_capacity_' if successful. > i think this needs more explanation. is it increasing capacity_ or allocati Done Line 192: BufferAllocator allocator_; > document what it's used for Done Line 199: const int64_t capacity_; > else where we refer to "capacity" as how much room the structure already ha Renamed to buffer_bytes_limit_ and buffer_bytes_remaining_ to be more explicit. PS15, Line 204: remaining_capacity_ > personally i find it easier to think about the positive than the negative. I started with a positive number, but it was a little simpler this way. It will be a lot simpler once we look at changing 'capacity_' dynamically: we can't easily update this this with an atomic increment if we have to check it against a variable 'capacity_'. Line 218: class BufferPool::Client { > who owns these objects? The client, like the PageHandle? Yes, it's similar. Made ownership clearer in the class comment. I thought about renaming it ClientHandle, particularly if we end up having some internal client-related state (we don't currently). Line 239: ReservationTracker* reservations_; > Are Clients and ReservationTrackers one-to-one? If so, why not put the Rese Query and process ReservationTrackers don't have an associated client, but otherwise typically yes. There's no reason the 1-to-1 relationship needs to be enforced. I looked at making reservations_ owned by the client, but it's a little clunky, since RegisterClient() then has to forward its arguments to InitChildTracker() and some of the unit tests have to be modified so that root trackers aren't used for buffer pool clients. I think I prefer it this way to avoid adding unnecessary coupling between Client and ReservationTracker. It ends up being simple enough in the final code, since they're initialized together in the ExecNode base class. Line 257: PageHandle& operator=(PageHandle&& src); > where are these two used? could we have a more explicit Transfer() routine One is needed for STL classes. When a std::vector is resized, it needs to move its contents. Another is needed to support std::move(), since we don't allow copying the handles. Added comments to document this. Line 259: bool is_open() const { return page_ != NULL; } > what does it mean for a PageHandle to be "open"? "closed"? Added to the class comment. PS15, Line 264: . > ... by this handle? Done PS15, Line 277: Open > what does that mean? Added to the class comment. PS15, Line 278: buffer > page? Done PS15, Line 283: pin count > which pin count? just the handle's count or the handle and the correspondin Done PS15, Line 284: buf > why doesn't this just come from the page? This is an internal helper. It's called from Page::IncrementPinCount() so it seems roundabout for Page to pass 'this' instead of its member 'page'. PS15, Line 286: . > for this handle or for the page as well? These are just internal helpers. I'll make the comments clearer that they're not part of the API. PS15, Line 297: buf_ > explain why this is duplicated here (from the page_). To avoid the indirec The indirection isn't a big deal, but acquiring the Page lock was more of a concern (either we acquire the lock or have to document when it's ok to do a lock-free read). Line 300: int64_t len_; > is this duplicated from the page_? explain that if so and why it's duplicat Done Line 303: int handle_pin_count_; > why do we have to track this? why isn't the page's pin count sufficient? It's necessary to support pinning the page multiple times via the same handle. E.g. in CloseHandle(), we need to know how many times to decrement the pages pin count. Also, to invalidate 'buf_' when it's no longer pinned by the page. As-is, it enforces that you can't dereference buf() on a page that you don't have pinned. http://gerrit.cloudera.org:8080/#/c/2569/15/be/src/bufferpool/reservation-tracker.h File be/src/bufferpool/reservation-tracker.h: PS15, Line 42: maximum reservations can enforce : /// per-query limits. > i'm not sure how the first part of this sentences is relevant to this concl "the maximum reservation mechanism can be used to enforce both process and query limits on reservations" PS15, Line 52: . > what's a case where a reservation tracker would not have a mem tracker? You only want them connected at one level of the hierarchy, otherwise you end up double-counting the consumption. In my implementation I have them connected at the exec node level, which works nicely. The ReservationTracker hierarchy enforces the limit on spillable memory and the MemTracker hierarchy reports consumption and enforces limit in the same way that it currently does at the process/query/operator level. Currently there's a hack to achieve this: memory is counted against both the block mgr and the exec node, then MemTracker::ConsumeLocal() is used to to avoid double-counting the consumption. That will no longer be necessary. PS15, Line 64: reservations > what reservations? only the ones for this root, right? No, all reservations for this and the trackers below it. I.e. all of 'reservations_' is counted against the mem tracker. PS15, Line 87: at least > so is this allowed to increase reservation more than amount? but then how d No, but it's possible that 'reservation_' - 'consumption_' > 'amount' before this is called. PS15, Line 107: RemainingReservation > Maybe UnusedReservation()? "remaining" seems a little confusing because th Done. This is clearer. -- 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: 15 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
