Tim Armstrong has posted comments on this change. Change subject: IMPALA-3201: in-memory buffer pool implementation ......................................................................
Patch Set 7: (11 comments) http://gerrit.cloudera.org:8080/#/c/4070/4/be/src/bufferpool/buffer-allocator.h File be/src/bufferpool/buffer-allocator.h: PS4, Line 31: Implement free SystemAllocator? http://gerrit.cloudera.org:8080/#/c/4070/4/be/src/bufferpool/buffer-pool.cc File be/src/bufferpool/buffer-pool.cc: Line 246: I think we need to handle this differently. Using ExtractBuffer() if it's pinned will avoid us shuffling it into the free lists. http://gerrit.cloudera.org:8080/#/c/4070/7/be/src/bufferpool/buffer-pool.cc File be/src/bufferpool/buffer-pool.cc: PS7, Line 66: > 0 > delete (though this may become pin_count anyway). Done PS7, Line 276: page->buffer.is_open() > why not page->pinned? Would be equivalent but seems more direct in intent. An unpinned page may still have a buffer associated with it if it hasn't been evicted. In that case we should not allocate a new buffer. Currently we never evict the page so checking page->pinned would always hit a DCHECK in AllocateBufferInternal() where we try to overwrite the open buffer hanlde. http://gerrit.cloudera.org:8080/#/c/4070/7/be/src/bufferpool/buffer-pool.h File be/src/bufferpool/buffer-pool.h: PS7, Line 262: > bytes Done Line 382: int pin_count() const { return pin_count_; } > why do we need to expose this? I added it for a unit test, but I didn't seen any reason to hide it - the client code should know the pin count implicitly anyway. Line 414: void DecrementPinCount(); > if we get rid of pin_count_ in the handle and only store that in the page ( Done Line 421: const Client* client_; > let's consider not having this as it doesn't seem to add much value. we can My concern was that if some code mistakenly provided the wrong client to a BufferPool API call, we could go a long way off into the weeds before the mistake became apparent - could be tricky to debug. Worse, it might actually succeed a lot of the time leaving a latent bug. PS7, Line 423: cached locally to avoid acquiring the page lock : /// unnecessarily. > I'm not sure this make sense. one way to look at it is if it's safe to cach Done Line 429: const BufferHandle* buffer_handle_; > this would also go away. Done Line 432: int pin_count_; > why do we have this and the pinned_ boolean in the page? i.e why not just Done -- To view, visit http://gerrit.cloudera.org:8080/4070 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4bda61c31cc02d26bc83c3d458c835b0984b86a0 Gerrit-PatchSet: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
