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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
