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
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Tim Armstrong
Gerrit-HasComments: Yes

Dan Hecht has posted comments on this change.
Change subject: IMPALA-3201: in-memory buffer pool implementation
..
Patch Set 7:
(8 comments)
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).
PS7, Line 276: page->buffer.is_open()
why not page->pinned? Would be equivalent but seems more direct in intent.
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
Line 382: int pin_count() const { return pin_count_; }
why do we need to expose this?
Line 414: void DecrementPinCount();
if we get rid of pin_count_ in the handle and only store that in the page (see
comment below), then these wouldn't be needed and it would be one less
abstraction level to think about.
Line 421: const Client* client_;
let's consider not having this as it doesn't seem to add much value. we can
always add it later if necessary.
Line 429: const BufferHandle* buffer_handle_;
this would also go away.
Line 432: int pin_count_;
why do we have this and the pinned_ boolean in the page? i.e why not just have
a single count in the page and no state in the handle?
--
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
Gerrit-Reviewer: Dan Hecht
Gerrit-HasComments: Yes

Dan Hecht has posted comments on this change.
Change subject: IMPALA-3201: in-memory buffer pool implementation
..
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4070/7/be/src/bufferpool/buffer-pool.h
File be/src/bufferpool/buffer-pool.h:
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 cache
this value, then it means that the underlying page_'s len/buffer can't be
changing (or else the cached value is wrong), and therefore it must be safe to
access those fields directly, right?
another way to look at is is that the whole point of pinning is to ensure that
the page to buffer mapping cannot change, and thus pinned pages' fields should
be accessible without a lock. any pages can only be pinned by a single client
which implies a single thread.
besides, what other thread should be able to modify (or even access) a pinned
page?
--
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
Gerrit-Reviewer: Dan Hecht
Gerrit-HasComments: Yes