impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3200: Implement suballocator for splitting buffers
Date Thu, 15 Dec 2016 01:57:44 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-3200: Implement suballocator for splitting buffers
......................................................................


Patch Set 14:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/4715/14/be/src/runtime/bufferpool/suballocator.cc
File be/src/runtime/bufferpool/suballocator.cc:

Line 68: 
extraneous blank


Line 80:     DCHECK(free_node != nullptr);
it looks like this dcheck can fire if SplitToSize() fails to allocate a node, no?


Line 107:   }
why do we need both policies?


Line 216:   *ptr_from_prev = move(node->next_free_);
should this reset result->prev_free_ so we don't have a dangling reference, for easier
debugging?


http://gerrit.cloudera.org:8080/#/c/4715/14/be/src/runtime/bufferpool/suballocator.h
File be/src/runtime/bufferpool/suballocator.h:

Line 31: /// allocation algorithm optimised for power-of-two allocations. Uses a binary buddy
does it require that the buffer pool buffers themselves are power of two? what size buffer
pool pages does it allocate?


Line 104:   static constexpr int64_t MAX_ALLOCATION_BYTES = 1L << LOG_MAX_ALLOCATION_BYTES;
this is really large (entire virtual address space on current x64 implementations). what does
it guard against?


PS14, Line 125: 'node' should already be free and not in
              :   /// any free list
what does this mean given that free nodes are in the free list? do you mean it was in the
free list but already removed or something else?


PS14, Line 159: support
supported?


-- 
To view, visit http://gerrit.cloudera.org:8080/4715
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8bfe0e429f67ad273f7c7d0816703a9e6c3da788
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: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message