Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Help LLVM vectorize comparison kernel ~50-80% faster #2646

Merged
merged 3 commits into from
Sep 4, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
68 changes: 37 additions & 31 deletions arrow/src/buffer/mutable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,41 @@ impl MutableBuffer {
assert!(len <= self.capacity());
self.len = len;
}

/// Invokes `f` with values `0..len` collecting the boolean results into a new `MutableBuffer`
///
/// This is similar to `from_trusted_len_iter_bool`, however, can be significantly faster
/// as it eliminates the conditional `Iterator::next`
#[inline]
pub(crate) fn collect_bool<F: FnMut(usize) -> bool>(len: usize, mut f: F) -> Self {
let mut buffer = Self::new(bit_util::ceil(len, 8));

let chunks = len / 8;
let remainder = len % 8;
for chunk in 0..chunks {
let mut packed = 0;
for bit_idx in 0..8 {
let i = bit_idx + chunk * 8;
packed |= (f(i) as u8) << bit_idx;
}

// SAFETY: Already allocated sufficient capacity
unsafe { buffer.push_unchecked(packed) }
}

if remainder != 0 {
let mut packed = 0;
for bit_idx in 0..remainder {
let i = bit_idx + chunks * 8;
packed |= (f(i) as u8) << bit_idx;
}

// SAFETY: Already allocated sufficient capacity
unsafe { buffer.push_unchecked(packed) }
}

buffer
}
}

/// # Safety
Expand Down Expand Up @@ -496,38 +531,9 @@ impl MutableBuffer {
mut iterator: I,
) -> Self {
let (_, upper) = iterator.size_hint();
let upper = upper.expect("from_trusted_len_iter requires an upper limit");

let mut result = {
let byte_capacity: usize = upper.saturating_add(7) / 8;
MutableBuffer::new(byte_capacity)
};
let len = upper.expect("from_trusted_len_iter requires an upper limit");

'a: loop {
let mut byte_accum: u8 = 0;
let mut mask: u8 = 1;

//collect (up to) 8 bits into a byte
while mask != 0 {
if let Some(value) = iterator.next() {
byte_accum |= match value {
true => mask,
false => 0,
};
mask <<= 1;
} else {
if mask != 1 {
// Add last byte
result.push_unchecked(byte_accum);
}
break 'a;
}
}

// Soundness: from_trusted_len
result.push_unchecked(byte_accum);
}
result
Self::collect_bool(len, |_| iterator.next().unwrap())
Copy link
Contributor

Choose a reason for hiding this comment

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

@tustvold one last thought, I wonder if using unwrap_unchecked here would result in the same performance (as it removes the condition).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gave this a go and honestly I don't really understand the results. The scalar comparison is on par with this PR, but the non-scalar variants are worse by 50% compared to master... My 2 cents is that not using iterators for performance critical code makes for code that is easier to reason about, both by humans and LLVM, and so even if we can somehow finagle from_trusted_len_iter_bool to perform the same, I'm more comfortable with a simple loop 😅

}

/// Creates a [`MutableBuffer`] from an [`Iterator`] with a trusted (upper) length or errors
Expand Down
19 changes: 8 additions & 11 deletions arrow/src/compute/kernels/comparison.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,10 @@ where
let null_bit_buffer =
combine_option_bitmap(&[left.data_ref(), right.data_ref()], left.len())?;

// Safety:
// `i < $left.len()` and $left.len() == $right.len()
let comparison = (0..left.len())
.map(|i| unsafe { op(left.value_unchecked(i), right.value_unchecked(i)) });
// same size as $left.len() and $right.len()
let buffer = unsafe { MutableBuffer::from_trusted_len_iter_bool(comparison) };
let buffer = MutableBuffer::collect_bool(left.len(), |i| unsafe {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add the safety comments?

// SAFETY: i in range 0..len
op(left.value_unchecked(i), right.value_unchecked(i))
});

let data = unsafe {
ArrayData::new_unchecked(
Expand All @@ -91,11 +89,10 @@ where
.null_buffer()
.map(|b| b.bit_slice(left.offset(), left.len()));

// Safety:
// `i < $left.len()`
let comparison = (0..left.len()).map(|i| unsafe { op(left.value_unchecked(i)) });
// same as $left.len()
let buffer = unsafe { MutableBuffer::from_trusted_len_iter_bool(comparison) };
let buffer = MutableBuffer::collect_bool(left.len(), |i| unsafe {
// SAFETY: i in range 0..len
op(left.value_unchecked(i))
});

let data = unsafe {
ArrayData::new_unchecked(
Expand Down