Use PiecewiseSequence indices for FSL take#8800
Conversation
Merging this PR will regress 2 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | decode_varbin[(1000, 4)] |
26.1 µs | 34.1 µs | -23.32% |
| ❌ | Simulation | true_count_vortex_buffer[128] |
522.2 ns | 580.6 ns | -10.05% |
| ⚡ | Simulation | take_fsl_random[1024, 1000] |
15.2 ms | 6 ms | ×2.5 |
| ⚡ | Simulation | take_fsl_nullable_random[1024, 1000] |
14.8 ms | 6 ms | ×2.5 |
| ⚡ | Simulation | take_fsl_random[256, 1000] |
3.5 ms | 1.6 ms | ×2.2 |
| ⚡ | Simulation | take_fsl_nullable_random[256, 1000] |
3.4 ms | 1.6 ms | ×2.1 |
| ⚡ | Simulation | take_fsl_random[1024, 100] |
1,567.8 µs | 799.3 µs | +96.14% |
| ⚡ | Simulation | take_fsl_nullable_random[1024, 100] |
1,535.6 µs | 811.9 µs | +89.12% |
| ⚡ | Simulation | copy_nullable[65536] |
1,027.9 µs | 573.6 µs | +79.2% |
| ⚡ | Simulation | take_fsl_random[256, 100] |
418.1 µs | 239 µs | +74.92% |
| ⚡ | Simulation | take_fsl_random[64, 1000] |
815.6 µs | 466.6 µs | +74.79% |
| ⚡ | Simulation | take_fsl_nullable_random[64, 1000] |
811.9 µs | 477.3 µs | +70.11% |
| ⚡ | Simulation | take_fsl_nullable_random[256, 100] |
415.5 µs | 252.1 µs | +64.82% |
| ⚡ | Simulation | take_fsl_nullable_random[16, 1000] |
257.6 µs | 199.3 µs | +29.28% |
| ⚡ | Simulation | take_fsl_random[16, 1000] |
237.7 µs | 189.4 µs | +25.55% |
| ⚡ | Simulation | take_fsl_random[4096, 1000] |
67.3 ms | 54.2 ms | +24.16% |
| ⚡ | Simulation | take_fsl_random[64, 100] |
123 µs | 99.3 µs | +23.8% |
| ⚡ | Simulation | take_fsl_nullable_random[4096, 1000] |
65.6 ms | 54.3 ms | +20.89% |
| ⚡ | Simulation | take_fsl_nullable_random[64, 100] |
131.5 µs | 111.9 µs | +17.51% |
| ⚡ | Simulation | copy_non_nullable[65536] |
908.7 µs | 794.3 µs | +14.4% |
| ... | ... | ... | ... | ... | ... |
ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing codex/piecewise-sequential-take (d694338) with codex/piecewise-sequential-array (2b6fa3e)
Footnotes
-
52 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
aba7e4d to
3edf6c8
Compare
07fde63 to
3ecd955
Compare
3edf6c8 to
1748e31
Compare
3ecd955 to
98a9180
Compare
01e95cb to
eb83451
Compare
eb83451 to
e79ec4b
Compare
| S: crate::dtype::UnsignedPType, | ||
| L: crate::dtype::UnsignedPType, | ||
| { | ||
| validate_index_ranges(source.len(), starts, lengths, output_len)?; |
There was a problem hiding this comment.
the slice will validate them
| array.buffer::<D>().as_slice(), | ||
| indices_ref.len(), | ||
| )?; | ||
| let validity = array.validity()?.take(indices_ref)?; |
There was a problem hiding this comment.
eh, we need my refactor pr to extract the ref from view
| L: crate::dtype::UnsignedPType, | ||
| T: NativeDecimalType, | ||
| { | ||
| validate_index_ranges(values.len(), starts, lengths, output_len)?; |
There was a problem hiding this comment.
this is already validated by the result slice
There was a problem hiding this comment.
so imho you either keep this and convert the loop below all into unsafe accesses or remove this and use the optional getters on slice
| for (&start, &length) in starts.iter().zip_eq(lengths) { | ||
| let start = start.as_(); | ||
| let length = length.as_(); | ||
| result.extend_from_slice(&values[start..start + length]); |
There was a problem hiding this comment.
I think this would be better if you had the mutable result and then wrote directly into the next available range result[curr..][..length]
|
|
||
| let (new_elements, new_len) = | ||
| take_non_empty_non_degenerate_elements::<I>(array, indices_array, ctx)?; | ||
| let new_validity = if array.dtype().is_nullable() || indices_nullability.is_nullable() { |
There was a problem hiding this comment.
I think validity take already handles this
robert3005
left a comment
There was a problem hiding this comment.
I think we have to audit the validation from the compute and verify that we do not do the work twice. We have to audit exactly but ideally we'd not validate and figure out how to make operations safe by default even given bad input.
6eb0c92 to
16ca5dc
Compare
b615895 to
6dcc52a
Compare
6dcc52a to
a2cbdf0
Compare
Signed-off-by: Daniel King <dan@spiraldb.com>
Signed-off-by: Daniel King <dan@spiraldb.com>
Signed-off-by: Daniel King <dan@spiraldb.com>
Signed-off-by: Daniel King <dan@spiraldb.com>
Signed-off-by: Daniel King <dan@spiraldb.com>
Signed-off-by: Daniel King <dan@spiraldb.com>
Signed-off-by: Daniel King <dan@spiraldb.com>
a2cbdf0 to
d694338
Compare
Summary
Ports the TakeSlices-style optimization to
Take(elements, PiecewiseSequenceArray)instead of introducing a value-wrapper API.1, then callselements.take(piecewise_indices).PiecewiseSequenceArrayindices and gather contiguous ranges only whenmultipliersis a constant unsigned1; non-unit multipliers fall back through normal materialized-index take.start + j * multipliersequences, while the current optimized consumers stay explicitly contiguous.Validation
cargo fmt -p vortex-arraycargo test -p vortex-array piecewise_sequencecargo check -p vortex-arraycargo check -p vortex-array --benchesgit diff --check