Tim Armstrong has posted comments on this change. Change subject: IMPALA-3203: Part 2: per-core free lists in buffer pool ......................................................................
Patch Set 14: (24 comments) http://gerrit.cloudera.org:8080/#/c/6414/14/be/src/runtime/bufferpool/buffer-allocator.cc File be/src/runtime/bufferpool/buffer-allocator.cc: PS14, Line 55: buffer > page? Done PS14, Line 57: Try > why are these "Try", whereas PopFreeBuffer is not? Removed Try Line 59: /// Free memory from this arena up to 'target_bytes'. > Free memory back to the system Done PS14, Line 62: Returns > If 'claim_memory' is true, returns ... Done PS14, Line 62: is returned > delete Done PS14, Line 77: . > what happens if claim_buffer is false? Done PS14, Line 79: bool > another case where we don't have "Try". let's be consistent (either delete Removed Try. PS14, Line 109: capacity > is that in terms of buffer counts or bytes? Done PS14, Line 124: allocate > reclaim? I think this makes more sense. Line 156: int64_t upper_bound = buffer_bytes_limit == 0 ? 1L : 1L > comment this: Yup. Unfortunately we don't have a general-purpose RoundDownToPowerOfTwo function. PS14, Line 174: )); > << min_buffer_len_ ? Done Line 216: } > can either of these legitimately happen, or does it indicate a bug? The first one would likely indicate a bug, although if we move more code over to the buffer pool, potentially you could configure Impala in a weird way to get there. E.g. maybe a LONG_MAX process memory limit plus a corrupt input file that claimed to be very large (e.g. there are some places where we allocate a buffer based on the purported uncompressed data size). The second can legitimately happen (in weird configs. most likely, e.g. running with a small process memory limit and trying to allocate a large hash table). PS14, Line 251: lock all the arenas on the : // last attempt to guarantee success. > instead of expressing as locking all arenas, it might be clearer to express Done Line 265: "attempts:\n$3", len, delta, max_scavenge_attempts_, pool_->DebugString())); > this indicates an accounting bug, right? add a comment to that effect. Done Line 296: if (bytes_found == target_bytes) return bytes_found; > i wonder if this code should be in the caller so that this routine is purel This attempt could be removed outside ScavengeBuffers() but the one at the bottom of the the method needs to be done with the locks held. Line 302: if (lock_arenas) arena_locks.resize(per_core_arenas_.size()); > i don't understand what guarantee this locking provides. the reservations g The memory is somewhere but we might get repeatedly unlucky with timing. E.g. if thread A is doing a pass over the lists, and thread B removes a buffer from ahead of thread A and adds a buffer behind thread A. Or an equivalent race between decrementing system_bytes_remaining_ and adding a free buffer. I initially thought this was unlikely and retrying a few times would be enough but the randomized test hit this frequently. The alternative is to just loop indefinitely until we get that memory, but that seems to risk a livelock scenario. PS14, Line 313: last > does this mean "final" or "previous"? Updated the comment to clarify - it's really referring to the time when we were iterating over the lists. Line 347: return arena->RemoveCleanPage(claim_buffer, page); > is there an ABA problem here where the page was clean in arena 0, then evic 'client_lock' would guard against the page being moved to a different arena. The only transition that can happen here is Clean->Evicted. Line 544: lists->clean_pages.Remove(page); > combine previous two lines? Thanks, missed this http://gerrit.cloudera.org:8080/#/c/6414/14/be/src/runtime/bufferpool/buffer-allocator.h File be/src/runtime/bufferpool/buffer-allocator.h: PS14, Line 114: sizes > size Done PS14, Line 124: no_partial_decrease > negatives in names are harder to reason about (passing false leads to a dou Done PS14, Line 126: memory to allocate 'target_bytes' through various : /// strategies > Tries to reclaim enough memory so that 'target_bytes' can be allocated from Done PS14, Line 161: buffer_bytes_remaining_ > I think the code would be more intuitive if this were called "system_bytes_ Done http://gerrit.cloudera.org:8080/#/c/6414/14/be/src/runtime/bufferpool/buffer-pool.cc File be/src/runtime/bufferpool/buffer-pool.cc: Line 274: counters_.num_allocations = ADD_TIMER(profile, "BufferPoolAllocations"); > Need to fix these - should use ADD_COUNTER Done -- 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: 14 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
