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 Thu, 30 Mar 2017 00:20:18 GMT
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 <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