impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Taras Bobrovytsky (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3203: Part 2: per-core free lists in buffer pool
Date Fri, 24 Mar 2017 02:37:58 GMT
Taras Bobrovytsky has posted comments on this change.

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

Patch Set 10:

File be/src/common/

PS10, Line 96: Maintenance thread
Log Maintenance thread

PS10, Line 97: 1)
It's kind of weird to have "1)" here. Maybe condense everything into a single paragraph

PS10, Line 224: from
File be/src/runtime/bufferpool/

Line 28:   DCHECK_GE(len, min_buffer_len_);
Should we also check that len is smaller than some maximum?

Line 29:   DCHECK(BitUtil::IsPowerOf2(len)) << len;
In the .h file comment you mentioned that len must be a power of two multiple of min_buffer_len,
but that's not what's being checked here.
I think we should check IsPowerOf2(len / min_buffer_len)
and maybe DCHECK(len % min_buffer_len == 0)
File be/src/runtime/

PS10, Line 312: curr_consumption - max_consumption + EXTRA_BYTES_TO_FREE
I think it would be clearer if the result of this calculation was put into a variable:
int64_t amount_to_free = curr_consumption + EXTRA - max_consumption

Line 315:     if (curr_consumption <= max_consumption) break;
Should we take into account EXTRA_BYTES_TO_FREE here?
if (curr_consumption + EXTRA_BYTES <= max)

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I612bd1cd0f0e87f7d8186e5bedd53a22f2d80832
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Taras Bobrovytsky <>
Gerrit-HasComments: Yes

View raw message