Tim Armstrong 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 usin Done 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? Added a comment to explain - it's the number of passes we do over the memory as part of the "work". Making the label longer messes up the formatting unfortunately. PS4, Line 234: 256 kb > won't this usually be 2MB? so why measuring only to 256 kb? It's not really interesting - even at 256KB throughput is memory-bound. 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? There's a DCHECK in ~BufferHandle to detect leaks. Added a comment to explain. 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: Done PS4, Line 42: which can cause difficulties for the OS in some : /// cases. > what's an example? Linux has a limit on the number of non-contiguous mapped regions. Updated the comment. Line 46: /// Gets a free buffer. Returns true and sets 'buffer' to a buffer. > explain the false case. Done PS4, Line 99: b1.data() >= b2.data() > why not '!SortCompare(...)' since that's the requirement? Done -- 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
