Jim Apple has posted comments on this change. Change subject: IMPALA-3200: Implement suballocator for splitting buffers ......................................................................
Patch Set 11: (3 comments) http://gerrit.cloudera.org:8080/#/c/4715/4//COMMIT_MSG Commit Message: Line 14: larger chunks. This helps avoid external fragmentation and is quite > I really really really really don't want "implement a general-purpose memor I think there are a few ways to get both what I am concerned about (buddy allocator internal fragmentation reduction) and what you want (a simple allocator). One way to is static_assert that sizeof(HashTable::Bucket) is a power of 2. I think this will be true in the status quo. Another is to add a parameter to the constructor of the Suballocator that sets the size of an Atom. All blocks would be of size 2^i * sizeof(Atom), and the logic would remain the same. Setting Atom = HashTable::Bucket would reduce internal fragmentation. http://gerrit.cloudera.org:8080/#/c/4715/11/be/src/bufferpool/suballocator-test.cc File be/src/bufferpool/suballocator-test.cc: Line 107: static void ExpectReservationUnused(ReservationTracker& reservation) { What happened to the const? http://gerrit.cloudera.org:8080/#/c/4715/4/be/src/bufferpool/suballocator.h File be/src/bufferpool/suballocator.h: Line 171: /// An allocation made by a Suballocator. Each allocation returned by Suballocator must > But why shouldn't we use it for a different case where it's the right tool? Just so I'm clear, in the current system, a Suballocation may have several pointers to it, but the only unique_ptr is: 1. in the free_lists_ if it is free and the head of its free list 2. In the Suballocation before it if it is free but not the head of its free list 3. In its left child if it is not free and already split. 4. In some client code if it is not free and not split. Additionally, at the function boundaries of the methods of Suballocator and Suballocation, there is always exactly one unique_ptr to each Suballocation. Did I get those right? -- 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: 11 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[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
