Dan Hecht has posted comments on this change. Change subject: IMPALA-3203: Part 1: Free list implementation ......................................................................
Patch Set 4: (8 comments) http://gerrit.cloudera.org:8080/#/c/6410/2//COMMIT_MSG Commit Message: Line 10: are stored in a heap, ordered by the memory address of the tc-malloc has similar caches. are we adding this because we will stop using tc-malloc, or do we want it even with tc-malloc (since tc-malloc only caches small allocations)? answering these questions in the commit message (or jira) would be helpful to people to understand motivation (now and in the future). http://gerrit.cloudera.org:8080/#/c/6410/4/be/src/benchmarks/free-lists-benchmark.cc File be/src/benchmarks/free-lists-benchmark.cc: PS4, Line 58: 0 iters what does 0 iterations mean? PS4, Line 234: 256 kb won't this usually be 2MB? so why measuring only to 256 kb? http://gerrit.cloudera.org:8080/#/c/6410/4/be/src/runtime/bufferpool/buffer-allocator.cc File be/src/runtime/bufferpool/buffer-allocator.cc: PS4, Line 39: buffer.Reset(); given that 'buffer' is passed by copy, what is this trying to do? http://gerrit.cloudera.org:8080/#/c/6410/4/be/src/runtime/bufferpool/free-list.h File be/src/runtime/bufferpool/free-list.h: Line 36: /// A non-threadsafe list of free buffers. a couple of things that wasn't clear without reading the code: - how does buffer allocation work. it's done by the caller, yet the free list does buffer freeing. - the list itself doesn't know (or care) about the property of the buffers it's storing (e.g. size, numa node). the caller is responsible for maintaining the list properly (this follows from the first bullet, but could be explicit). PS4, Line 42: which can cause difficulties for the OS in some : /// cases. what's an example? Line 46: /// Gets a free buffer. Returns true and sets 'buffer' to a buffer. explain the false case. PS4, Line 99: b1.data() >= b2.data() why not '!SortCompare(...)' since that's the requirement? -- To view, visit http://gerrit.cloudera.org:8080/6410 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia89acfa4efdecb96d3678443b4748932b4133b9b Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
