Skip to content

Materialize list offsets once in list_view_from_list - #9299

Merged
connortsui20 merged 1 commit into
developfrom
claude/list-offsets-optimization-9btwil
Aug 10, 2026
Merged

Materialize list offsets once in list_view_from_list#9299
connortsui20 merged 1 commit into
developfrom
claude/list-offsets-optimization-9btwil

Conversation

@connortsui20

@connortsui20 connortsui20 commented Aug 9, 2026

Copy link
Copy Markdown
Member

Rationale for this change

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 a second time when a consumer canonicalized the offsets child. Extracted from investigating #9255, where the CodSpeed report showed ~35% improvements on the take_filter_list benchmarks. This fix alone recovers about half of that locally (12.6µs to 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 at all.

What changes are included in this PR?

list_view_from_list executes the offsets to a PrimitiveArray once at the top, computes sizes from that array, and slices it zero-copy for the view's offsets. build_sizes_from_offsets now takes the materialized offsets instead of executing its own copy. Also adds a regression test that list-to-listview conversion resets nonzero leading offsets.

@codspeed-hq

codspeed-hq Bot commented Aug 9, 2026

Copy link
Copy Markdown

Merging this PR will regress 6 benchmarks

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 18 improved benchmarks
❌ 6 regressed benchmarks
✅ 1920 untouched benchmarks
⏩ 85 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation cold_misaligned[(64, 256)] 4.4 ms 5.2 ms -16.31%
Simulation take[small_m/shuffled/primitive/nonnull/chunks=1024/indices=1000] 5.6 ms 6.4 ms -13.03%
Simulation take[small_m/shuffled/primitive/nonnull/chunks=1024/indices=256] 2.1 ms 2.4 ms -12.69%
Simulation take[small_m/shuffled/primitive/nonnull/chunks=16384/indices=1000] 9.6 ms 10.8 ms -11.84%
Simulation take[core/shuffled/primitive/nonnull/chunks=1024/indices=10000] 13.6 ms 15.3 ms -10.97%
Simulation compress_fsst[(1000, 64, 8)] 1 ms 1.1 ms -10.39%
Simulation multipolygon_random_nulls 942.4 µs 816 µs +15.49%
Simulation take_filter_list_slice_mask_random_indices[768, 50] 240.9 µs 211.2 µs +14.08%
Simulation take_filter_list_random_mask_sequential_indices[768, 50] 236.8 µs 208.7 µs +13.46%
Simulation take_filter_list_slice_mask_random_indices[256, 50] 239.1 µs 210.9 µs +13.39%
Simulation take_filter_list_random_mask_sequential_indices[256, 50] 241.8 µs 213.3 µs +13.34%
Simulation multipolygon_non_nullable 1.2 ms 1.1 ms +12.77%
Simulation take_filter_list_slice_mask_sequential_indices[256, 50] 237.4 µs 210.6 µs +12.7%
Simulation multipolygon_mixed_validity 1.2 ms 1.1 ms +12.36%
Simulation take_filter_list_small_uncached_random_mask_random_indices[256, 10] 233.7 µs 208.1 µs +12.35%
Simulation take_filter_list_small_uncached_random_mask_random_indices[768, 10] 242.2 µs 215.8 µs +12.25%
Simulation take_filter_list_small_random_mask_random_indices[256, 10] 232.8 µs 207.6 µs +12.15%
Simulation take_filter_list_random_mask_random_indices[768, 50] 256.1 µs 228.3 µs +12.14%
Simulation take_filter_list_slice_mask_sequential_indices[768, 50] 236.8 µs 211.2 µs +12.11%
Simulation take_filter_list_random_mask_random_indices[256, 50] 245.9 µs 220.3 µs +11.61%
... ... ... ... ... ...

ℹ️ 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 claude/list-offsets-optimization-9btwil (d97e53e) with develop (66d096b)

Open in CodSpeed

Footnotes

  1. 85 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.

@connortsui20 connortsui20 added the changelog/performance A performance improvement label Aug 9, 2026
@connortsui20

connortsui20 commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

This makes me sad.

I believe that this is mostly because dispatch completely ruins optimizations that the compiler would otherwise be able to make.

@connortsui20
connortsui20 enabled auto-merge (squash) August 9, 2026 18:29

@robert3005 robert3005 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@connortsui20

Copy link
Copy Markdown
Member Author

@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?

@connortsui20

Copy link
Copy Markdown
Member Author

Also @robert3005, it seems like the slowdown is actually because we give adjusted_offsets as a lazy array to ListArray::new_unchecked, and then when it becomes a list view later it does a bunch of extra work it didnt need to.

@connortsui20
connortsui20 marked this pull request as draft August 9, 2026 20:02
auto-merge was automatically disabled August 9, 2026 20:02

Pull request was converted to draft

@connortsui20
connortsui20 force-pushed the claude/list-offsets-optimization-9btwil branch from fa54891 to 06c6570 Compare August 9, 2026 20:15
@connortsui20 connortsui20 changed the title Reset list offsets with a direct subtraction Materialize list offsets once in list_view_from_list Aug 9, 2026
@connortsui20
connortsui20 requested a review from robert3005 August 9, 2026 20:25
`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
@connortsui20
connortsui20 force-pushed the claude/list-offsets-optimization-9btwil branch from 06c6570 to d97e53e Compare August 9, 2026 20:36
@connortsui20
connortsui20 marked this pull request as ready for review August 9, 2026 20:42
@connortsui20
connortsui20 enabled auto-merge (squash) August 9, 2026 20:43
@connortsui20
connortsui20 merged commit a99dd87 into develop Aug 10, 2026
76 of 77 checks passed
@connortsui20
connortsui20 deleted the claude/list-offsets-optimization-9btwil branch August 10, 2026 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/performance A performance improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants