Skip to content

Use ArcSwap for aggregate fn registry#8072

Open
robert3005 wants to merge 3 commits into
developfrom
rk/aggregatearcswap
Open

Use ArcSwap for aggregate fn registry#8072
robert3005 wants to merge 3 commits into
developfrom
rk/aggregatearcswap

Conversation

@robert3005
Copy link
Copy Markdown
Contributor

ArcSwap is faster than a lock for read. These session are mutable but mutations
are rare and retrievals are common

Signed-off-by: Robert Kruszewski <github@robertk.io>
@robert3005 robert3005 requested a review from gatesn May 22, 2026 22:50
@robert3005 robert3005 added the changelog/chore A trivial change label May 22, 2026
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented May 22, 2026

Merging this PR will not alter performance

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

⚡ 1 improved benchmark
❌ 3 regressed benchmarks
✅ 1257 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation chunked_varbinview_opt_canonical_into[(1000, 10)] 225.1 µs 187.5 µs +20.05%
Simulation chunked_varbinview_canonical_into[(100, 100)] 273.1 µs 307.8 µs -11.27%
Simulation baseline_lt[16, 65536] 217.4 µs 245 µs -11.24%
Simulation null_count_run_end[(10000, 4, 0.01)] 112.7 µs 126.5 µs -10.93%

Tip

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


Comparing rk/aggregatearcswap (ba733d5) with develop (4b089af)

Open in CodSpeed

Comment thread vortex-array/src/aggregate_fn/session.rs
Comment thread vortex-array/src/aggregate_fn/session.rs Outdated
Comment thread vortex-array/src/aggregate_fn/accumulator.rs Outdated
Signed-off-by: Robert Kruszewski <github@robertk.io>
Signed-off-by: Robert Kruszewski <github@robertk.io>
@robert3005 robert3005 requested a review from onursatici May 27, 2026 10:26
.or_else(|| kernels_r.get(&(batch_id, None)))
.copied();
drop(kernels_r);
let kernel =
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.

❤️

use crate::arrays::dict::compute::min_max::DictMinMaxKernel;

/// Registry of aggregate function vtables.
pub type AggregateFnRegistry = Registry<AggregateFnPluginRef>;
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.

Shall we have a pattern like this for ArcSwap?

Copy link
Copy Markdown
Contributor

@joseph-isaacs joseph-isaacs left a comment

Choose a reason for hiding this comment

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

Can we break this out into a pattern with a struct or type wrapper?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/chore A trivial change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants