perf(arrow-cast): gate Dictionary -> View fast path on cardinality - #10436
perf(arrow-cast): gate Dictionary -> View fast path on cardinality#10436Abhisheklearn12 wants to merge 12 commits into
Conversation
|
hi @Jefffrey, I’d love to get your feedback whenever you have time, appreciate it! |
|
I'll try take a look at this soon |
could we pull out the bug fix changes into a separate PR from the performance improvements |
|
also i re-ran after the split, since removing the checks touched existing fast paths on main
new arms, dictionary larger than the array
dense shapes on the new arms: -0.4% to +2.5%, the gate routes them to both impls compiled into one binary and timed in alternating rounds, outputs asserted equal per cell before timing. i7-11700F. one caveat: my harness's |
…ew (#10510) # Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> Split out of #10436 at review request, which is the PR linked to #8985. # Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> `view_from_dict_values` appends a view for every non-null key without checking whether the value it points at is null, so a null dictionary value surfaces as the empty slice its offsets span: ```rust // values ["aa", NULL, "cc"], keys [0, 1, 2] cast(&dict, &DataType::Utf8View) // before: ["aa", "", "cc"] // after: ["aa", null, "cc"] cast(&dict, &DataType::Utf8) // unchanged: ["aa", null, "cc"] ``` unpack_dictionary goes through take, and impl From<&GenericByteArray> for GenericByteViewArray branches on is_null, so both already produce null here. Only the dictionary to view path disagreed. # What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> Check value validity before appending a view. Also bounds-check the key before indexing the offsets, turning an out of range key from undefined behaviour into an InvalidArgumentError. # Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? If this PR claims a performance improvement, please include evidence such as benchmark results. --> Yes. The new test fails on the unfixed code, giving `Some("")` where `None` is expected. # Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. --> `Dictionary<_, Utf8>` -> `Utf8View `and `Dictionary<_, Binary> -> BinaryView` now produce null for a null dictionary value instead of an empty string. --------- Co-authored-by: Jeffrey Vo <jeffrey.vo.australia@gmail.com>
|
what benchmarks am i looking at here? cast kernels seems to only have a single related benchmark, for can we add these benchmarks in a separate PR so we can verify with the bot (ideally keeping the minimum required, e.g. don't need both largestring -> utf8view and largebinary -> binaryview since theyre essentially the same code paths) |
well, those numbers came from a local harness rather than anything checked in, which is why they aren't in the repo. i'll add the benchmarks in a separate pr so the bot can verify them. agreed on keeping the set minimal.
the existing |
|
large variant benchmark probably not needed since theres only one extra check thats not part of the hot loop, so to keep things simple we just need the |
|
Hi @Jefffrey , I pushed the bench |
# Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> None. Split out of #10436 at review request, so these land on main first and the bot can measure that PR against them. # Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> `cast_kernels` has one dictionary to view benchmark, at 10,000 rows over 3 values. Nothing covers the opposite shape, a dictionary much larger than the array, and nothing covers `dict<binary> -> utf8view`, which validates the values as UTF-8. # What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> Two benchmarks, both 1024 rows over 32,768 values: - `cast dict to string view (sparse)`. For #10436 this is a no regression check rather than an expected win, since main already takes the direct path at this shape - `cast binary dict to string view (sparse)`. No dense counterpart, since that shape runs the same code either way Keys come from `seedable_rng` so they spread across the dictionary, and values exceed 12 bytes so the views reference the buffer rather than inlining. # Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? If this PR claims a performance improvement, please include evidence such as benchmark results. --> Benchmarks only, no library code touched. They build and run under `cargo bench -p arrow --features test_utils --bench cast_kernels`. # Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. --> No.
|
run benchmark cast_kernels |
This comment was marked as duplicate.
This comment was marked as duplicate.
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing fix/dict-view-cardinality-gate (93a5cb6) to bb82f3e (merge-base) diff Run configurationrun benchmark cast_kernels
env:
BENCH_FILTER: "dict.*to.*view"CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
run benchmark cast_kernels |
This comment was marked as duplicate.
This comment was marked as duplicate.
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing fix/dict-view-cardinality-gate (93a5cb6) to bb82f3e (merge-base) diff Run configurationrun benchmark cast_kernels
env:
BENCH_FILTER: "dict.*to.*view"CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
hi @Jefffrey, would appreciate your any new feedbacks on this :) |
| // There are two ways to produce a view array from a dictionary, and neither of them | ||
| // copies the values buffer: | ||
| // | ||
| // (a) `unpack_dictionary` builds one view per *dictionary value* and then uses `take`, | ||
| // which gathers 16-byte views and passes the data buffers through untouched. | ||
| // (b) `view_from_dict_values` builds one view per *row* directly against the values | ||
| // buffer, skipping the intermediate array entirely. | ||
| // | ||
| // (a) is memory-bandwidth-bound -- the gather in `take` benchmarks at or below the cost | ||
| // of a hand-written `u128` gather -- so it wins once rows reach roughly 0.6x the | ||
| // dictionary size. (b) reads the payload bytes of every row out of the values buffer to | ||
| // build each view, which is far more expensive per row, and only pays off when the | ||
| // dictionary is substantially larger than the array is long: there (a) spends most of | ||
| // its time building views that no row ever references. | ||
| // | ||
| // So take (b) only when it actually wins. Everything else falls through to (a) below. |
There was a problem hiding this comment.
| // There are two ways to produce a view array from a dictionary, and neither of them | |
| // copies the values buffer: | |
| // | |
| // (a) `unpack_dictionary` builds one view per *dictionary value* and then uses `take`, | |
| // which gathers 16-byte views and passes the data buffers through untouched. | |
| // (b) `view_from_dict_values` builds one view per *row* directly against the values | |
| // buffer, skipping the intermediate array entirely. | |
| // | |
| // (a) is memory-bandwidth-bound -- the gather in `take` benchmarks at or below the cost | |
| // of a hand-written `u128` gather -- so it wins once rows reach roughly 0.6x the | |
| // dictionary size. (b) reads the payload bytes of every row out of the values buffer to | |
| // build each view, which is far more expensive per row, and only pays off when the | |
| // dictionary is substantially larger than the array is long: there (a) spends most of | |
| // its time building views that no row ever references. | |
| // | |
| // So take (b) only when it actually wins. Everything else falls through to (a) below. | |
| // `unpack_dictionary` operates per dictionary value before using take kernel to form | |
| // final view. `view_from_dict_values` builds output view directly per row (key index). | |
| // Based on benchmarking, `view_from_dict_values` is more efficient for sparse dictionaries | |
| // (more dictionary values than there are rows/keys), whilst `unpack_dictionary` is | |
| // more efficient for dense dictionaries, where a sparse dictionary is when rows reach | |
| // roughly 0.6x the dictionary size. | |
| // | |
| // Therefore delegate to the faster method based on the density of the input dictionary. |
My attempt at cutting down the verbosity a bit, feel free to edit or make suggestions if it doesn't seem as clear or is inaccurate at bits
| /// Whether building views per row beats `unpack_dictionary` for this array. | ||
| /// | ||
| /// `unpack_dictionary` costs `O(values)` to build the intermediate view array plus `O(keys)` for | ||
| /// a bandwidth-bound gather; building views directly costs `O(keys)` but with a much larger | ||
| /// constant, since every row has to read its payload out of the values buffer. The direct path | ||
| /// therefore only wins when the dictionary is substantially larger than the array is long. | ||
| /// | ||
| /// The measured crossover sits near `keys ~= 0.6 * values`, so this deliberately switches short | ||
| /// of it rather than at the crossover itself: at `keys * 2 == values` the direct path is still | ||
| /// ahead by 15-30%, which leaves margin for the exact crossover moving with cache size or | ||
| /// microarchitecture. Being wrong in this direction merely forgoes a win; being wrong the other | ||
| /// way is a large regression on the common case. | ||
| #[inline] | ||
| fn prefer_direct_views<K: ArrowDictionaryKeyType>(array: &DictionaryArray<K>) -> bool { |
There was a problem hiding this comment.
| /// Whether building views per row beats `unpack_dictionary` for this array. | |
| /// | |
| /// `unpack_dictionary` costs `O(values)` to build the intermediate view array plus `O(keys)` for | |
| /// a bandwidth-bound gather; building views directly costs `O(keys)` but with a much larger | |
| /// constant, since every row has to read its payload out of the values buffer. The direct path | |
| /// therefore only wins when the dictionary is substantially larger than the array is long. | |
| /// | |
| /// The measured crossover sits near `keys ~= 0.6 * values`, so this deliberately switches short | |
| /// of it rather than at the crossover itself: at `keys * 2 == values` the direct path is still | |
| /// ahead by 15-30%, which leaves margin for the exact crossover moving with cache size or | |
| /// microarchitecture. Being wrong in this direction merely forgoes a win; being wrong the other | |
| /// way is a large regression on the common case. | |
| #[inline] | |
| fn prefer_direct_views<K: ArrowDictionaryKeyType>(array: &DictionaryArray<K>) -> bool { | |
| #[inline] | |
| fn is_sparse<K: ArrowDictionaryKeyType>(array: &DictionaryArray<K>) -> bool { |
I feel the doc comment is too verbose and repeats what is said above; maybe we can move this method as an inner method to dictionary_cast to try colocate the comments to avoid redundancy
also a name like is_sparse to me reads better than prefer_direct_views because we dont exactly know what direct_views means here
| /// and still carry an oversized buffer -- and it is the buffer that `append_block` rejects. | ||
| #[inline] | ||
| fn values_buffer_fits_in_view<T: ByteArrayType>(values: &GenericByteArray<T>) -> bool { | ||
| values.values().len() < u32::MAX as usize |
There was a problem hiding this comment.
| values.values().len() < u32::MAX as usize | |
| values.values().len() < i32::MAX as usize |
| // LargeUtf8/LargeBinary additionally require a values buffer small enough to be addressed | ||
| // by the u32 offset of a view. |
There was a problem hiding this comment.
| // LargeUtf8/LargeBinary additionally require a values buffer small enough to be addressed | |
| // by the u32 offset of a view. | |
| // `view_from_dict_values` directly appends the values buffer as a block using | |
| // `GenericByteViewBuilder::append_block` which asserts length of the buffer; we must | |
| // ensure this assertion holds to use it for large variants which may exceed the max allowable length. | |
| // If we exceed the length we can simply fallback to `unpack_dictionary` which still builds it | |
| // correctly. |
| /// This deliberately measures the whole buffer rather than the largest live offset: slicing a | ||
| /// byte array slices its offsets but keeps `value_data` intact, so a slice can have small offsets | ||
| /// and still carry an oversized buffer -- and it is the buffer that `append_block` rejects. | ||
| #[inline] |
There was a problem hiding this comment.
| #[inline] |
same here, dont need to repeat here, easier to just keep it within the actual match arms above per my previous comment
| } | ||
| Err(e) => { | ||
| if !cast_options.safe { | ||
| return Err(e); |
There was a problem hiding this comment.
do we need to consider if potentially invalid utf8 is actually masked out by the null buffer of the byte array? this could be overly restrictive for such niche inputs
There was a problem hiding this comment.
I had a look, this isn't new behaviour from the pr. binary_dict_to_string_view calls try_from_binary on the values, same as cast_binary_to_string does, and that validates the whole buffer in one pass without looking at the null buffer, so dict -> utf8view and dict -> utf8 fail on the same input, and even unused invalid values fail too.
imo, i'd leave it as is here. relaxing only utf8view would make the two behave differently, and doing it properly means changing validation for both, which would cost the performance this pr is after. happy to look at that separately if you think it's worth it
| fn keys_taking_direct_path() -> Int32Array { | ||
| // 2 keys < 6/2 values -> views are built directly per row | ||
| Int32Array::from_iter([Some(0), Some(3)]) | ||
| } |
There was a problem hiding this comment.
it might be better to move this test code into cast/dictionary.rs file, so it lives closer together
There was a problem hiding this comment.
also i forgot to say, i left the older dict to view tests (test_dict_to_view, test_string_dicts_to_binary_view, test_binary_dicts_to_string_view) in mod.rs since they're already on main, happy to move them in a follow up if you'd rather they all sit together :)
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
the test code seems overly verbose/complex, not to mention has a lot a redundancies. can we ensure we also take care of our test code, since that also is something that would need to be maintained
| } | ||
|
|
||
| #[test] | ||
| fn test_dict_to_view_both_paths_agree() { |
There was a problem hiding this comment.
this test is really confusing; its named "both_paths_agree" but we're not testing them with the same input data. so we're not testing them against each as the name suggests, just in parallel? can we clarify what we're testing here
There was a problem hiding this comment.
yeah, you're right. the name was off since each path had its own hardcoded expectation.
both are now checked against take(values, keys) then cast, so they'd fail if they disagree. renamed it to test_dict_to_view_matches_take_then_cast, the keys can't be the same since their ratio to values determines the path, so i only varied the key count
| let large_utf8: ArrayRef = Arc::new(LargeStringArray::from(vec![ | ||
| "aa", "bb", long, "dd", "ee", "ff", | ||
| ])); | ||
| let binary: ArrayRef = Arc::new(BinaryArray::from_iter_values([ | ||
| b"aa".as_slice(), | ||
| b"bb", | ||
| long.as_bytes(), | ||
| b"dd", | ||
| b"ee", | ||
| b"ff", | ||
| ])); | ||
| let large_binary: ArrayRef = Arc::new(LargeBinaryArray::from_iter_values([ | ||
| b"aa".as_slice(), | ||
| b"bb", | ||
| long.as_bytes(), | ||
| b"dd", | ||
| b"ee", | ||
| b"ff", |
There was a problem hiding this comment.
we could simplify this setup by casting the input utf8 to the required binary/largebinary etc. type
| } | ||
|
|
||
| // every source type that can reach BinaryView | ||
| for (label, values, to_type) in [ | ||
| ("Utf8->BinaryView", utf8, DataType::BinaryView), | ||
| ("LargeUtf8->BinaryView", large_utf8, DataType::BinaryView), | ||
| ("Binary->BinaryView", binary, DataType::BinaryView), |
There was a problem hiding this comment.
i feel we can unify these loops together instead of repeating them; consider doing equality on the output arrays instead of collecting the values first
| let casted = cast_with_options(&dict, &DataType::Utf8View, &safe).unwrap(); | ||
| let got: Vec<_> = casted.as_string_view().iter().collect(); | ||
| // only rows whose key points at the invalid value are nullified | ||
| assert!(got.iter().all(|v| *v != Some("\u{FFFD}"))); |
There was a problem hiding this comment.
this is confusing; we're not checking the nullability we're checking the exact value? so the comment is misleading
| let mut b32 = BinaryBuilder::new(); | ||
| let mut b64 = GenericBinaryBuilder::<i64>::new(); | ||
| for v in [b"aa".as_slice(), b"bb", &[0xFF, 0xFE], b"dd", b"ee", b"ff"] { | ||
| b32.append_value(v); | ||
| b64.append_value(v); | ||
| } | ||
| let binary: ArrayRef = Arc::new(b32.finish()); | ||
| let large_binary: ArrayRef = Arc::new(b64.finish()); |
There was a problem hiding this comment.
feels like we can greatly simplify this by using from_vec for both types
| } | ||
|
|
||
| #[test] | ||
| fn test_dict_large_utf8_to_utf8view() { |
There was a problem hiding this comment.
is this not tested as part of test_dict_to_view_both_paths_agree() above? same for the tests below
| } | ||
|
|
||
| #[test] | ||
| fn test_dict_binary_to_utf8view_invalid_utf8_safe() { |
There was a problem hiding this comment.
how is this different from test_dict_binary_to_utf8view_invalid_utf8_both_paths
There was a problem hiding this comment.
it wasn't, merged into the one test
3 participants
Which issue does this PR close?
Supersedes #9768, which I closed it implemented these fast paths ungated, and benchmarking showed that made things significantly slower. Details below.
Rationale for this change
dictionary_castcan build a view array from a dictionary two ways:unpack_dictionarybuilds one view per dictionary value, then gathers withtakeview_from_dict_valuesbuilds one view per row directly against the values bufferThe comment above (b) says (a) "incurs unnecessary data copy of the value buffer". That
isn't the case.
impl From<&GenericByteArray> for GenericByteViewArrayreuses the buffer(it only falls back to copying when the values exceed the
u32offset a view can hold),and
take_byte_viewpassesdata_buffers().to_vec()through untouched. Neither stepcopies value data, so there was no copy to remove.
What the two actually trade is a bandwidth-bound
u128gather intakeagainst aper-row
make_viewthat reads each row's payload out of the values buffer. The gatherwins once rows reach roughly 0.6x the dictionary size so (b) is a large pessimisation in
the common case, and only pays off when the dictionary is substantially larger than the
array is long, where (a) spends most of its time building views no row references.
This means the existing
Utf8 -> Utf8ViewandBinary -> BinaryViewfast paths on mainare currently 5-8x slower than
unpack_dictionaryat typical batch shapes.What changes are included in this PR?
keys.len() < values.len() / 2; everything else falls throughto
unpack_dictionary. The measured crossover is nearrows ≈ 0.6 * values; the gateswitches short of it, at
rows = values / 2, where the direct path is still ahead by15-30%. That margin covers the crossover moving with cache size or microarchitecture.
Being wrong in that direction only forgoes a win; being wrong the other way is a
regression on the common case.
LargeUtf8/LargeBinary->Utf8View/BinaryView, and theUtf8<->Binarycrosscasts with the offset-fit check and UTF-8 validation the issue calls out.
view_from_dict_valuesdropping null dictionary values: they became empty stringsrather than nulls, where
unpack_dictionaryandimpl From<&GenericByteArray>bothproduce nulls.
from undefined behaviour into an error. The gate confines this loop to short row counts,
so it is within noise when the dictionary holds no nulls. When the dictionary does hold
nulls, the null check costs 25-40% on the direct path an unavoidable random bitmap
lookup per row, and the price of emitting nulls rather than the empty strings the
previous code produced.
Benchmarks
Existing fast paths on main:
Utf8->Utf8ViewUtf8->Utf8ViewBinary->BinaryViewBinary->BinaryViewNew arms, sparse shapes (dictionary larger than the array):
LargeUtf8->Utf8ViewLargeUtf8->BinaryViewLargeBinary->BinaryViewUtf8->BinaryViewBinary->Utf8ViewLargeBinary->Utf8View(
Binary/LargeBinary -> Utf8Viewgain less because UTF-8 validation of the dictionaryvalues dominates.)
Dense shapes on the new arms are unchanged, -2.9% to +1.2% the gate routes them to
unpack_dictionary. Worst cell across the whole matrix is +5.0%, on a shape where bothversions take the direct path and the delta is the null and bounds checks above.
Method
Both implementations were compiled into a single binary and timed in alternating rounds so
thermal drift cancels out of the ratio. Cells whose code is identical in both versions come
out at -2.9% to +1.2%, which bounds the method's noise; a two-run criterion comparison of
those same cells reported up to +47% drift, which is why it wasn't used. Outputs are
asserted equal per cell before timing. Measured on an i7-11700F; a second independent run
reproduced every headline result within 3.2 points and produced the identical set of cells
above 50%; the largest shift on any cell was 8.3 points, on a cell that is flat in both runs.
Are these changes tested?
Yes. Every arm is exercised through both implementations (row counts either side of the
gate), with results asserted equal to the
unpack_dictionaryreference in each case. Nulldictionary values, null keys, and invalid UTF-8 under both
safeand strictCastOptionsare covered by dedicated tests.
Are there any user-facing changes?
Casting
Dictionary<_, Utf8> -> Utf8ViewandDictionary<_, Binary> -> BinaryViewbecomessubstantially faster at typical batch shapes 5-8x on the shapes benchmarked above.
Dictionary<_, LargeUtf8/LargeBinary> -> Utf8View/BinaryViewand theUtf8<->Binarycross casts gain a fast path when the dictionary holds more than twice as many values as the
array has rows. Outside that they behave as before, going through
unpack_dictionary.Two behaviour changes, called out explicitly:
InvalidArgumentErrorinstead of beingundefined behaviour. Only reachable for a dictionary built without validation.
Happy to split either into its own PR if you'd prefer this one stay purely about the fast path.