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, 28 Mar 2017 16:04:17 GMT
Tim Armstrong has posted comments on this change.

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


Patch Set 13:

(15 comments)

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

Line 35: static int64_t FREE_LIST_MAX_BYTES = 1024L * 1024L * 1024L;
> how are these chosen?
Pretty arbitrarily, I was just guestimating at what a reasonable upper bound to the amount
of memory to hold onto was. Potentially we don't need these and could just rely on scavenging/maintenance
to shrink the lists if they get too large.


PS13, Line 94: BufferSize
> PerSizeLists?
Works for me. I struggled to come up with a good name since it's more of a grouping of data
structures than an abstraction.


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

Line 31: /// released to SystemAllocator.
> buffers allocation lengths requested from this class must be a power of 2, 
Done


Line 44: /// system error occurs (e.g. we can't allocate all of the required memory from the
OS).
> what about allocations beyond the limit? are they guaranteed to fail, or ar
Done


PS13, Line 50: len
> required to be power of 2?
Done


PS13, Line 74: it
> the buffer?
Done


PS13, Line 81: free
> what does that mean? free back to the SystemAllocator?
Tried to clarify. Not sure if the level of detail is appropriate for this comment.


PS13, Line 108: 'min_buffer_len'
> hard to imagine what this input means and how it affects the result without
Done


PS13, Line 115: 'len'
> what is that?
Done


PS13, Line 122: The buffers are freed with 'system_allocator_'
> why would memory that comes from 'buffer_bytes_remaining_' need to be freed
That part was confusing. Reworded to make it clearer that there are two different strategies
to come up with the memory. The code works out simpler to implement all the strategies in
this method, but I guess it makes the name ScavengeBuffers less accurate. I'm not sure what
a better alternative mightbe.


PS13, Line 135: we support allocating
> that can be allocated. i.e. i assume it's not just unsupported, it wouldn't
Done


PS13, Line 138: we support allocating.
> same
Done


Line 159:   static const int MAX_SCAVENGE_ATTEMPTS = 3;
> what happens after this number of retries?
elaborated in the comment.


Line 164: };
> how do we (or will we) expose whatever needs to be exposed from this (and b
Most of the perf counters are client-centric, so we will get a lot of info in the profiles
for debugging query performance.

We'll also get information through the MemTracker hierarchy (you can see used/reserved). We
have DebugString() here too that dumps out the current state of the allocator when an allocation
unexpectedly fails.

I have some basic memory metrics (bytes in use, etc) in a follow-on patch that also switches
to using mmap(). 

It would be easy enough to add more global metrics but it's unclear to me what would be actually
useful for debugging. E.g. total #/bytes of allocations and total #/bytes of buffers released
to the system might be marginally useful.


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

Line 28:   /// Amount of time spent trying to allocate a buffer.
> there are multiple levels of "allocate" in the bufferpool subsystem. are th
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: 13
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