impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3200: Implement suballocator for splitting buffers
Date Thu, 15 Dec 2016 02:52:14 GMT
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 <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