Tim Armstrong has posted comments on this change. Change subject: IMPALA-3200: Implement suballocator for splitting buffers ......................................................................
Patch Set 14: (9 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 Done Line 80: DCHECK(free_node != nullptr); > it looks like this dcheck can fire if SplitToSize() fails to allocate a nod Good point - SplitToSize() should return an error in that case. Fixed. Line 107: } > why do we need both policies? WHEN_NEEDED will be convenient for porting the existing exec nodes, and I think we'll need NEVER in future when exec nodes want to have better control over their reservations. If we want to remove NEVER for the time being that also works. Line 137: return Status::OK(); > Now that I read one of Dan's comments, I wonder if this should be an error Yep this was a mistake. Line 216: *ptr_from_prev = move(node->next_free_); > should this reset result->prev_free_ so we don't have a dangling reference, Good point - Done 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? w Expanded to be clearer about how min_buffer_len plays into this and what is a power of two. We require that we can allocate power-of-two buffers from the buffer pool. Line 104: static constexpr int64_t MAX_ALLOCATION_BYTES = 1L << LOG_MAX_ALLOCATION_BYTES; > after reading the rest of the code I see it's used so to bound the depth of Done 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's really referring to node->in_use so updated to reflect. PS14, Line 159: support > supported? Done -- 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 <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Jim Apple <jbapple-imp...@apache.org> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
