Skip to content

[nexus] Refactor test-utilities to helper crate, add test benchmarks #492

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

Merged
merged 8 commits into from
Dec 9, 2021

Conversation

smklein
Copy link
Collaborator

@smklein smklein commented Dec 8, 2021

Results are visible by running:

$ cargo bench -p omicron-nexus --bench setup_benchmark

Test Setup/do_full_setup                                                                          
                        time:   [8.3949 s 8.4655 s 8.5444 s]
                        change: [+2.0754% +4.8573% +7.9552%] (p = 0.00 < 0.05)

Test Setup/do_crdb_setup                                                                          
                        time:   [7.7077 s 7.7553 s 7.8020 s]
                        change: [+2.6847% +6.6535% +11.453%] (p = 0.00 < 0.05)

Test Setup/do_clickhouse_setup                                                                          
                        time:   [517.82 ms 520.83 ms 524.71 ms]
                        change: [-0.6317% +0.3463% +1.2429%] (p = 0.53 > 0.05)

This implies:

  • Running a "null" Nexus test takes about 8.5 seconds
  • 90% of that time is setting up + tearing down CRDB.

davepacheco
davepacheco previously approved these changes Dec 9, 2021
Copy link
Collaborator

@davepacheco davepacheco left a comment

Choose a reason for hiding this comment

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

Neat. Is there an easy way to get on-CPU time vs. blocked time? I wonder how much of this latency is hidden by the parallelism.

@davepacheco
Copy link
Collaborator

Oh, one question before you land this: should we not add this crate to the workspace?

@davepacheco davepacheco dismissed their stale review December 9, 2021 00:14

question about workspace toml

@davepacheco
Copy link
Collaborator

Output from my test machine (lots of noise trimmed out):

$ cargo bench -p omicron-nexus --bench setup_benchmark
...
                                                                                     Test Setup/do_full_setup                        
                        time:   [5.4927 s 5.5361 s 5.5889 s]
Found 1 outliers among 10 measurements (10.00%)
  1 (10.00%) high mild
...
Test Setup/do_crdb_setup                        
                        time:   [4.7564 s 4.7928 s 4.8355 s]
Found 1 outliers among 10 measurements (10.00%)
  1 (10.00%) high mild
...
Test Setup/do_clickhouse_setup                        
                        time:   [523.34 ms 525.28 ms 527.28 ms]

This is quite a bit faster than you saw. This is ivanova, which is a Matisse with 16 threads and 64 GiB of DRAM.

@smklein
Copy link
Collaborator Author

smklein commented Dec 9, 2021

Neat. Is there an easy way to get on-CPU time vs. blocked time? I wonder how much of this latency is hidden by the parallelism.

Certainly would be possible to add profiling, but that doesn't seem to exist out-of-the-box with criterion. They do provide hooks if we have a profiler we wanna use.

Oh, one question before you land this: should we not add this crate to the workspace?

Sure, added. I don't really have a clear idea when this is necessary at the top-level, vs just being an implicit dependency -- regardless of status in the top-level workspace file, this crate gets built when nexus is tested, since it's a dev-dependency. By adding it to the workspace, it'll be built (I think) even when we aren't trying to build tests. Maybe that's fine?

This is quite a bit faster than you saw. This is ivanova, which is a Matisse with 16 threads and 64 GiB of DRAM.

I'm running on a Linux laptop with 16 threads, but 32 GB DRAM. TBH, I don't think absolute performance matters as much as relative performance. But you bring up a good point regarding parallelism.

Copy link
Contributor

@david-crespo david-crespo left a comment

Choose a reason for hiding this comment

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

whatever enables #493 is good

@smklein smklein merged commit 914924d into main Dec 9, 2021
@smklein smklein deleted the benchmark-tests branch December 9, 2021 21:51
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