Adar Dembo 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: (2 comments) 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@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 the underlying allocation for the component? Just make sure the test isn't dog slow in ASAN though. 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@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 calculation is done just once and the comment placement is clear? -- 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: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Thu, 05 Mar 2020 23:25:29 +0000 Gerrit-HasComments: Yes
