-
Notifications
You must be signed in to change notification settings - Fork 646
Adding game loading benchmarks #801
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
Conversation
kazimuth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, it's nice to have some join benchmarks.
I'm also planning on adding some basic sql benches once #803 is merged, but those won't have joins so this is super helpful.
crates/bench/src/game.rs
Outdated
| /// Will generate on `release`: | ||
| /// Chunks * 100 | ||
| /// FootprintTileState * 1'000 | ||
| /// LocationState * 1'000.000 rows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make this smaller? Million row benchmarks add to the ci run time a lot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should not add this to criterion anyway...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Mario. This benchmark is mainly for performance analysis, so we don't need it to be run in CI. At least not right now.
| let start = Instant::now(); | ||
| black_box(fill_tables(&tables, data)?); | ||
|
|
||
| let result = black_box(run(&tables.db, "SELECT * FROM LocationState", AuthCtx::for_testing())?); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically this is now timing more than just insertions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah as long as the inserts succeed we shouldn't need this validation.
crates/bench/src/game.rs
Outdated
| assert_eq!(result, EXPECT_ROWS / 100); | ||
|
|
||
| #[cfg(not(debug_assertions))] | ||
| assert_eq!(result, EXPECT_ROWS / 10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly confused about the math here, could we define these as constants next to EXPECT_ROWS?
crates/bench/src/game.rs
Outdated
|
|
||
| fn make_test_db() -> Result<(RelationalDB, TempDir), DBError> { | ||
| let tmp_dir = TempDir::new("stdb_test")?; | ||
| let stdb = open_db(&tmp_dir, true, false)?.with_row_count(Arc::new(|_, _| i64::MAX)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want to use the default row count function so that we produce accurate plans.
crates/bench/benches/special.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn game_sql_benchmarks(c: &mut Criterion) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just add the utility for right now. We can decide later if we want this to be a CI benchmark.
crates/bench/src/game.rs
Outdated
| #[cfg(not(debug_assertions))] | ||
| pub const BASE_ROWS: usize = 1_000; | ||
| #[cfg(debug_assertions)] | ||
| pub const BASE_ROWS: usize = 10; | ||
| const EXPECT_ROWS: usize = BASE_ROWS * 1_000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets keep this simple for right now and just have a single table size. Let's do 1M rows each for both tables.
de5c5d7 to
18b059b
Compare
57a60b5 to
18b059b
Compare
18b059b to
a1e99c3
Compare
|
Closing as this was merged as part of #839 |
Description of Changes
Adding a test load that mimics patterns common in games we are interested in.
Also documentation on how to use https://github.com/mstange/samply/, which allows to see perf data on Firefox with integrated flamegraph.
Expected complexity level and risk
1