Bankim Bhavsar 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: (9 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 Strangely without Or function change, I couldn't reproduce it but with Or function change repro rate was like 50%. In order to reproduce it always, one approach would be to misalign the aligned bytes returned by the allocator and ensure AVX code path is used. Such a test that always crashes would prove that certain AVX operations that are used in BlockBloomFilter code on non-aligned bytes will result in SIGSEV. So I don't see much value in adding such a test. 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. compil Okay. I can remove the other static_assert(). For readability and context, I think it's better to keep the static_assert() close to where it's used. 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 Done http://gerrit.cloudera.org:8080/#/c/15372/1/src/kudu/util/memory/arena-test.cc@88 PS1, Line 88: ASSERT_EQ(0, reinterpret_cast<uintptr_t>(ret) % alignment) << > Should we also write to the memory, so that ASAN will catch any issues with Done. Refactored existing functions in this file to work with multi-threaded tests and tests that write some data to the allocated bytes. Verified with ASAN the difference is around 2.5X compared to debug build, 1750ms v/s 750 for all tests in arena-test. 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 allocation Good point. Done. http://gerrit.cloudera.org:8080/#/c/15372/1/src/kudu/util/memory/arena-test.cc@127 PS1, Line 127: int alignment = 32; > constexpr? Done 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 Allo const in the function definition is here is not useless as it prevents the implementation from accidentally updating its value. I removed the const specified in the declarations of AllocateBytesAligned() & AllocateBytesFallback() since they are useless there and the tidy bot complained about it. 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 a The max value that could get assigned to aligned variable is 63 so 32-bit number is sufficient. http://gerrit.cloudera.org:8080/#/c/15372/1/src/kudu/util/memory/arena.h@433 PS1, Line 433: : // Component start address "data_" is only guaranteed to be 16-byte aligned with enough : // bytes for the first request size plus any padding needed for alignment. : // So to support alignment values greater than 16 bytes, align the destination address ptr : // that'll be returned to the caller and not just the "offset_". : const auto data_start_addr = reinterpret_cast<uintptr_t>(data_); : size_t aligned = KUDU_ALIGN_UP((data_start_addr + offset_), alignment) - data_start_addr; > Could you refactor this into another inline function, so that the alignment Done -- 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: Sat, 07 Mar 2020 01:17:20 +0000 Gerrit-HasComments: Yes
