impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3203: Part 1: Free list implementation
Date Fri, 17 Mar 2017 00:12:34 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-3203: Part 1: Free list implementation

Patch Set 4:

Commit Message:

Line 10: are stored in a heap, ordered by the memory address of the
tc-malloc has similar caches.  are we adding this because we will stop using tc-malloc, or
do we want it even with tc-malloc (since tc-malloc only caches small allocations)?  answering
these questions in the commit message (or jira) would be helpful to people to understand motivation
(now and in the future).
File be/src/benchmarks/

PS4, Line 58: 0 iters 
what does 0 iterations mean?

PS4, Line 234: 256 kb
won't this usually be 2MB? so why measuring only to 256 kb?
File be/src/runtime/bufferpool/

PS4, Line 39: buffer.Reset();
given that 'buffer' is passed by copy, what is this trying to do?
File be/src/runtime/bufferpool/free-list.h:

Line 36: /// A non-threadsafe list of free buffers.
a couple of things that wasn't clear without reading the code:
- how does buffer allocation work. it's done by the caller, yet the free list does buffer
- the list itself doesn't know (or care) about the property of the buffers it's storing (e.g.
size, numa node). the caller is responsible for maintaining the list properly (this follows
from the first bullet, but could be explicit).

PS4, Line 42: which can cause difficulties for the OS in some
            : /// cases.
what's an example?

Line 46:   /// Gets a free buffer. Returns true and sets 'buffer' to a buffer.
explain the false case.

PS4, Line 99: >=
why not '!SortCompare(...)' since that's the requirement?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia89acfa4efdecb96d3678443b4748932b4133b9b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message