ARROW-8413: [C++][Parquet] Refactor Generating validity bitmap for values column by emkornfield · Pull Request #6985 · apache/arrow

emkornfield · 2020-04-20T00:18:50Z

This change does the following:

Vectorizes computation for validity bitmaps with no repeated parents for all little-endian architectures
Vectorizes computation for validity bitmaps with repeated parents for AVX2 + architectures (really it requires BMI2)
Exposes some building blocks to do vectorized computation for all validity bitmaps in for parquet level data (more need to be handled for generating appropriate bitmaps and offset data for lists). These will be added level_conversions.h in a future PR.
Replaces loops over bitmaps with SetBitsTo
Leaves a fallback for non BMI2/little-endian capable machines.

With AVX2 enabled this seems to improve benchmarks by 20% for nullable columns (BM_Read..) on my box. I didn't see any impacts in other benchmarkmarks.

See checklist for what is remaining. If possible i'd like to get early feedback on naming and my approach to checking for BMI2.

Still needed:

Still adding more focused unit tests for what I've added.
Move the changes to Bitmap::ToString to there own PR.

github-actions · 2020-04-20T00:31:27Z

https://issues.apache.org/jira/browse/ARROW-8413

kiszk · 2020-04-22T13:35:18Z

cpp/src/parquet/CMakeLists.txt

nit: indentation

kiszk · 2020-04-22T13:38:51Z

cpp/src/parquet/column_reader.cc

nit: AVx2 -> AVX2, o detect -> to detect, BIM2 -> BMI2

Remove the comment as it was stale.

cpp/src/arrow/util/bit_util.h

cpp/cmake_modules/SetupCxxFlags.cmake

cpp/src/parquet/column_reader.cc

emkornfield · 2020-04-23T07:30:36Z

CC @wesm @pitrou I think this is ready for review now. Will take a closer look at CI failures tomorrow.

pitrou

I can't comment on the correctness of the levels handling (someone competent will have to review). I would like to see some code cleanup and organization changes, though.

cpp/cmake_modules/SetupCxxFlags.cmake

pitrou · 2020-04-23T10:22:24Z

cpp/cmake_modules/SetupCxxFlags.cmake

This doesn't seem useful :-)

should be removed now.

pitrou · 2020-04-23T10:24:14Z

cpp/src/parquet/column_reader.cc

I think it would be nice to move the low-level handling of levels to a dedicated C++ file.

I've moved all everything in the anonymous namespace to level_conversion.cc

Note that this prevents inlining of the Scalar version which was happening before, but I'm not sure that is a big deal.

pitrou · 2020-04-23T10:26:39Z

cpp/src/parquet/column_reader.cc

"instructions"

pitrou · 2020-04-23T10:29:47Z

cpp/src/parquet/level_conversion.h

"corresponding"?

yes. fixed.

pitrou · 2020-04-23T10:43:28Z

cpp/src/parquet/level_conversion.h

I don't think it makes sense for this to be a helper function.

I agree now that the AppendWord is moved to FirstTimeBitmapWriter

pitrou · 2020-04-23T10:43:48Z

cpp/src/parquet/level_conversion.h

Then please only define this function if ARROW_HAVE_BMI2 is defined.

pitrou · 2020-04-23T10:44:11Z

cpp/src/parquet/level_conversion_test.cc

Please, a more specific name (such as BitmapToString).

pitrou · 2020-04-23T10:44:17Z

cpp/src/parquet/level_conversion_test.cc

Ditto here.

pitrou · 2020-04-23T10:44:49Z

cpp/src/parquet/level_conversion_test.cc

Then just don't define the test.

good point. done.

kiszk · 2020-04-23T15:40:06Z

cpp/src/parquet/column_reader.cc

typo: max_repitition_level -> max_repetition_level

kiszk · 2020-04-23T15:43:07Z

cpp/src/parquet/column_reader.cc

Do we need to move these two lines before line 186?

I think just 1.

kiszk · 2020-04-23T16:41:26Z

cpp/src/parquet/column_reader.cc

Since this path is executed only when #if defined(ARROW_HAVE_BMI2) is true, it would be good to add such as code

#if defined(ARROW_HAVE_BMI2) ... #else assert(false && "must not execute this without BMI2"); #endif

kiszk · 2020-04-23T16:42:54Z

cpp/src/parquet/level_conversion_test.cc

FYI: Most of these tests fail on big-endian platform

[ FAILED ] TestAppendBitmap.TestOffsetOverwritesCorrectBitsOnExistingByte [ FAILED ] TestAppendBitmap.TestOffsetShiftBitsCorrectly [ FAILED ] TestAppendBitmap.AllBytesAreWrittenWithEnoughSpace [ FAILED ] TestAppendBitmap.OnlyApproriateBytesWrittenWhenLessThen8BytesAvailable [ FAILED ] TestAppendToValidityBitmap.BasicOperation

There was a substantial refactoring, I tried to provide guards but without a big-endian machine in CI it will be very hard to to catch all of these issues.

kiszk · 2020-04-23T16:51:35Z

cpp/src/parquet/level_conversion.h

Why do we need to use union? bytes[] is used only at line 106.
It looks simple to explicitly extract the lowest 8bit instead of using union.

I originally thought I would have to do more. I removed the union.

emkornfield · 2020-04-26T05:57:42Z

@pitrou I think I addressed your comments. One of them that went stale was the complexity for "AppendWord", I tried to remove parts that did not seem to affect performance on benchmarks for parquet column reading but I spent some time trying to maximize word level parallelism for the unaligned case, because I think at least for repeated fields I expect this case to be fairly common.

As a point of comparison using the AppendWord implementation I put in place for BigEndian shows much smaller improvements. Really this should use CopyBitmap but I didn't want to start moving a move code then I needed to in bit_util.h for this PR. I opened
a JIRA (ARROW-8595) to track some improvements in that regard.

If more comments are needed or you think there is a cleaner way of writing the code, I'm happy for input.

@wesm do you want to look at the parquet specific logic/comments to make sure I captured them correctly?

wesm · 2020-04-27T00:51:55Z

I'll try to have a closer look tomorrow or Tuesday

emkornfield · 2020-04-30T16:52:43Z

@wesm wanted to make sure this is still on your radar

wesm · 2020-04-30T17:25:31Z

Yep sorry thanks for the nudge, will look today

wesm

At a high level this looks good.

One problem with introducing more SIMD code is that we won't yet have a runtime dispatching strategy. We will need to go through all of our SIMD accelerations in this library and refactor things so that we can build a "fat" binary that includes both AVX2/BMI2-accelerated versions and the non-SIMD versions. That way we can reap the benefits of this work in portable packages like Python wheels / conda packages.

wesm · 2020-04-30T22:20:13Z

cpp/src/arrow/util/bit_util.h

remove this?

wesm · 2020-04-30T22:23:08Z

cpp/src/arrow/util/bit_util.h

This section is a bit too much bit manipulation for me to carefully scrutinize but I trust you. There are some typos in the comments

OK, if there is anything I can add for documentation please let me know.

This is cryptic. Can you rewrite it in a more understandable way? For example (untested):

if (bit_mask_ == 0x1) { current_byte_ = 0; } else { current_byte_ = *(append_position + bytes_for_word - 1); }

wesm · 2020-04-30T22:41:01Z

I think this is fine to merge once most of the typos in the comments are fixed. A rebase will probably fix the Rust lint error

emkornfield

rebased.

emkornfield · 2020-05-01T04:47:15Z

cpp/src/arrow/util/bit_util.h

emkornfield · 2020-05-01T05:16:36Z

cpp/src/arrow/util/bit_util.h

OK, if there is anything I can add for documentation please let me know.

This reverts commit 59e39794974ebade0cff0ba702b6cbc9cbd43b87.

emkornfield · 2020-06-04T05:59:01Z

@pitrou I think the C GLib build failure looks unrelated to me. I fixed the s390x build.

emkornfield · 2020-06-04T05:59:54Z

@wesm I expect your PR to get merged first but if it doesn't consider using PopCount methods in this one.

wesm · 2020-06-04T23:54:54Z

+1. I'm going to go ahead and merge this and then I'll rebase #7352

emkornfield force-pushed the ARROW-8413 branch 2 times, most recently from 39280c5 to fe2f609 Compare April 20, 2020 03:25

wesm changed the title ~~ARROW-8413: [C++][WIP] Refactor Generating validity bitmap for values column~~ ARROW-8413: [C++][Parquet][WIP] Refactor Generating validity bitmap for values column Apr 20, 2020

kszucs force-pushed the master branch from 1b71ca7 to 5093b80 Compare April 20, 2020 19:21

emkornfield force-pushed the ARROW-8413 branch from adcd336 to 77b38a2 Compare April 21, 2020 05:42

kiszk reviewed Apr 22, 2020

View reviewed changes

fsaintjacques reviewed Apr 23, 2020

View reviewed changes

cpp/src/arrow/util/bit_util.h Outdated Show resolved Hide resolved

emkornfield commented Apr 23, 2020

View reviewed changes

cpp/cmake_modules/SetupCxxFlags.cmake Outdated Show resolved Hide resolved

emkornfield commented Apr 23, 2020

View reviewed changes

cpp/src/parquet/column_reader.cc Outdated Show resolved Hide resolved

emkornfield changed the title ~~ARROW-8413: [C++][Parquet][WIP] Refactor Generating validity bitmap for values column~~ ARROW-8413: [C++][Parquet] Refactor Generating validity bitmap for values column Apr 23, 2020

pitrou requested changes Apr 23, 2020

View reviewed changes

kiszk reviewed Apr 23, 2020

View reviewed changes

emkornfield requested a review from pitrou April 26, 2020 05:58

emkornfield mentioned this pull request Apr 26, 2020

ARROW-8440: [C++] Refine SIMD header files #6954

Closed

wesm self-requested a review April 27, 2020 00:51

wesm reviewed Apr 30, 2020

View reviewed changes

emkornfield force-pushed the ARROW-8413 branch from dfa64bd to f41d8f1 Compare May 1, 2020 05:18

emkornfield commented May 1, 2020

View reviewed changes

emkornfield and others added 19 commits June 3, 2020 05:04

add missing file

9aeb97d

move docs inside defined guard

435523f

fix lint and some other unnecessary includes

4226e16

reword/try to fix typos in docs

a362883

try suggested R fix

439cb01

Revert "try suggested R fix"

e1d0b78

This reverts commit 59e39794974ebade0cff0ba702b6cbc9cbd43b87.

simplify AppendWord

66e8595

add tests for offsets correct typo

c0baaa9

add tests

d07849d

address the rest of feedbac

c82ac10

static assert failure on big endian

1593f9e

fix comparison

d541d3a

Refactor DefinitionLevelsBatchToBitmap signature and reformat

0f57459

add repeated benchmark

858ef01

update benchmark

0113e02

add header and some includes

9047390

only use bmi2 on avx512 to prevent use on AMD

2b8be0c

fix size argument

74a6aa9

address comments

fe9263b

emkornfield force-pushed the ARROW-8413 branch from 5c76623 to fe9263b Compare June 3, 2020 05:12

try to fix s390x

a0d93c2

emkornfield mentioned this pull request Jun 4, 2020

ARROW-9032: [C++] Split up arrow/util/bit_util.h into multiple header files #7352

Closed

wesm closed this in 5389008 Jun 4, 2020

emkornfield mentioned this pull request Sep 10, 2020

ARROW-9810: [C++] Generalized nested reconstruction helpers #8156

Closed

asfimport mentioned this pull request Jun 4, 2020

[C++] Refactor DefLevelsToBitmap #24595

Closed

This was referenced Apr 27, 2020

[C++]Expose API for pushing down rep/def level comparison down to decoder #24765

Closed

[C++] Rearrange code in bit-util.h/.cc for AppendWord #24760

Closed

Conversation

emkornfield commented Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emkornfield commented Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emkornfield commented Apr 20, 2020 •

edited

Loading

emkornfield commented Apr 23, 2020 •

edited

Loading

kiszk Apr 23, 2020 •

edited

Loading