Append constant runs and sparse patches without slicing or canonicalizing [builders-child-stack] - #9131
Open
robert3005 wants to merge 5 commits into
Open
Conversation
`Constant::append_to_builder` canonicalized every dtype it had no fast path for. For a list that builds a whole `ListViewArray` only for `append_listview_array` to rebuild it and cast its offsets and sizes back to the builder's types - fixed cost per appended run, paid by every caller that covers a run of rows with one repeated list. A list builder can record the run from the scalar alone. `ListViewBuilder` stores the elements once and points every view at them; `ListBuilder` repeats them, because its offsets can only describe contiguous, in-order lists. Dispatch to both through `match_each_list_builder!` and leave every other builder on the canonical path. Signed-off-by: Robert Kruszewski <robert@spiraldb.com>
…_array Trimming through `rebuild(ListViewRebuildMode::TrimElements)` subtracts the window start from every offset with a compute kernel, only for the rebase in `append_listview_array` to add this builder's elements base straight back on: two passes, one of them through the compute stack, for one addition per offset. Casting the sizes to the builder's type is another kernel for what is a copy. Compute the referenced window here instead. An exact source covers it back to back, so its first and last view bound it, and only some other layout has to be searched for. Slice the elements to that window and rebase in the single pass that was already walking the offsets. The sizes go straight into the builder's `uninit_range` - as a `copy_from_slice` when they already have its type, and a typed conversion loop when they do not. Signed-off-by: Robert Kruszewski <robert@spiraldb.com>
Slicing is not the constant-time operation its doc comment claims: `ArrayRef::slice` ends in `.optimize()`, so every slice pays a full optimizer pass. Slicing a `ListView` is four of them, because slicing the array slices its offsets, its sizes and its elements in turn. A profile of `canonicalize_sparse_list[(512, 7, 4)]` put 42% of the benchmark in `optimizer::try_optimize` under `ArrayRef::slice`. So take each patch's elements instead of slicing runs of patches out of the patch array: `list_elements_at` slices `elements` alone, which is one optimizer pass over a primitive array rather than four over a list view. The same reasoning applies to the fixed-size-list path, which sliced its elements and its validity per run. Gaps keep their bulk append, but reach it without a `ConstantArray`: the fill's elements are materialized once, up front, and every gap points its rows at that one array through `append_array_as_repeated_list`. The fill's elements are now stored once for the whole result rather than once per gap, and a gap costs nothing per row it covers. `canonicalize_sparse_list` medians, against develop: (512, 7, 4) 24.1 us -> 22.5 us (1024, 17, 8) 33.4 us -> 18.7 us (4096, 8, 4) 173.3 us -> 119.6 us (4096, 64, 4) 99.9 us -> 17.3 us (8192, 1024, 4) 180.6 us -> 5.6 us Signed-off-by: Robert Kruszewski <robert@spiraldb.com>
… directly `Constant::append_to_builder` had fast paths for the flat dtypes and fell back to canonicalizing everything else, which builds the whole run as an array only to copy it into the builder. The nested dtypes with a builder can all skip that, and two of them can skip the values entirely: - a constant struct is a constant array per field, so each field's builder takes one as a chunk and the fields stay constant-encoded; - an extension array is its storage wearing a dtype, so a constant one is a constant storage array; - a fixed-size list cannot share one copy of its elements between rows the way a list view can, since its elements sit back to back. A null value still needs only placeholders, and a value whose elements are all the same scalar still tiles to a constant array. Otherwise the tile is copied in per row - one copy of each element, where canonicalizing first made two. That last case is why `ChildBuilder` grows `append_array_values`: appending the same tiny array over and over is the one case where a chunk per append costs more than copying the values, and only the caller can see it. `canonicalize_sparse_fixed_size_list` medians: (512, 7, 4) 79.0 us -> 55.0 us (1024, 17, 8) 139.5 us -> 63.9 us (8192, 1024, 4) 375.9 us -> 142.3 us `Union` is the only dtype left on the fallback, and it has no builder yet. Signed-off-by: Robert Kruszewski <robert@spiraldb.com>
`append_fill` handed the fill scalar to `Constant::append_to_builder` per gap, which materialized the scalar's elements into an array every time. Hoist that out of the loop the way the list path already does: the fill's elements become an array once, up front, and every gap tiles that same array through `FixedSizeListBuilder::append_array_as_repeated_list`. Elements that are all the same scalar stay a constant array, so tiling them over a gap costs nothing at all however many rows it covers. Anything else is copied in per row, because a fixed-size list holds its elements back to back and cannot point its rows at one shared copy. `canonicalize_sparse_fixed_size_list` medians, against develop: (512, 7, 4) 21.1 us -> 25.6 us (was 55.0 us) (1024, 17, 8) 29.1 us -> 32.9 us (was 63.9 us) (8192, 1024, 4) 150.1 us -> 140.1 us (was 142.3 us) What is left of the gap against develop is the per-row copy: each row pays its own append for `list_size` elements, where tiling a run could double the region already written instead. Signed-off-by: Robert Kruszewski <robert@spiraldb.com>
Merging this PR will regress 1 benchmark
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #8966, below #8967.
Sparselist and fixed-size-list canonicalization regressed badly when #9064 replaced its per-row appends with bulk ones. This is the other half of that change: the bulk appends stay, but they stop paying a fixed cost per appended run.Three things were doing that:
Slicing is not constant-time.
ArrayRef::sliceends in.optimize(), so every slice pays a full optimizer pass, and slicing aListViewis four of them - the array's offsets, sizes and elements are each sliced in turn. A profile ofcanonicalize_sparse_list[(512, 7, 4)]put 42% of the benchmark inoptimizer::try_optimizeunderArrayRef::slice. So take each patch's elements withlist_elements_atinstead of slicing runs of patches out of the patch array: one optimizer pass over a primitive array rather than four over a list view.Constant runs were canonicalized before being appended.
Constant::append_to_builderhad fast paths only for the flat dtypes; everything else was built as an array and then copied into the builder. The nested dtypes with a builder can all skip that, and two of them can skip the values entirely - a constant struct is a constant array per field, and an extension array is its storage wearing a dtype.Unionis the only dtype left on the fallback, and it has no builder.The fill was rebuilt per gap. Both sparse paths now materialize the fill's elements once, up front, and every gap points its rows at that one array. For lists the rows share a single copy through their views; a fixed-size list holds its elements back to back so it tiles them, unless they are all the same scalar, in which case the tile stays constant-encoded and costs nothing.
append_listview_arrayalso stops rebuilding: trimming throughListViewRebuildMode::TrimElementssubtracted the window start from every offset with a compute kernel only for the rebase to add the builder's base straight back on, and the sizes cast was another kernel for what is a copy. It now computes the referenced window itself and rebases in the pass that was already walking the offsets.Benchmarks
sparse_canonicalmedians, againstdevelop:canonicalize_sparse_list[(512, 7, 4)]canonicalize_sparse_list[(1024, 17, 8)]canonicalize_sparse_fixed_size_list[(512, 7, 4)]canonicalize_sparse_fixed_size_list[(1024, 17, 8)]canonicalize_sparse_list[(8192, 1024, 4)]*canonicalize_sparse_fixed_size_list[(8192, 1024, 4)]** added locally to cover long gaps; not part of the committed benchmark set.
Lists now beat
develop. Fixed-size lists are within 1.13-1.21x of it, and what is left is the per-row copy of the tile: a fixed-size list cannot point its rows at one shared copy, so a run ofnrows copies the tilentimes. Removing that needs an encoding that expresses a repeated multi-element tile lazily, which does not exist yet -RunEndcannot, since its runs must cover the whole logical length, so a single run is just a constant.Behaviour change worth reviewing
Patches now go into the builder one at a time, and #8964 keeps every appended array as a chunk of its own, so a patch run of length
nleavesnchunks in the elements child where it previously left one.test_sparse_list_chunks_elements_per_run_not_per_patchasserted the old shape and is nowtest_sparse_list_chunks_elements_per_patch_and_once_per_gap. Gaps still cost exactly one chunk however many rows they cover. The trade-off shows up on dense patches:(4096, 2, 4), which patches every other row, is 532 µs against develop's 384 µs.Checks
cargo nextest run --workspace- 7234 passedcargo clippy --all-targets --all-features- cleancargo +nightly fmt --all --check- clean