Jim Apple has posted comments on this change. Change subject: IMPALA-3200: Implement suballocator for splitting buffers ......................................................................
Patch Set 8: (4 comments) http://gerrit.cloudera.org:8080/#/c/4715/4//COMMIT_MSG Commit Message: PS4, Line 13: buddy allocation > That's true. I think that's the first bridge this will come to, and so I think it's a mistake to delay consideration of the HT use case. There may be hidden barriers to changing the HT size - the resizing strategy, the hash function used, the prbing strategy, and so on. http://gerrit.cloudera.org:8080/#/c/4715/5/be/src/bufferpool/suballocator-test.cc File be/src/bufferpool/suballocator-test.cc: PS5, Line 290: . T > The documentation for lognormal_distribution is misleading/wrong. Empirical That could be a bug in libstdc++. I don't think we can use libc++ yet because of some c++03 #includes in our dependencies. I'd say to use a different distribution here that you trust so that this test does not break if/when Impala uses a newer libstdc++/gcc. http://gerrit.cloudera.org:8080/#/c/4715/4/be/src/bufferpool/suballocator.h File be/src/bufferpool/suballocator.h: Line 171: /// be freed with Suballocator::Free(). We use unique_ptr to manage ownership of these > unique_ptr really only enforces unique ownership, not any kind of static sc While it may be a happy case, I think it is the far more common case of unique_ptr use. I don't see a similar pattern in plan-fragment-executor.{h,cc}. A typo from my last comment: I said "I would find aw pointer less confusing". I should have said "I would find Raw pointerS less confusing". http://gerrit.cloudera.org:8080/#/c/4715/8/be/src/bufferpool/suballocator.h File be/src/bufferpool/suballocator.h: PS8, Line 86: otherwise destructing : /// the returned 'result' will DCHECK Does that mean I don't have to worry about calling Free if we compile only in release mode of if I remove the DCHECK? What is the reason under the DCHECK - preventing memory leaks? -- 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: 8 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
