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

@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 degrade performance by 10.45%

⚠️ 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 regressed benchmark
✅ 1265 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation null_count_run_end[(10000, 4, 0.01)] 113.3 µs 126.5 µs -10.45%

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 (a472bd2) with develop (126c431)

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
@robert3005 robert3005 requested a review from onursatici May 27, 2026 10:26
Comment thread vortex-array/src/aggregate_fn/accumulator.rs
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?

@robert3005
Copy link
Copy Markdown
Contributor Author

I will make a follow up.

Signed-off-by: Robert Kruszewski <github@robertk.io>
Signed-off-by: Robert Kruszewski <github@robertk.io>
Signed-off-by: Robert Kruszewski <github@robertk.io>
@robert3005 robert3005 force-pushed the rk/aggregatearcswap branch from ba733d5 to a472bd2 Compare May 27, 2026 23:19
@robert3005 robert3005 enabled auto-merge (squash) May 27, 2026 23:19
@robert3005
Copy link
Copy Markdown
Contributor Author

Looks like this broke the backcompat check https://github.com/vortex-data/vortex/actions/runs/26544463401/job/78201950621?pr=8072. Need to investigate this tomorrow as it's passing locally

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