-
Notifications
You must be signed in to change notification settings - Fork 30
Improve benchmark stability with cpusets, nice, and disabling of hyper threading #354
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
base: main
Are you sure you want to change the base?
Conversation
For the curious, it looks like it isn't finding any cpuset files in CI, it's only setting the nice value there. |
Looks pretty good to me but I will also tag Alan and Kokubun to review since it's very important that we get benchmarking right. |
require 'etc' | ||
require 'yaml' | ||
require_relative 'misc/stats' | ||
require_relative 'misc/benchmark_mode' |
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.
Unlike misc/graph.rb
that supports both standalone execution and use as a library or other standalone scripts, these two files (misc/stats.rb
and misc/benchmark_mode.rb
) only make sense as a library for run_benchmarks.rb
. So it feels confusing to me to have them under misc/
.
WDYT about moving misc/stats.rb
and misc/benchmark_mode.rb
to lib/
? And it might be also nice to split BenchmarkMode::Xxx
into lib/benchmark_mode/xxx.rb
?
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.
Sounds good
|
||
# Disable Turbo Boost while running benchmarks. Maximize the CPU frequency. | ||
def set_bench_config(turbo:) | ||
def set_bench_config(turbo:, benchmark_mode:) |
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.
Can we move the turbo boost one (and maybe check_pstate
as well. I prefer it to still not have disable/disengage though) into BenchmarkMode
as well? It seems inconsistent (and thus confusing) that some things live in run_benchmarks.rb
and others in BenchmarkMode
while they seem to serve the same purpose.
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 we can move it all under one namespace, I was just waiting to get some general feedback before doing more work on it 👍
Part of the point of Can we rename |
Thanks for the feedback. |
This adds to
run_benchmarks.rb
several additional linux features which can help to make the benchmark results more reliable:You can see this improve the results when running the same ruby version twice:
it increases the number of benchmarks that compare at
1.000
on two different computers:By putting all this logic into a module we can easily reuse it from
yjit-metrics
.I think it would make sense to eventually move the existing checks for
turbo
into this module as well.