-
Notifications
You must be signed in to change notification settings - Fork 30
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
improve(benchmarks): support multiple values + visualizing runtimes #316
base: main
Are you sure you want to change the base?
Conversation
f91abb4
to
430d974
Compare
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.
Seems good overall, but some issues:
|
||
```bash | ||
./zig-out/bin/benchmark accounts_db_readwrite -r 2>&1 | tee bench_results.txt # save output to file | ||
# NOTE: need to format doc to below | ||
python scripts/view_bench.py bench_results.txt # view runtimes as a charts with one file source | ||
python scripts/view_bench.py bench_results.txt b_results2.txt # compare runtimes against two *equivalent* files | ||
``` | ||
|
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.
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.
scripts/view_bench.py
Outdated
for df_i, df in enumerate(dfs): | ||
benchmark_runtimes = df.T[1:][i] | ||
# convert to milliseconds | ||
if units == 'ms': | ||
benchmark_runtimes = benchmark_runtimes / 1_000_000 | ||
if units == 's': | ||
benchmark_runtimes = benchmark_runtimes / 1_000_000_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.
this seems to be the source of the error in my previous comment
benchmark_runtimes = benchmark_runtimes / 1_000_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.
src/accountsdb/db.zig
Outdated
// defer logger.deinit(); | ||
// logger.spawn(); |
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.
dangling comments
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.
src/accountsdb/swiss_map.zig
Outdated
}); | ||
|
||
return write_time; | ||
// const read_faster_or_slower = if (read_speedup < 1.0) "slower" else "faster"; |
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.
dangling 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.
// test "benchmarkPullRequests" { | ||
// _ = try BenchmarkGossipServicePullRequests.benchmarkPullRequests(.{ | ||
// .name = "1k_data_1k_pull_reqs", | ||
// .n_data_populated = 10, | ||
// .n_pull_requests = 2, | ||
// }); | ||
// } | ||
|
||
// test "benchmarkGossipService" { | ||
// _ = try BenchmarkGossipServiceGeneral.benchmarkGossipService(.{ | ||
// .message_counts = .{ | ||
// .n_ping = 10, | ||
// .n_push_message = 10, | ||
// .n_pull_response = 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.
Are we fixing and re-enabling these, or removing these?
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.
left a comment - right now they leak when run with the testing allocator -- will fix the leak in another PR since its a bit more involved to fix
https://github.com/orgs/Syndica/projects/2/views/10?pane=issue&itemId=84676970
Useful for when you need to build a specific executable, but not install it.
bb8257f
to
c5d914b
Compare
discussed offline - changes to be included:
|
docs/benchmarks.md
scripts/view_bench.py
src/benchmarks.zig
support multiple values from a benchmark
NOTE: for multiple value outputs its not human readable at all - but i think thats ok -- most of our confirmations of speed ups/slow downs shouldnt be human checked, it should be computed and the increase/decrease should be human checked (which this PR allows for you to do easily)
support viewing all runtimes (
-r
){benchmark_name} ({field}), {runtime1}, {runtime2}, ...
visualizing all runtimes
example output:
note: builds off #289 -- i switched to mainly working on this feat so moved it to my own branch (instead of rexi's)