Skip to content

Commit 2c8c7ee

Browse files
connortsui20connortsui20
authored
perf(array): reduce RowFn UTF-8 decode overhead (#9985)
Stacked on #9987, which adds the CI benchmark without changing production code. ## Summary Calls `VarBinViewData::validate_and_fix` directly during RowFn UTF-8 decoding, avoiding an intermediate `VarBinViewArray`. Validation and null-payload sanitation still run on every decode, so unchecked string access keeps the same safety contract. The revised, guide-compliant CI cases do not establish a consistent throughput improvement at 8,192 rows. NEON is close, while AVX2 and AVX-512 show mixed movements. The old tiny-case gains are not sufficient CI evidence for merging this change. ## Changes Adds repeated-decode sanitation tests for empty and sliced arrays. The production patch is unchanged and previously passed 78 UTF-8 and VarBinView tests and all-target, all-feature `vortex-array` Clippy. Both layers ran all revised cases locally and in native CI. Every reported sample stayed below 1 ms. [CI comparison and raw results](https://github.com/vortex-data/vortex/blob/ct/row-fn-performance-evidence/research/row-fn-engine/performance/ci-utf8-guide.md) isolate this layer with identical benchmarks. The overall CodSpeed performance check flags other benchmark cases whose movement remains unattributed. Keep this PR as a draft pending performance evidence sufficient to justify the change. Signed-off-by: Connor Tsui <[email protected]>
1 parent d196f60 commit 2c8c7ee

1 file changed

Lines changed: 41 additions & 12 deletions

File tree

  • vortex-array/src/scalar_fn/unstable/row/types/element

‎vortex-array/src/scalar_fn/unstable/row/types/element/utf8.rs‎

Lines changed: 41 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ use crate::arrays::Constant;
2323
use crate::arrays::VarBinViewArray;
2424
use crate::arrays::varbinview::BinaryView;
2525
use crate::arrays::varbinview::VarBinViewArrayExt as _;
26+
use crate::arrays::varbinview::VarBinViewData;
2627
use crate::buffer::BufferHandle;
2728
use crate::dtype::DType;
2829
use crate::scalar::ScalarValue;
@@ -136,10 +137,9 @@ impl AsRef<str> for Utf8View<'_> {
136137

137138
/// Decodes `array` into views and data buffers whose every view addresses valid UTF-8.
138139
///
139-
/// The second [`VarBinViewArray::try_new`] is the sanitization step, not a round trip: it
140-
/// validates each valid view and replaces every null row's view with an empty one. The
141-
/// [`InputElement`] implementation relies on that, so a dense callback can read a null row's
142-
/// payload and [`Utf8View::as_str`] can skip the UTF-8 check. Removing it makes those unsafe.
140+
/// Validation also replaces each null view with an empty one. Canonical arrays built from
141+
/// buffer handles do not carry host-content validation evidence, so this step remains necessary
142+
/// before unchecked string access, even when an earlier decode validated the same input.
143143
fn decode_utf8(array: ArrayRef, ctx: &mut ExecutionCtx) -> VortexResult<Utf8Values> {
144144
let array = array.execute::<VarBinViewArray>(ctx)?;
145145
let views = Buffer::<BinaryView>::from_byte_buffer(array.views_handle().try_to_host_sync()?);
@@ -152,14 +152,7 @@ fn decode_utf8(array: ArrayRef, ctx: &mut ExecutionCtx) -> VortexResult<Utf8Valu
152152
);
153153

154154
let validity = array.varbinview_validity();
155-
let array = VarBinViewArray::try_new(
156-
views,
157-
Arc::clone(&buffers),
158-
array.dtype().clone(),
159-
validity,
160-
ctx,
161-
)?;
162-
let views = Buffer::<BinaryView>::from_byte_buffer(array.views_handle().try_to_host_sync()?);
155+
let views = VarBinViewData::validate_and_fix(views, &buffers, array.dtype(), &validity, ctx)?;
163156

164157
Ok(Utf8Values { views, buffers })
165158
}
@@ -330,6 +323,42 @@ mod tests {
330323
Ok(())
331324
}
332325

326+
#[rstest]
327+
#[case::empty(0)]
328+
#[case::one(1)]
329+
#[case::word_remainder(65)]
330+
fn input_sanitizes_sliced_null_views_repeatedly(#[case] len: usize) -> VortexResult<()> {
331+
let views: Buffer<BinaryView> = (0..len + 2)
332+
.map(|index| {
333+
if index % 2 == 0 {
334+
BinaryView::from(u128::MAX)
335+
} else {
336+
BinaryView::make_view(b"valid", 0, 0)
337+
}
338+
})
339+
.collect();
340+
let validity = BoolArray::from_iter((0..len + 2).map(|index| index % 2 != 0)).into_array();
341+
let array = VarBinViewArray::new_handle(
342+
BufferHandle::new_host(views.into_byte_buffer()),
343+
Default::default(),
344+
DType::Utf8(Nullability::Nullable),
345+
Validity::Array(validity),
346+
)
347+
.into_array()
348+
.slice(1..len + 1)?;
349+
let mut ctx = VortexSession::empty().create_execution_ctx();
350+
for _ in 0..2 {
351+
let column = Utf8Column::decode(array.clone(), &mut ctx)?;
352+
for index in 0..len {
353+
assert_eq!(
354+
Utf8Column::get(&column, index).as_str(),
355+
if index % 2 == 0 { "valid" } else { "" }
356+
);
357+
}
358+
}
359+
Ok(())
360+
}
361+
333362
#[test]
334363
fn input_rejects_unvalidated_utf8() {
335364
let invalid_view = BinaryView::make_view(&[0xff], 0, 0);

0 commit comments

Comments
 (0)