You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org> on 2020/03/05 21:19:20 UTC
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
Bankim Bhavsar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15372
Change subject: [util] Add support for 32 & 64 byte alignment to Arena allocator
......................................................................
[util] Add support for 32 & 64 byte alignment to Arena allocator
On adding couple of member variables and methods to BlockBloomFilter class
for importing Or() function, predicate-test would sometimes randomly crash
with SIGSEGV in AVX operations.
On debugging, the crash would only happen when the "directory_" structure
in BlockBloomFilter is not 32-bytes aligned. It's surprising without the
addition of those methods, "directory_" so far has always been 32 bytes
aligned despite Arena allocator not supporting alignment values greater than
16 bytes.
With input from Todd, explored 3 options to fix the alignment problem:
1) Update the HeapBufferAllocator in util/memory to align allocation to
64 bytes. See AllocateInternal() implementation. Surprisingly the
FLAGS_allocator_aligned_mode is OFF by default so we appear to be relying
on the allocator to always allocate 16 byte aligned buffers. So this option
would require turning ON the FLAGS_allocator_aligned_mode flag by default.
2) Update the Arena allocator such that when needed extra bytes are allocated
to allow aligning with 32/64 bytes considering the new component will always
be 16 byte aligned. This requires updating some address/alignment logic
with offset_ and the new component allocation.
3) Don't touch the Arena allocator and simply add padding in the
ArenaBlockBloomFilterBufferAllocator to minimize any risk to other parts of
the codebase.
Opted for option #2 since it broadly adds support for 32/64 byte alignment
instead of limited scope of option #3. Option #1 is tempting but unsure about
the unknowns that turning on the allocator_aligned_mode would bring.
Although we need only support for 32 byte alignment for AVX operations,
also added support for 64 bytes to better align with cache line size.
Additionally this change:
- Adds a simple BlockBloomFilter unit test that reproduced the alignment
problem compared to end-to-end predicate-test which was turning out to be
difficult to debug.
- Fixes and enhances the arena-test with bunch of variations.
Change-Id: Ib665115fa0fc262a8b76c48f52947dedb84be2a7
---
M src/kudu/util/block_bloom_filter-test.cc
M src/kudu/util/block_bloom_filter.cc
M src/kudu/util/memory/arena-test.cc
M src/kudu/util/memory/arena.cc
M src/kudu/util/memory/arena.h
5 files changed, 103 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/15372/1
--
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: newchange
Gerrit-Change-Id: Ib665115fa0fc262a8b76c48f52947dedb84be2a7
Gerrit-Change-Number: 15372
Gerrit-PatchSet: 1
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15372/4/src/kudu/util/memory/arena.h
File src/kudu/util/memory/arena.h:
http://gerrit.cloudera.org:8080/#/c/15372/4/src/kudu/util/memory/arena.h@406
PS4, Line 406: // Adjusts the supplied "offset" such that the combined "data" ptr and "offset" aligns
: // with "alignment" bytes.
: template<typename T>
: inline T AlignOffset(const uint8_t* data, const T offset, const size_t alignment) {
: const auto data_start_addr = reinterpret_cast<uintptr_t>(data);
: return KUDU_ALIGN_UP((data_start_addr + offset), alignment) - data_start_addr;
: }
Did you mean to duplicate this?
--
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: 4
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 09 Mar 2020 22:49:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15372
to look at the new patch set (#2).
Change subject: [util] Add support for 32 & 64 byte alignment to Arena allocator
......................................................................
[util] Add support for 32 & 64 byte alignment to Arena allocator
On adding couple of member variables and methods to BlockBloomFilter class
for importing Or() function, predicate-test would sometimes randomly crash
with SIGSEGV in AVX operations.
On debugging, the crash would only happen when the "directory_" structure
in BlockBloomFilter is not 32-bytes aligned. It's surprising without the
addition of those methods, "directory_" so far has always been 32 bytes
aligned despite Arena allocator not supporting alignment values greater than
16 bytes.
With input from Todd, explored 3 options to fix the alignment problem:
1) Update the HeapBufferAllocator in util/memory to align allocation to
64 bytes. See AllocateInternal() implementation. Surprisingly the
FLAGS_allocator_aligned_mode is OFF by default so we appear to be relying
on the allocator to always allocate 16 byte aligned buffers. So this option
would require turning ON the FLAGS_allocator_aligned_mode flag by default.
2) Update the Arena allocator such that when needed extra bytes are allocated
to allow aligning with 32/64 bytes considering the new component will always
be 16 byte aligned. This requires updating some address/alignment logic
with offset_ and the new component allocation.
3) Don't touch the Arena allocator and simply add padding in the
ArenaBlockBloomFilterBufferAllocator to minimize any risk to other parts of
the codebase.
Opted for option #2 since it broadly adds support for 32/64 byte alignment
instead of limited scope of option #3. Option #1 is tempting but unsure about
the unknowns that turning on the allocator_aligned_mode would bring.
Although we need only support for 32 byte alignment for AVX operations,
also added support for 64 bytes to better align with cache line size.
Additionally this change:
- Adds a simple BlockBloomFilter unit test that reproduced the alignment
problem compared to end-to-end predicate-test which was turning out to be
difficult to debug.
- Fixes and enhances the arena-test with bunch of variations.
Change-Id: Ib665115fa0fc262a8b76c48f52947dedb84be2a7
---
M src/kudu/util/block_bloom_filter-test.cc
M src/kudu/util/block_bloom_filter.cc
M src/kudu/util/memory/arena-test.cc
M src/kudu/util/memory/arena.cc
M src/kudu/util/memory/arena.h
5 files changed, 161 insertions(+), 51 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/15372/2
--
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: newpatchset
Gerrit-Change-Id: Ib665115fa0fc262a8b76c48f52947dedb84be2a7
Gerrit-Change-Number: 15372
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15372
to look at the new patch set (#3).
Change subject: [util] Add support for 32 & 64 byte alignment to Arena allocator
......................................................................
[util] Add support for 32 & 64 byte alignment to Arena allocator
On adding couple of member variables and methods to BlockBloomFilter class
for importing Or() function, predicate-test would sometimes randomly crash
with SIGSEGV in AVX operations.
On debugging, the crash would only happen when the "directory_" structure
in BlockBloomFilter is not 32-bytes aligned. It's surprising without the
addition of those methods, "directory_" so far has always been 32 bytes
aligned despite Arena allocator not supporting alignment values greater than
16 bytes.
With input from Todd, explored 3 options to fix the alignment problem:
1) Update the HeapBufferAllocator in util/memory to align allocation to
64 bytes. See AllocateInternal() implementation. Surprisingly the
FLAGS_allocator_aligned_mode is OFF by default so we appear to be relying
on the allocator to always allocate 16 byte aligned buffers. So this option
would require turning ON the FLAGS_allocator_aligned_mode flag by default.
2) Update the Arena allocator such that when needed extra bytes are allocated
to allow aligning with 32/64 bytes considering the new component will always
be 16 byte aligned. This requires updating some address/alignment logic
with offset_ and the new component allocation.
3) Don't touch the Arena allocator and simply add padding in the
ArenaBlockBloomFilterBufferAllocator to minimize any risk to other parts of
the codebase.
Opted for option #2 since it broadly adds support for 32/64 byte alignment
instead of limited scope of option #3. Option #1 is tempting but unsure about
the unknowns that turning on the allocator_aligned_mode would bring.
Although we need only support for 32 byte alignment for AVX operations,
also added support for 64 bytes to better align with cache line size.
Additionally this change:
- Adds a simple BlockBloomFilter unit test that reproduced the alignment
problem compared to end-to-end predicate-test which was turning out to be
difficult to debug.
- Fixes and enhances the arena-test with bunch of variations.
Change-Id: Ib665115fa0fc262a8b76c48f52947dedb84be2a7
---
M src/kudu/util/block_bloom_filter-test.cc
M src/kudu/util/block_bloom_filter.cc
M src/kudu/util/memory/arena-test.cc
M src/kudu/util/memory/arena.cc
M src/kudu/util/memory/arena.h
5 files changed, 165 insertions(+), 51 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/15372/3
--
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: newpatchset
Gerrit-Change-Id: Ib665115fa0fc262a8b76c48f52947dedb84be2a7
Gerrit-Change-Number: 15372
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [util] Add support for 32 & 64 byte alignment to Arena allocator
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
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: deleteVote
Gerrit-Change-Id: Ib665115fa0fc262a8b76c48f52947dedb84be2a7
Gerrit-Change-Number: 15372
Gerrit-PatchSet: 5
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15372
to look at the new patch set (#5).
Change subject: [util] Add support for 32 & 64 byte alignment to Arena allocator
......................................................................
[util] Add support for 32 & 64 byte alignment to Arena allocator
On adding couple of member variables and methods to BlockBloomFilter class
for importing Or() function, predicate-test would sometimes randomly crash
with SIGSEGV in AVX operations.
On debugging, the crash would only happen when the "directory_" structure
in BlockBloomFilter is not 32-bytes aligned. It's surprising without the
addition of those methods, "directory_" so far has always been 32 bytes
aligned despite Arena allocator not supporting alignment values greater than
16 bytes.
With input from Todd, explored 3 options to fix the alignment problem:
1) Update the HeapBufferAllocator in util/memory to align allocation to
64 bytes. See AllocateInternal() implementation. Surprisingly the
FLAGS_allocator_aligned_mode is OFF by default so we appear to be relying
on the allocator to always allocate 16 byte aligned buffers. So this option
would require turning ON the FLAGS_allocator_aligned_mode flag by default.
2) Update the Arena allocator such that when needed extra bytes are allocated
to allow aligning with 32/64 bytes considering the new component will always
be 16 byte aligned. This requires updating some address/alignment logic
with offset_ and the new component allocation.
3) Don't touch the Arena allocator and simply add padding in the
ArenaBlockBloomFilterBufferAllocator to minimize any risk to other parts of
the codebase.
Opted for option #2 since it broadly adds support for 32/64 byte alignment
instead of limited scope of option #3. Option #1 is tempting but unsure about
the unknowns that turning on the allocator_aligned_mode would bring.
Although we need only support for 32 byte alignment for AVX operations,
also added support for 64 bytes to better align with cache line size.
Additionally this change:
- Adds a simple BlockBloomFilter unit test that reproduced the alignment
problem compared to end-to-end predicate-test which was turning out to be
difficult to debug.
- Fixes and enhances the arena-test with bunch of variations.
Change-Id: Ib665115fa0fc262a8b76c48f52947dedb84be2a7
---
M src/kudu/util/block_bloom_filter-test.cc
M src/kudu/util/block_bloom_filter.cc
M src/kudu/util/memory/arena-test.cc
M src/kudu/util/memory/arena.cc
M src/kudu/util/memory/arena.h
5 files changed, 167 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/15372/5
--
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: newpatchset
Gerrit-Change-Id: Ib665115fa0fc262a8b76c48f52947dedb84be2a7
Gerrit-Change-Number: 15372
Gerrit-PatchSet: 5
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 05 Mar 2020 23:25:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15372 )
Change subject: [util] Add support for 32 & 64 byte alignment to Arena allocator
......................................................................
[util] Add support for 32 & 64 byte alignment to Arena allocator
On adding couple of member variables and methods to BlockBloomFilter class
for importing Or() function, predicate-test would sometimes randomly crash
with SIGSEGV in AVX operations.
On debugging, the crash would only happen when the "directory_" structure
in BlockBloomFilter is not 32-bytes aligned. It's surprising without the
addition of those methods, "directory_" so far has always been 32 bytes
aligned despite Arena allocator not supporting alignment values greater than
16 bytes.
With input from Todd, explored 3 options to fix the alignment problem:
1) Update the HeapBufferAllocator in util/memory to align allocation to
64 bytes. See AllocateInternal() implementation. Surprisingly the
FLAGS_allocator_aligned_mode is OFF by default so we appear to be relying
on the allocator to always allocate 16 byte aligned buffers. So this option
would require turning ON the FLAGS_allocator_aligned_mode flag by default.
2) Update the Arena allocator such that when needed extra bytes are allocated
to allow aligning with 32/64 bytes considering the new component will always
be 16 byte aligned. This requires updating some address/alignment logic
with offset_ and the new component allocation.
3) Don't touch the Arena allocator and simply add padding in the
ArenaBlockBloomFilterBufferAllocator to minimize any risk to other parts of
the codebase.
Opted for option #2 since it broadly adds support for 32/64 byte alignment
instead of limited scope of option #3. Option #1 is tempting but unsure about
the unknowns that turning on the allocator_aligned_mode would bring.
Although we need only support for 32 byte alignment for AVX operations,
also added support for 64 bytes to better align with cache line size.
Additionally this change:
- Adds a simple BlockBloomFilter unit test that reproduced the alignment
problem compared to end-to-end predicate-test which was turning out to be
difficult to debug.
- Fixes and enhances the arena-test with bunch of variations.
Change-Id: Ib665115fa0fc262a8b76c48f52947dedb84be2a7
Reviewed-on: http://gerrit.cloudera.org:8080/15372
Tested-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/util/block_bloom_filter-test.cc
M src/kudu/util/block_bloom_filter.cc
M src/kudu/util/memory/arena-test.cc
M src/kudu/util/memory/arena.cc
M src/kudu/util/memory/arena.h
5 files changed, 167 insertions(+), 52 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved; Verified
Adar Dembo: Looks good to me, approved
--
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: merged
Gerrit-Change-Id: Ib665115fa0fc262a8b76c48f52947dedb84be2a7
Gerrit-Change-Number: 15372
Gerrit-PatchSet: 6
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 3: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15372/3/src/kudu/util/memory/arena.h
File src/kudu/util/memory/arena.h:
http://gerrit.cloudera.org:8080/#/c/15372/3/src/kudu/util/memory/arena.h@415
PS3, Line 415: // 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_".
Nit: could you move this up to the definition of AlignOffset, so it's clear that it applies to both variants of AllocateBytesAligned?
--
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: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sun, 08 Mar 2020 22:18:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
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 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15372/4/src/kudu/util/memory/arena.h
File src/kudu/util/memory/arena.h:
http://gerrit.cloudera.org:8080/#/c/15372/4/src/kudu/util/memory/arena.h@406
PS4, Line 406: // Adjusts the supplied "offset" such that the combined "data" ptr and "offset" aligns
: // with "alignment" bytes.
: template<typename T>
: inline T AlignOffset(const uint8_t* data, const T offset, const size_t alignment) {
: const auto data_start_addr = reinterpret_cast<uintptr_t>(data);
: return KUDU_ALIGN_UP((data_start_addr + offset), alignment) - data_start_addr;
: }
> Did you mean to duplicate this?
Nope.
--
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: 4
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 09 Mar 2020 22:54:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 5: Code-Review+2
LGTM, maybe Adar has more feedback on this changelist.
--
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: 5
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 10 Mar 2020 04:08:38 +0000
Gerrit-HasComments: No
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 5: Code-Review+2
--
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: 5
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 10 Mar 2020 06:17:23 +0000
Gerrit-HasComments: No
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 06 Mar 2020 06:13:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15372
to look at the new patch set (#4).
Change subject: [util] Add support for 32 & 64 byte alignment to Arena allocator
......................................................................
[util] Add support for 32 & 64 byte alignment to Arena allocator
On adding couple of member variables and methods to BlockBloomFilter class
for importing Or() function, predicate-test would sometimes randomly crash
with SIGSEGV in AVX operations.
On debugging, the crash would only happen when the "directory_" structure
in BlockBloomFilter is not 32-bytes aligned. It's surprising without the
addition of those methods, "directory_" so far has always been 32 bytes
aligned despite Arena allocator not supporting alignment values greater than
16 bytes.
With input from Todd, explored 3 options to fix the alignment problem:
1) Update the HeapBufferAllocator in util/memory to align allocation to
64 bytes. See AllocateInternal() implementation. Surprisingly the
FLAGS_allocator_aligned_mode is OFF by default so we appear to be relying
on the allocator to always allocate 16 byte aligned buffers. So this option
would require turning ON the FLAGS_allocator_aligned_mode flag by default.
2) Update the Arena allocator such that when needed extra bytes are allocated
to allow aligning with 32/64 bytes considering the new component will always
be 16 byte aligned. This requires updating some address/alignment logic
with offset_ and the new component allocation.
3) Don't touch the Arena allocator and simply add padding in the
ArenaBlockBloomFilterBufferAllocator to minimize any risk to other parts of
the codebase.
Opted for option #2 since it broadly adds support for 32/64 byte alignment
instead of limited scope of option #3. Option #1 is tempting but unsure about
the unknowns that turning on the allocator_aligned_mode would bring.
Although we need only support for 32 byte alignment for AVX operations,
also added support for 64 bytes to better align with cache line size.
Additionally this change:
- Adds a simple BlockBloomFilter unit test that reproduced the alignment
problem compared to end-to-end predicate-test which was turning out to be
difficult to debug.
- Fixes and enhances the arena-test with bunch of variations.
Change-Id: Ib665115fa0fc262a8b76c48f52947dedb84be2a7
---
M src/kudu/util/block_bloom_filter-test.cc
M src/kudu/util/block_bloom_filter.cc
M src/kudu/util/memory/arena-test.cc
M src/kudu/util/memory/arena.cc
M src/kudu/util/memory/arena.h
5 files changed, 174 insertions(+), 51 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/15372/4
--
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: newpatchset
Gerrit-Change-Id: Ib665115fa0fc262a8b76c48f52947dedb84be2a7
Gerrit-Change-Number: 15372
Gerrit-PatchSet: 4
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
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 5:
> Patch Set 5: Verified-1
>
> Build Failed
>
> http://jenkins.kudu.apache.org/job/kudu-gerrit/20738/ : FAILURE
Unrelated test failure "time_anomalies-itest.0"
http://dist-test.cloudera.org/job?job_id=jenkins-slave.1583796856.26307#
--
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: 5
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 10 Mar 2020 02:05:34 +0000
Gerrit-HasComments: No
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
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 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15372/3/src/kudu/util/memory/arena.h
File src/kudu/util/memory/arena.h:
http://gerrit.cloudera.org:8080/#/c/15372/3/src/kudu/util/memory/arena.h@415
PS3, Line 415: // 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_".
> Nit: could you move this up to the definition of AlignOffset, so it's clear
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: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 09 Mar 2020 22:47:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 5: Verified+1
Unrelated isolate fluke while trying to run time_anomalies-itest scenario:
failed to download task files: WARNING 142 isolateserver(1484): Adding unknown file 5ecae781fdd0a497573024d6683b2be7d8064807 to cache
WARNING 148 isolateserver(1490): Added back 1 unknown files
INFO 160 tools(106): Profiling: Section Setup took 0.069 seconds
INFO 205 tools(106): Profiling: Section GetIsolateds took 0.045 seconds
INFO 209 tools(106): Profiling: Section GetRest took 0.004 seconds
INFO 222 isolateserver(1365): 1 ( 37776kb) added
INFO 223 isolateserver(1369): 2571 ( 7464720kb) current
INFO 223 isolateserver(1372): 0 ( 0kb) removed
INFO 223 isolateserver(1375): 40230544kb free
INFO 223 tools(106): Profiling: Section CleanupTrimming took 0.014 seconds
INFO 224 isolateserver(1365): 1 ( 37776kb) added
INFO 225 isolateserver(1369): 2571 ( 7464720kb) current
INFO 225 isolateserver(1372): 0 ( 0kb) removed
INFO 225 isolateserver(1375): 40230544kb free
INFO 225 tools(106): Profiling: Section CleanupTrimming took 0.001 seconds
INFO 225 isolateserver(381): Waiting for all threads to die...
INFO 225 isolateserver(390): Done.
Traceback (most recent call last):
File "/swarming.client/isolateserver.py", line 2211, in <module>
sys.exit(main(sys.argv[1:]))
File "/swarming.client/isolateserver.py", line 2204, in main
return dispatcher.execute(OptionParserIsolateServer(), args)
File "/swarming.client/third_party/depot_tools/subcommand.py", line 242, in execute
return command(parser, args[1:])
File "/swarming.client/isolateserver.py", line 2064, in CMDdownload
require_command=False)
File "/swarming.client/isolateserver.py", line 1827, in fetch_isolated
create_directories(outdir, bundle.files)
File "/swarming.client/isolateserver.py", line 212, in create_directories
os.mkdir(os.path.join(base_directory, d))
OSError: [Errno 17] File exists: '/tmp/dist-test-tasknKfZ6F/build'
--
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: 5
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 10 Mar 2020 04:07:06 +0000
Gerrit-HasComments: No
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
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 <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 07 Mar 2020 01:17:20 +0000
Gerrit-HasComments: Yes