impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Taras Bobrovytsky (Code Review)" <ger...@cloudera.org>
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:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/6414/10/be/src/common/init.cc
File be/src/common/init.cc:

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
for


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

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)


http://gerrit.cloudera.org:8080/#/c/6414/10/be/src/runtime/mem-tracker.cc
File be/src/runtime/mem-tracker.cc:

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 http://gerrit.cloudera.org:8080/6414
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

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

Mime
View raw message