Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add benchmarking tests as in state_changes module #184

Merged

Conversation

0xbbbaron
Copy link

@0xbbbaron 0xbbbaron commented Jul 5, 2024

Motivation

In benches module there was a "TODO" to add state change benchmarking tests, which I did.

Solution

Simple benchmarking tests added as in state_change::tests module.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

Bench Output

Here's the results on my machine on cargo bench call:

test result: ok. 0 passed; 0 failed; 30 ignored; 0 measured; 0 filtered out; finished in 0.00s

Running benches/state_space.rs (target/release/deps/state_space-eb8fcd7e36391f42)
Gnuplot not found, using plotters backend
add state changes benchmark
                        time:   [116.82 ns 116.88 ns 116.94 ns]
                        change: [-0.2926% -0.1940% -0.0996%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild

add empty state changes benchmark
                        time:   [96.076 ns 96.159 ns 96.270 ns]
                        change: [-0.5076% -0.3311% -0.1669%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 3 outliers among 100 measurements (3.00%)
  3 (3.00%) high severe

@0xbbbaron
Copy link
Author

@0xKitsune @0xOsiris 🙂

criterion_group!(benches, criterion_benchmark);
criterion_main!(benches);

mod bench_funcs {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets remove this additional module and just put any helper functions directly the existing mod.


/// Duplicated helper method entirely as in `state_space::tests` module
/// Reason of duplication: to retain encapsulation
async fn add_state_change_to_cache(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of adding this function, lets just mark the existing function in src/state_space/mod.rs as pub.

Ok(())
}

/// Duplicated method test from `state_change::tests` module
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thing here lets dedup this function and expose whatever is necessary in the state space module.

Ok(())
}

/// Duplicated method test from `state_change::tests` module
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thing here

@0xKitsune
Copy link
Collaborator

Hey thanks for the PR and sorry for the delay!

I added a few quick comments. Let me know if you have any thoughts/questions.

@0xbbbaron
Copy link
Author

@0xKitsune Changes applied based on your comments

@0xKitsune 0xKitsune merged commit 30a284d into darkforestry:main Aug 24, 2024
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.

2 participants