Skip to content

cache DTypes parsed from flatbuffers - #9765

Merged
myrrc merged 1 commit into
developfrom
myrrc/cache-parsed-dtype-fields
Sep 4, 2026
Merged

cache DTypes parsed from flatbuffers#9765
myrrc merged 1 commit into
developfrom
myrrc/cache-parsed-dtype-fields

Conversation

@myrrc

@myrrc myrrc commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

In clickbench queries, duckdb_reader_open accounts for 13% CPU time of the scan.
This is because for every file's field for every file Vortex parses DType from
a flatbuffer.

This change adds a cache of DTypes parsed from flatbuffers so among multiple
query iterations parsing is done exactly once per field

@myrrc
myrrc requested a review from robert3005 September 4, 2026 12:20
@myrrc myrrc added the changelog/feature A new feature label Sep 4, 2026
@myrrc
myrrc enabled auto-merge (squash) September 4, 2026 12:20
Signed-off-by: Mikhail Kot <mikhail@spiraldb.com>
@myrrc
myrrc force-pushed the myrrc/cache-parsed-dtype-fields branch from 002b353 to 67e168c Compare September 4, 2026 12:23
@robert3005

Copy link
Copy Markdown
Contributor

Maybe FieldDType should have interior mutability and if we ever are forced to parse the view we mutate internal state and replace the view with the owned version?

@myrrc

myrrc commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

I've considered this option but it doesn't look neither less ugly nor more performant.

@codspeed-hq

codspeed-hq Bot commented Sep 4, 2026

Copy link
Copy Markdown

Merging this PR will regress 1 benchmark

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚠️ 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

⚡ 3 improved benchmarks
❌ 1 regressed benchmark
✅ 2195 untouched benchmarks
⏩ 206 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation random_i16[0.95] 77.2 µs 95.9 µs -19.55%
WallTime arrow_checked_add_u32_neon[16384] 20.4 µs 12.8 µs +59.92%
Simulation random_i16[0.8] 92.7 µs 75.3 µs +22.98%
Simulation allocate_drop_arrow[0] 456.9 ns 402.7 ns +13.45%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing myrrc/cache-parsed-dtype-fields (67e168c) with develop (3170657)

Open in CodSpeed

Footnotes

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

@myrrc

myrrc commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Even if we have interior mutability, the issue is value() + cloning the field. If they refer to a flatbuffer view, then we need to re-parse the copied view, so we need an Arc to prevent that.

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

I wish there was a nicer way but I can't think of one. You could come up with a way to apply expressions to dtypes but that still has to be progressively refined

@myrrc
myrrc merged commit 0d9d5b8 into develop Sep 4, 2026
82 of 83 checks passed
@myrrc
myrrc deleted the myrrc/cache-parsed-dtype-fields branch September 4, 2026 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/feature A new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants