Materialize list offsets once in list_view_from_list - #9299
Conversation
Merging this PR will regress 6 benchmarks
|
|
This makes me sad. I believe that this is mostly because dispatch completely ruins optimizations that the compiler would otherwise be able to make. |
robert3005
left a comment
There was a problem hiding this comment.
The question worth asking is are you optimising the benchmark or are you optimising some real thing. I don't think it's anything to do with what you're saying. It's that comparatively to a small list the dispatch is the bigger part. Basically I don't think this is an optimisation in practice
|
@robert3005 there is a problem here either way. If this is a real optimization, then this should be fixed. If this is not a real optimization, then we really need to reconsider if these microbenchmarks are useful to us at all. How can we make any decision if we cannot trust our own benchmarks? |
|
Also @robert3005, it seems like the slowdown is actually because we give |
Pull request was converted to draft
fa54891 to
06c6570
Compare
`list_view_from_list` executed the lazy offsets that `reset_offsets` produces inside `build_sizes_from_offsets`, dropped that result, and then sliced the still-lazy handle into the view, so the subtraction kernel ran again when a consumer canonicalized the offsets child. Execute the offsets to a `PrimitiveArray` once, compute sizes from it, and slice it zero-copy for the view. `build_sizes_from_offsets` now takes those offsets and derives the row count from them, so the two can no longer disagree. Extracted from investigating #9255, where the CodSpeed report showed ~35% improvements on the `take_filter_list` benchmarks. This recovers about half of that locally (12.6µs -> 10.1µs median on `take_filter_list_random_mask_sequential_indices[768, 50]`); the remainder is the cost of building and executing the lazy subtraction node itself. Signed-off-by: "Connor Tsui" <connor@spiraldb.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnDprx2HVVYzEU6mNAAhNC
06c6570 to
d97e53e
Compare
Rationale for this change
list_view_from_listexecuted the lazy offsets thatreset_offsetsproduces insidebuild_sizes_from_offsets, dropped that result, and then sliced the still-lazy handle into the view, so the subtraction kernel ran a second time when a consumer canonicalized the offsets child. Extracted from investigating #9255, where the CodSpeed report showed ~35% improvements on thetake_filter_listbenchmarks. This fix alone recovers about half of that locally (12.6µs to 10.1µs median ontake_filter_list_random_mask_sequential_indices[768, 50]); the remainder is the cost of building and executing the lazy subtraction node at all.What changes are included in this PR?
list_view_from_listexecutes the offsets to aPrimitiveArrayonce at the top, computessizesfrom that array, and slices it zero-copy for the view's offsets.build_sizes_from_offsetsnow takes the materialized offsets instead of executing its own copy. Also adds a regression test that list-to-listview conversion resets nonzero leading offsets.