Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15372 )
Change subject: [util] Add support for 32 & 64 byte alignment to Arena allocator ...................................................................... Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/15372/1/src/kudu/util/block_bloom_filter-test.cc File src/kudu/util/block_bloom_filter-test.cc: http://gerrit.cloudera.org:8080/#/c/15372/1/src/kudu/util/block_bloom_filter-test.cc@118 PS1, Line 118: sometimes How often 'sometimes' is? Since it's quite simple scenario, is it possible to have 100% reproduction rate with prior non-patched code? http://gerrit.cloudera.org:8080/#/c/15372/1/src/kudu/util/block_bloom_filter.cc File src/kudu/util/block_bloom_filter.cc: http://gerrit.cloudera.org:8080/#/c/15372/1/src/kudu/util/block_bloom_filter.cc@311 PS1, Line 311: static_assert(CACHELINE_SIZE >= 32, : "For AVX operations, need buffers to be 32-bytes aligned or higher"); : *ptr = arena_->AllocateBytesAligned(bytes, CACHELINE_SIZE); nit: I would expect it's enough to have just one static_assert (i.e. compile-time assert) of this sort since every method is processed by compiler? Maybe, move it into renaBlockBloomFilterBufferAllocator constructor? http://gerrit.cloudera.org:8080/#/c/15372/1/src/kudu/util/memory/arena-test.cc File src/kudu/util/memory/arena-test.cc: http://gerrit.cloudera.org:8080/#/c/15372/1/src/kudu/util/memory/arena-test.cc@83 PS1, Line 83: upto nit: up to http://gerrit.cloudera.org:8080/#/c/15372/1/src/kudu/util/memory/arena-test.cc@115 PS1, Line 115: ThreadSafeArena Does it make sense to run multiple threads performing those test allocations in case of ThreadSafeArena? http://gerrit.cloudera.org:8080/#/c/15372/1/src/kudu/util/memory/arena-test.cc@127 PS1, Line 127: int alignment = 32; constexpr? http://gerrit.cloudera.org:8080/#/c/15372/1/src/kudu/util/memory/arena.h File src/kudu/util/memory/arena.h: http://gerrit.cloudera.org:8080/#/c/15372/1/src/kudu/util/memory/arena.h@397 PS1, Line 397: const nit: remove this and next useless 'const' specifier to be uniform with AllocateBytesFallback()? http://gerrit.cloudera.org:8080/#/c/15372/1/src/kudu/util/memory/arena.h@409 PS1, Line 409: Atomic32 Is 32-bit number is always enough to store addresses here even for 64-bit architectures? -- To view, visit http://gerrit.cloudera.org:8080/15372 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib665115fa0fc262a8b76c48f52947dedb84be2a7 Gerrit-Change-Number: 15372 Gerrit-PatchSet: 1 Gerrit-Owner: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Fri, 06 Mar 2020 06:13:33 +0000 Gerrit-HasComments: Yes
