Skip to content

Add more benchmarks to algobench - #2655

Open
DanielLacina wants to merge 28 commits into
Pometry:db_v4from
DanielLacina:algo_optimization
Open

Add more benchmarks to algobench#2655
DanielLacina wants to merge 28 commits into
Pometry:db_v4from
DanielLacina:algo_optimization

Conversation

@DanielLacina

@DanielLacina DanielLacina commented Jun 19, 2026

Copy link
Copy Markdown
Collaborator

What changes were proposed in this pull request?

Added algo benchmarks that benchmark all algorithms with different views

Why are the changes needed?

To give insights on what algorithms need to be improved

Does this PR introduce any user-facing change? If yes is this documented?

no

How was this patch tested?

its a benchmark

Are there any further changes required?

no

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

LGTM

Edit: Will need to fix the errors on CI...

Comment thread raphtory-benchmark/benches/algobench.rs Outdated
Comment thread raphtory-benchmark/benches/algobench.rs Outdated
Comment thread raphtory-benchmark/benches/algobench.rs Outdated
@fabubaker fabubaker changed the title Algo optimization Add more benchmarks to algobench Jul 17, 2026
Comment thread raphtory-benchmark/benches/algobench.rs Outdated
20,
10,
large_random_attachment_filtered,
|graph, _| louvain::<ModularityUnDir, _>(graph, 1.0, None, None),

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.

This is broken on CI

@ljeub-pometry ljeub-pometry left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

  • I don't think you need the custom iter (unless some algorithm has extremely variable runtime, but then benchmarking it would be essentially useless anyway)
  • don't shrink the warmup time too much (might contribute to bad timing estimates)
  • Possible deadlocks in graph construction (see comments)
  • Could simplify some code by getting rid of closures that get only called once and pass in the values directly (not really a problem that you need to change, more of a stylistic comment for the future)

Comment thread raphtory-benchmark/src/algobench_common.rs Outdated
Comment thread raphtory-benchmark/src/algobench_common.rs Outdated
c: &mut Criterion,
name: &str,
sample_size: usize,
build_graph: BuildGraph,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You can just pass in graph: G?

name: &str,
sample_size: usize,
build_graph: BuildGraph,
setup: Setup,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same as above, no reason to pass in a closure, you are only calling it once, you can just pass in the result

Comment thread raphtory-benchmark/src/algobench_common.rs
Comment thread raphtory-benchmark/src/algobench_common.rs Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants