[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
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 Reviewed-by: Alexey Serbin Reviewed-by: Adar Dembo --- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 10 Mar 2020 06:17:23 + Gerrit-HasComments: No
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 10 Mar 2020 04:08:38 + Gerrit-HasComments: No
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
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: WARNING142 isolateserver(1484): Adding unknown file 5ecae781fdd0a497573024d6683b2be7d8064807 to cache WARNING148 isolateserver(1490): Added back 1 unknown files INFO160 tools(106): Profiling: Section Setup took 0.069 seconds INFO205 tools(106): Profiling: Section GetIsolateds took 0.045 seconds INFO209 tools(106): Profiling: Section GetRest took 0.004 seconds INFO222 isolateserver(1365): 1 ( 37776kb) added INFO223 isolateserver(1369): 2571 ( 7464720kb) current INFO223 isolateserver(1372): 0 ( 0kb) removed INFO223 isolateserver(1375):40230544kb free INFO223 tools(106): Profiling: Section CleanupTrimming took 0.014 seconds INFO224 isolateserver(1365): 1 ( 37776kb) added INFO225 isolateserver(1369): 2571 ( 7464720kb) current INFO225 isolateserver(1372): 0 ( 0kb) removed INFO225 isolateserver(1375):40230544kb free INFO225 tools(106): Profiling: Section CleanupTrimming took 0.001 seconds INFO225 isolateserver(381): Waiting for all threads to die... INFO225 isolateserver(390): Done. Traceback (most recent call last): File "/swarming.client/isolateserver.py", line 2211, in 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 10 Mar 2020 04:07:06 + Gerrit-HasComments: No
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 10 Mar 2020 02:05:34 + Gerrit-HasComments: No
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
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
: inline T AlignOffset(const uint8_t* data, const T offset, const
size_t alignment) {
: const auto data_start_addr = reinterpret_cast(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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Mon, 09 Mar 2020 22:54:11 +
Gerrit-HasComments: Yes
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
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
: inline T AlignOffset(const uint8_t* data, const T offset, const
size_t alignment) {
: const auto data_start_addr = reinterpret_cast(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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Mon, 09 Mar 2020 22:49:42 +
Gerrit-HasComments: Yes
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 09 Mar 2020 22:47:29 + Gerrit-HasComments: Yes
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Sun, 08 Mar 2020 22:18:25 + Gerrit-HasComments: Yes
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
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(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(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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Sat, 07 Mar 2020 01:17:20 +
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 06 Mar 2020 06:13:33 + Gerrit-HasComments: Yes
[kudu-CR] [util] Add support for 32 & 64 byte alignment to Arena allocator
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(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(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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 05 Mar 2020 23:25:29 + Gerrit-HasComments: Yes
[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
