impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3203: Part 2: per-core free lists in buffer pool
Date Tue, 04 Apr 2017 21:05:08 GMT
Tim Armstrong has posted comments on this change.

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


Patch Set 15:

(41 comments)

http://gerrit.cloudera.org:8080/#/c/6414/15/be/src/common/init.cc
File be/src/common/init.cc:

PS15, Line 101:  
> weird indentation and close parenthesis is missing.
Done


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 59: .
> should be more explicit that it might return less memory than 'target_bytes
Done


PS15, Line 64: returns the total amount freed 
> hmm, it's kinda confusing that the return value has this subtle difference 
I think it's doing the same operation, I just think I needed to get two different number out
of it. I changed it to return a pair and changed the arguments so that the meaning was hopefully
a bit clearer.


PS15, Line 70: FreeMemory
> do you think it makes sense to call this FreeSystemMemory() to make it clea
Works for me, I think it's better to be clear


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


PS15, Line 138: GetBuffersOfSize
> GetListsForSize?
Done


PS15, Line 254: ffinally
> typo
Done


Line 261:       if (delta == len) break;
> since we can't get rid of the other DecreaseSystemBytesRemaining() probably
Done


PS15, Line 263: lock_arenas
> thought this would become 'final_attempt'
Done


Line 304:   vector<std::unique_lock<SpinLock>> arena_locks;
> let's add a comment explaining all of this. Something like:
Added a lightly-edited version of this comment


PS15, Line 317:   // It's possible that 'system_bytes_remaining_' was incremented since we
last checked
              :   // it.
> maybe this sentence would now be explained in the proposed comment above.
Done


PS15, Line 319: another thread between when that thread released memory to the system and
              :   // incremented 'system_bytes_remaining_'.
> I think it'd be good to explicitly say:
Tried to make this explicit.


PS15, Line 322: false
> why false instead of true? I guess it doesn't matter since it should succee
That's true, I hadn't thought of it like that.


PS15, Line 419: FreeBuffers
> this name is a bit confusing now that i see it in use since "free buffers" 
Done


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


PS15, Line 423: bytes_freed > 0
> when would this be false?
This code was removed.


PS15, Line 478: !al.owns_lock() && 
> why have this condition here? it's still correct to skip the whole block if
That's true, it doesn't really matter, removed it.


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 h
Tried to improve the comment.


PS15, Line 489: bytes_to_free
> this is so similarly named to buffers_to_free and buffer_bytes_to_free but 
Yeah I think I had a bug at one point that was a result of me confusing the two. Renamed it.


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 u
Done


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.
I changed the interface a bit. Hopefully the new version will make it less 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::AddFreeBu
AddFreeBuffer() got simplified as a consequence of other changes so these should align now.


PS15, Line 569: touched
> needed?
Done


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 
After looking at this again I realised I was probably overthinking it. We only really need
one low water mark. I think we may want to reduce the frequency of memory maintenance - every
1s may be too aggressive at cleaning up memory.


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


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

Line 47: /// required memory from the OS).
> I think explaining a couple of more things would help:
Done


PS15, Line 91: void
> does the caller care if it couldn't release 'bytes_to_free' bytes?
It's not needed currently (the caller checks the process memory limit instead).


PS15, Line 133: buffers
> memory? since some of it might come from 'system_bytes_remaining_'.
Good point


PS15, Line 135: lock_arenas
> one thing that was confusing is that the name of this makes it sound like a
I called it 'slow_but_sure', which I think conveys the idea.


http://gerrit.cloudera.org:8080/#/c/6414/15/be/src/runtime/bufferpool/buffer-pool-counters.h
File be/src/runtime/bufferpool/buffer-pool-counters.h:

PS15, Line 28: allocate buffers
> this makes it confusing because i'm not sure what "level" of allocation it'
Done


Line 36:   RuntimeProfile::Counter* bytes_alloced;
> similarly, for these it's not obvious which level of "allocation" we're tal
Done


http://gerrit.cloudera.org:8080/#/c/6414/15/be/src/runtime/bufferpool/buffer-pool-internal.h
File be/src/runtime/bufferpool/buffer-pool-internal.h:

PS15, Line 48: BufferPool::clean_pages.
> shouldn't this be: in a FreeBufferArena clean_pages list or something?
Done


Line 77: #include "common/atomic.h"
> why here? doesn't look used by this header.
Missed this when moving around code (I originally had BuffereAllocator etc declared in this
header).


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

PS15, Line 375: clean up the write
> this and the fact that the method is called "CancelWriteAndRestoreData" mak
Done


PS15, Line 385: the
              :   // earlier 'evicted' check.
> this no longer exists.
Done


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

PS15, Line 247: free unneeded memory.
> Does this mean "release unneeded memory back to the system allocator" or so
Done


PS15, Line 273: ObjectPool obj_pool_;
> okay, but why did we move to obj_pool instead of the scoped_ptr?
I can't remember actually, I think I originally was going to add more things to it. Reverted
this part.


http://gerrit.cloudera.org:8080/#/c/6414/15/be/src/runtime/disk-io-mgr.cc
File be/src/runtime/disk-io-mgr.cc:

PS15, Line 763: int idx = 0; idx < free_buffers_.size
> when we free everything, it probably doesn't matter which order we traverse
I don't think either order is clearly better.

Large-to-small may result in more fragmentation, since we'll tend to hold onto many small
buffers. We could hold onto small buffers almost indefinitely.

Small-to-large probably means the caching will be more effective since we hold onto more buffers.

Small-to-large seems less likely to result in pathological behaviour so may be better.

Alternatively we could just ignore the argument and revert to the old behaviour, since that
never seemed to cause any problems.


http://gerrit.cloudera.org:8080/#/c/6414/15/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:

PS15, Line 315: from cheapest to most expensive
> is that really the reason? we have multiple levels of GC, i.e. allocated ba
I agree it would probably be better to design the AddGcFunction() interface differently but
I think it ends up being a somewhat invasive change that doesn't buy that much. I added a
comment to explain that TCMalloc must run last and clarified that the functions are called
in the order that they were added via AddGcFunction()


http://gerrit.cloudera.org:8080/#/c/6414/15/be/src/util/cpu-info.h
File be/src/util/cpu-info.h:

PS15, Line 133: in
> delete
Done


PS15, Line 133: index
> what is this used for? does the index value have any significance or just t
It's just to give a dense numbering and saves having to do a linear search over the return
value of GetCoresOfNumaNode() to figure it out.


-- 
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