Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
0a7e537
compiling
emkornfield Apr 17, 2020
83c38c4
remove grouping
emkornfield Apr 20, 2020
3480711
fix msvc
emkornfield Apr 20, 2020
677c81f
more msvc fixes
emkornfield Apr 20, 2020
a8e7637
move fixes
emkornfield Apr 20, 2020
0d2ebd2
one more fix
emkornfield Apr 20, 2020
e69d87a
change not to !
emkornfield Apr 21, 2020
ee20f7a
put back missing value
emkornfield Apr 21, 2020
bdc1333
better names cleanup docs
emkornfield Apr 22, 2020
5b5df2a
add some tests more needed
emkornfield Apr 22, 2020
fa4e364
actually add test
emkornfield Apr 22, 2020
a9c8b05
address comments
emkornfield Apr 23, 2020
9b3bbeb
test GreaterThanBitmap
emkornfield Apr 23, 2020
91f6814
add shift tests
emkornfield Apr 23, 2020
0fef43b
add remaining unit tests
emkornfield Apr 23, 2020
44411f3
skip selection test if no BMI2
emkornfield Apr 23, 2020
04109d4
change back to -mavx2 and add -mbmi2
emkornfield Apr 24, 2020
f08d836
fix format
emkornfield Apr 24, 2020
6a08225
address more comments
emkornfield Apr 24, 2020
189403d
Address remaining comments.
emkornfield Apr 25, 2020
9aeb97d
add missing file
emkornfield Apr 26, 2020
435523f
move docs inside defined guard
emkornfield Apr 26, 2020
4226e16
fix lint and some other unnecessary includes
emkornfield Apr 26, 2020
a362883
reword/try to fix typos in docs
emkornfield May 1, 2020
439cb01
try suggested R fix
emkornfield May 4, 2020
e1d0b78
Revert "try suggested R fix"
emkornfield May 5, 2020
66e8595
simplify AppendWord
emkornfield May 5, 2020
c0baaa9
add tests for offsets correct typo
emkornfield May 5, 2020
d07849d
add tests
emkornfield May 6, 2020
c82ac10
address the rest of feedbac
emkornfield May 6, 2020
1593f9e
static assert failure on big endian
emkornfield May 6, 2020
d541d3a
fix comparison
emkornfield May 6, 2020
0f57459
Refactor DefinitionLevelsBatchToBitmap signature and reformat
fsaintjacques May 6, 2020
858ef01
add repeated benchmark
emkornfield May 17, 2020
0113e02
update benchmark
emkornfield May 17, 2020
9047390
add header and some includes
emkornfield May 17, 2020
2b8be0c
only use bmi2 on avx512 to prevent use on AMD
emkornfield May 22, 2020
74a6aa9
fix size argument
emkornfield Jun 2, 2020
fe9263b
address comments
emkornfield Jun 3, 2020
a0d93c2
try to fix s390x
emkornfield Jun 4, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions cpp/cmake_modules/SetupCxxFlags.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ if(ARROW_CPU_FLAG STREQUAL "x86")
set(ARROW_SSE4_2_FLAG "-msse4.2")
set(ARROW_AVX2_FLAG "-mavx2")
# skylake-avx512 consists of AVX512F,AVX512BW,AVX512VL,AVX512CD,AVX512DQ
set(ARROW_AVX512_FLAG "-march=skylake-avx512")
set(ARROW_AVX512_FLAG "-march=skylake-avx512 -mbmi2")
check_cxx_compiler_flag(${ARROW_SSE4_2_FLAG} CXX_SUPPORTS_SSE4_2)
endif()
check_cxx_compiler_flag(${ARROW_AVX2_FLAG} CXX_SUPPORTS_AVX2)
Expand Down Expand Up @@ -313,13 +313,14 @@ if(ARROW_CPU_FLAG STREQUAL "x86")
message(FATAL_ERROR "AVX512 required but compiler doesn't support it.")
endif()
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} ${ARROW_AVX512_FLAG}")
add_definitions(-DARROW_HAVE_AVX512 -DARROW_HAVE_AVX2 -DARROW_HAVE_SSE4_2)
add_definitions(-DARROW_HAVE_AVX512 -DARROW_HAVE_AVX2 -DARROW_HAVE_BMI2
-DARROW_HAVE_SSE4_2)
elseif(ARROW_SIMD_LEVEL STREQUAL "AVX2")
if(NOT CXX_SUPPORTS_AVX2)
message(FATAL_ERROR "AVX2 required but compiler doesn't support it.")
endif()
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} ${ARROW_AVX2_FLAG}")
add_definitions(-DARROW_HAVE_AVX2 -DARROW_HAVE_SSE4_2)
add_definitions(-DARROW_HAVE_AVX2 -DARROW_HAVE_BMI2 -DARROW_HAVE_SSE4_2)
elseif(ARROW_SIMD_LEVEL STREQUAL "SSE4_2")
if(NOT CXX_SUPPORTS_SSE4_2)
message(FATAL_ERROR "SSE4.2 required but compiler doesn't support it.")
Expand Down
10 changes: 1 addition & 9 deletions cpp/src/arrow/util/bit_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,6 @@
// specific language governing permissions and limitations
// under the License.

// Alias MSVC popcount to GCC name
#ifdef _MSC_VER
#include <intrin.h>
#define __builtin_popcount __popcnt
#include <nmmintrin.h>
#define __builtin_popcountll _mm_popcnt_u64
#endif

#include <algorithm>
#include <cstdint>
#include <cstring>
Expand Down Expand Up @@ -131,7 +123,7 @@ int64_t CountSetBits(const uint8_t* data, int64_t bit_offset, int64_t length) {
const uint64_t* end = u64_data + p.aligned_words;

for (auto iter = u64_data; iter < end; ++iter) {
count += __builtin_popcountll(*iter);
count += BitUtil::PopCount(*iter);
}
}

Expand Down
60 changes: 59 additions & 1 deletion cpp/src/arrow/util/bit_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,18 @@

#if defined(_MSC_VER)
#include <intrin.h> // IWYU pragma: keep
#include <nmmintrin.h>
#pragma intrinsic(_BitScanReverse)
#pragma intrinsic(_BitScanForward)
#define ARROW_BYTE_SWAP64 _byteswap_uint64
#define ARROW_BYTE_SWAP32 _byteswap_ulong
#define ARROW_POPCOUNT64 __popcnt64
#define ARROW_POPCOUNT32 __popcnt
#else
#define ARROW_BYTE_SWAP64 __builtin_bswap64
#define ARROW_BYTE_SWAP32 __builtin_bswap32
#define ARROW_POPCOUNT64 __builtin_popcountll
#define ARROW_POPCOUNT32 __builtin_popcount
#endif

#include <algorithm>
Expand Down Expand Up @@ -107,6 +112,9 @@ static constexpr uint8_t kBytePopcount[] = {
5, 4, 5, 5, 6, 3, 4, 4, 5, 4, 5, 5, 6, 4, 5, 5, 6, 5, 6, 6, 7, 3, 4, 4, 5, 4, 5, 5, 6,
4, 5, 5, 6, 5, 6, 6, 7, 4, 5, 5, 6, 5, 6, 6, 7, 5, 6, 6, 7, 6, 7, 7, 8};

static inline uint64_t PopCount(uint64_t bitmap) { return ARROW_POPCOUNT64(bitmap); }
static inline uint32_t PopCount(uint32_t bitmap) { return ARROW_POPCOUNT32(bitmap); }

//
// Bit-related computations on integer values
//
Expand Down Expand Up @@ -578,6 +586,56 @@ class FirstTimeBitmapWriter {
}
}

/// Appends number_of_bits from word to valid_bits and valid_bits_offset.
///
/// \param[in] word The LSB bitmap to append. Any bits past number_of_bits are assumed
/// to be unset (i.e. 0).
/// \param[in] number_of_bits The number of bits to append from word.
void AppendWord(uint64_t word, int64_t number_of_bits) {
if (ARROW_PREDICT_FALSE(number_of_bits == 0)) {
return;
}

// Location that the first byte needs to be written to.
uint8_t* append_position = bitmap_ + byte_offset_;

// Update state variables except for current_byte_ here.
position_ += number_of_bits;
int64_t bit_offset = BitUtil::CountTrailingZeros(static_cast<uint32_t>(bit_mask_));
bit_mask_ = BitUtil::kBitmask[(bit_offset + number_of_bits) % 8];
byte_offset_ += (bit_offset + number_of_bits) / 8;

if (bit_offset != 0) {
// We are in the middle of the byte. This code updates the byte and shifts
// bits appropriately within word so it can be memcpy'd below.
int64_t bits_to_carry = 8 - bit_offset;
// Carry over bits from word to current_byte_. We assume any extra bits in word
// unset so no additional accounting is needed for when number_of_bits <
// bits_to_carry.
current_byte_ |= (word & BitUtil::kPrecedingBitmask[bits_to_carry]) << bit_offset;
// Check if everything is transfered into current_byte_.
if (ARROW_PREDICT_FALSE(number_of_bits < bits_to_carry)) {
return;
}
*append_position = current_byte_;
append_position++;
// Move the carry bits off of word.
word = word >> bits_to_carry;
number_of_bits -= bits_to_carry;
}
word = BitUtil::ToLittleEndian(word);
int64_t bytes_for_word = ::arrow::BitUtil::BytesForBits(number_of_bits);
std::memcpy(append_position, &word, bytes_for_word);
// At this point, the previous current_byte_ has been written to bitmap_.
// The new current_byte_ is either the last relevant byte in 'word'
// or cleared if the new position is byte aligned (i.e. a fresh byte).
if (bit_mask_ == 0x1) {
current_byte_ = 0;
} else {
current_byte_ = *(append_position + bytes_for_word - 1);
}
}

void Set() { current_byte_ |= bit_mask_; }

void Clear() {}
Expand All @@ -594,7 +652,7 @@ class FirstTimeBitmapWriter {
}

void Finish() {
// Store current byte if we didn't went past bitmap storage
// Store current byte if we didn't go past bitmap storage
if (length_ > 0 && (bit_mask_ != 0x01 || position_ < length_)) {
bitmap_[byte_offset_] = current_byte_;
}
Expand Down
181 changes: 181 additions & 0 deletions cpp/src/arrow/util/bit_util_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include <string>
#include <vector>

#include <gmock/gmock.h>
#include <gtest/gtest.h>

#include "arrow/buffer.h"
Expand All @@ -46,6 +47,8 @@ using internal::CopyBitmap;
using internal::CountSetBits;
using internal::InvertBitmap;

using ::testing::ElementsAreArray;

template <class BitmapWriter>
void WriteVectorToWriter(BitmapWriter& writer, const std::vector<int> values) {
for (const auto& value : values) {
Expand Down Expand Up @@ -315,6 +318,184 @@ TEST(FirstTimeBitmapWriter, NormalOperation) {
}
}

std::string BitmapToString(const uint8_t* bitmap, int64_t bit_count) {
return arrow::internal::Bitmap(bitmap, /*offset*/ 0, /*length=*/bit_count).ToString();
}

std::string BitmapToString(const std::vector<uint8_t>& bitmap, int64_t bit_count) {
return BitmapToString(bitmap.data(), bit_count);
}

TEST(FirstTimeBitmapWriter, AppendWordOffsetOverwritesCorrectBitsOnExistingByte) {
auto check_append = [](const std::string& expected_bits, int64_t offset) {
std::vector<uint8_t> valid_bits = {0x00};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For each of the tests here it would be nice to have two variants or iterations: one that starts with a bitmap full of 0s, and one with a bitmap full of 1s.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added variants to this test. Note that I don't think have a bitmap full of 111s iand overwriting trailing ones isn't really expected because any not processed are expected to be zero.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, FirstTimeBitmapWriter is supposed to work on uninitialized data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I misunderstood the contract. updated tests.

constexpr int64_t kBitsAfterAppend = 8;
internal::FirstTimeBitmapWriter writer(valid_bits.data(), offset,
/*length=*/(8 * valid_bits.size()) - offset);
writer.AppendWord(/*word=*/0xFF, /*number_of_bits=*/kBitsAfterAppend - offset);
writer.Finish();
EXPECT_EQ(BitmapToString(valid_bits, kBitsAfterAppend), expected_bits);
};
check_append("11111111", 0);
check_append("01111111", 1);
check_append("00111111", 2);
check_append("00011111", 3);
check_append("00001111", 4);
check_append("00000111", 5);
check_append("00000011", 6);
check_append("00000001", 7);

auto check_with_set = [](const std::string& expected_bits, int64_t offset) {
std::vector<uint8_t> valid_bits = {0x1};
constexpr int64_t kBitsAfterAppend = 8;
internal::FirstTimeBitmapWriter writer(valid_bits.data(), offset,
/*length=*/(8 * valid_bits.size()) - offset);
writer.AppendWord(/*word=*/0xFF, /*number_of_bits=*/kBitsAfterAppend - offset);
writer.Finish();
EXPECT_EQ(BitmapToString(valid_bits, kBitsAfterAppend), expected_bits);
};
// 0ffset zero would not be a valid mask.
check_with_set("11111111", 1);
check_with_set("10111111", 2);
check_with_set("10011111", 3);
check_with_set("10001111", 4);
check_with_set("10000111", 5);
check_with_set("10000011", 6);
check_with_set("10000001", 7);

auto check_with_preceding = [](const std::string& expected_bits, int64_t offset) {
std::vector<uint8_t> valid_bits = {0xFF};
constexpr int64_t kBitsAfterAppend = 8;
internal::FirstTimeBitmapWriter writer(valid_bits.data(), offset,
/*length=*/(8 * valid_bits.size()) - offset);
writer.AppendWord(/*word=*/0xFF, /*number_of_bits=*/kBitsAfterAppend - offset);
writer.Finish();
EXPECT_EQ(BitmapToString(valid_bits, kBitsAfterAppend), expected_bits);
};
check_with_preceding("11111111", 0);
check_with_preceding("11111111", 1);
check_with_preceding("11111111", 2);
check_with_preceding("11111111", 3);
check_with_preceding("11111111", 4);
check_with_preceding("11111111", 5);
check_with_preceding("11111111", 6);
check_with_preceding("11111111", 7);
}

TEST(FirstTimeBitmapWriter, AppendZeroBitsHasNoImpact) {
std::vector<uint8_t> valid_bits(/*count=*/1, 0);
internal::FirstTimeBitmapWriter writer(valid_bits.data(), /*start_offset=*/1,
/*length=*/valid_bits.size() * 8);
writer.AppendWord(/*word=*/0xFF, /*number_of_bits=*/0);
writer.AppendWord(/*word=*/0xFF, /*number_of_bits=*/0);
writer.AppendWord(/*word=*/0x01, /*number_of_bits=*/1);
writer.Finish();
EXPECT_EQ(valid_bits[0], 0x2);
}

TEST(FirstTimeBitmapWriter, AppendLessThanByte) {
{
std::vector<uint8_t> valid_bits(/*count*/ 8, 0);
internal::FirstTimeBitmapWriter writer(valid_bits.data(), /*start_offset=*/1,
/*length=*/8);
writer.AppendWord(0xB, 4);
writer.Finish();
EXPECT_EQ(BitmapToString(valid_bits, /*bit_count=*/8), "01101000");
}
{
// Test with all bits initially set.
std::vector<uint8_t> valid_bits(/*count*/ 8, 0xFF);
internal::FirstTimeBitmapWriter writer(valid_bits.data(), /*start_offset=*/1,
/*length=*/8);
writer.AppendWord(0xB, 4);
writer.Finish();
EXPECT_EQ(BitmapToString(valid_bits, /*bit_count=*/8), "11101000");
}
}

TEST(FirstTimeBitmapWriter, AppendByteThenMore) {
{
std::vector<uint8_t> valid_bits(/*count*/ 8, 0);
internal::FirstTimeBitmapWriter writer(valid_bits.data(), /*start_offset=*/0,
/*length=*/9);
writer.AppendWord(0xC3, 8);
writer.AppendWord(0x01, 1);
writer.Finish();
EXPECT_EQ(BitmapToString(valid_bits, /*bit_count=*/9), "11000011 1");
}
{
std::vector<uint8_t> valid_bits(/*count*/ 8, 0xFF);
internal::FirstTimeBitmapWriter writer(valid_bits.data(), /*start_offset=*/0,
/*length=*/9);
writer.AppendWord(0xC3, 8);
writer.AppendWord(0x01, 1);
writer.Finish();
EXPECT_EQ(BitmapToString(valid_bits, /*bit_count=*/9), "11000011 1");
}
}

TEST(FirstTimeBitmapWriter, AppendWordShiftsBitsCorrectly) {
constexpr uint64_t kPattern = 0x9A9A9A9A9A9A9A9A;
auto check_append = [&](const std::string& leading_bits, const std::string& middle_bits,
const std::string& trailing_bits, int64_t offset,
bool preset_buffer_bits = false) {
ASSERT_GE(offset, 8);
std::vector<uint8_t> valid_bits(/*count=*/10, preset_buffer_bits ? 0xFF : 0);
valid_bits[0] = 0x99;
internal::FirstTimeBitmapWriter writer(valid_bits.data(), offset,
/*length=*/(9 * sizeof(kPattern)) - offset);
writer.AppendWord(/*word=*/kPattern, /*number_of_bits=*/64);
writer.Finish();
EXPECT_EQ(valid_bits[0], 0x99); // shouldn't get changed.
EXPECT_EQ(BitmapToString(valid_bits.data() + 1, /*num_bits=*/8), leading_bits);
for (int x = 2; x < 9; x++) {
EXPECT_EQ(BitmapToString(valid_bits.data() + x, /*num_bits=*/8), middle_bits)
<< "x: " << x << " " << offset << " " << BitmapToString(valid_bits.data(), 80);
}
EXPECT_EQ(BitmapToString(valid_bits.data() + 9, /*num_bits=*/8), trailing_bits);
};
// Original Pattern = "01011001"
check_append(/*leading_bits= */ "01011001", /*middle_bits=*/"01011001",
/*trailing_bits=*/"00000000", /*offset=*/8);
check_append("00101100", "10101100", "10000000", 9);
check_append("00010110", "01010110", "01000000", 10);
check_append("00001011", "00101011", "00100000", 11);
check_append("00000101", "10010101", "10010000", 12);
check_append("00000010", "11001010", "11001000", 13);
check_append("00000001", "01100101", "01100100", 14);
check_append("00000000", "10110010", "10110010", 15);

check_append(/*leading_bits= */ "01011001", /*middle_bits=*/"01011001",
/*trailing_bits=*/"11111111", /*offset=*/8, /*preset_buffer_bits=*/true);
check_append("10101100", "10101100", "10000000", 9, true);
check_append("11010110", "01010110", "01000000", 10, true);
check_append("11101011", "00101011", "00100000", 11, true);
check_append("11110101", "10010101", "10010000", 12, true);
check_append("11111010", "11001010", "11001000", 13, true);
check_append("11111101", "01100101", "01100100", 14, true);
check_append("11111110", "10110010", "10110010", 15, true);
}

TEST(TestAppendBitmap, AppendWordOnlyApproriateBytesWritten) {
std::vector<uint8_t> valid_bits = {0x00, 0x00};

uint64_t bitmap = 0x1FF;
{
internal::FirstTimeBitmapWriter writer(valid_bits.data(), /*start_offset=*/1,
/*length=*/(8 * valid_bits.size()) - 1);
writer.AppendWord(bitmap, /*number_of_bits*/ 7);
writer.Finish();
EXPECT_THAT(valid_bits, ElementsAreArray(std::vector<uint8_t>{0xFE, 0x00}));
}
{
internal::FirstTimeBitmapWriter writer(valid_bits.data(), /*start_offset=*/1,
/*length=*/(8 * valid_bits.size()) - 1);
writer.AppendWord(bitmap, /*number_of_bits*/ 8);
writer.Finish();
EXPECT_THAT(valid_bits, ElementsAreArray(std::vector<uint8_t>{0xFE, 0x03}));
}
}

// Tests for GenerateBits and GenerateBitsUnrolled

struct GenerateBitsFunctor {
Expand Down
3 changes: 3 additions & 0 deletions cpp/src/parquet/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ set(PARQUET_SRCS
file_writer.cc
internal_file_decryptor.cc
internal_file_encryptor.cc
level_conversion.cc
metadata.cc
murmur3.cc
"${ARROW_SOURCE_DIR}/src/generated/parquet_constants.cpp"
Expand Down Expand Up @@ -336,6 +337,7 @@ set_source_files_properties(public_api_test.cc
add_parquet_test(reader_test
SOURCES
column_reader_test.cc
level_conversion_test.cc
column_scanner_test.cc
reader_test.cc
stream_reader_test.cc
Expand Down Expand Up @@ -371,6 +373,7 @@ add_parquet_test(schema_test)

add_parquet_benchmark(column_io_benchmark)
add_parquet_benchmark(encoding_benchmark)
add_parquet_benchmark(level_conversion_benchmark)
add_parquet_benchmark(arrow/reader_writer_benchmark PREFIX "parquet-arrow")

if(ARROW_WITH_BROTLI)
Expand Down
Loading