impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3203: Part 2: per-core free lists in buffer pool
Date Fri, 31 Mar 2017 00:18:43 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-3203: Part 2: per-core free lists in buffer pool
......................................................................


Patch Set 15:

(14 comments)

Next batch. I've gone through buffer-allocator.h/.cc.

http://gerrit.cloudera.org:8080/#/c/6414/15/be/src/runtime/bufferpool/buffer-allocator.cc
File be/src/runtime/bufferpool/buffer-allocator.cc:

PS15, Line 137:  Return the buffer size for a buffer of the given length.
doesn't match


PS15, Line 138: GetBuffersOfSize
GetListsForSize?


PS15, Line 419: FreeBuffers
this name is a bit confusing now that i see it in use since "free buffers" is also used as
a noun.  Anyway, this code might be clearer if instead of this method, the free-list exposes
a way to get N buffers from the tail, and then this class calls the system_allocator_->Free().
That way, all system_allocator_ Allocate()/Free() happens in one layer and FreeList doesn't
have to know about SystemAllocator. (You'd have to remove FreeAll() also, but that seems like
you could just use the same routine by passing the number of buffers in the list as the count).


PS15, Line 421: so that other threads
              :     // can claim the memory.
not sure what this comment is saying.


PS15, Line 423: bytes_freed > 0
when would this be false?


PS15, Line 478: !al.owns_lock() && 
why have this condition here? it's still correct to skip the whole block if the lock is already
held, right?


Line 488:     // Figure out how many buffers we should free of this size.
i was misled by this comment since at this point, buffers_to_free is only how many buffers
from the free list (not  how many buffers we should free of this size).


PS15, Line 489: bytes_to_free
this is so similarly named to buffers_to_free and buffer_bytes_to_free but has a different
meaning in that it's the target, not the accumulated. That makes the loop a bit harder to
understand.  So, how about renaming this to 'target_bytes' or something?


Line 496:     // that they are freed based on memory address in the expected order.
this is basically the same as evicting the pages, right? it would help to use that terminology
here to make the connection explicit.


PS15, Line 525:   int64_t max_to_claim = claim_memory ? bytes_to_free : 0;
              :   if (bytes_freed > max_to_claim) {
              :     // Add back the extra for other threads before releasing the lock to avoid
race
              :     // where the other thread may not be able to find enough buffers.
              :     parent_->system_bytes_remaining_.Add(bytes_freed - max_to_claim);
              :     bytes_freed = max_to_claim;
              :   }
as mentioned earlier in the function comment, this is pretty confusing.


PS15, Line 551:     BufferHandle buffer;
              :     {
              :       lock_guard<SpinLock> pl(page->buffer_lock);
              :       buffer = move(page->buffer);
              :     }
              :     lists->free_buffers.AddFreeBuffer(move(buffer));
              :     lists->num_free_buffers.Add(1);
why doesn't this need to do the other stuff that FreeBufferArena::AddFreeBuffer() does?  Why
not just remove 'claim_buffer' and this block, and then have the buffer-pool just always call
back down into BufferAllocator::Free().

Oh we just talked about that and you pointed out that then a client would temporarily be over
its reservation while the lock is dropped.

But, shouldn't this block do everything FreeBufferArena::AddFreeBuffer() does?


PS15, Line 569: touched
needed?


Line 571:       // least one, we guarantee that an idle list will shrink to zero entries.
can you add some motivation for why we need to care about the previous two intervals, rather
than, say, just the last interval?


PS15, Line 576: bytes_freed > 0
when would this be false?


-- 
To view, visit http://gerrit.cloudera.org:8080/6414
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I612bd1cd0f0e87f7d8186e5bedd53a22f2d80832
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message