-
Notifications
You must be signed in to change notification settings - Fork 432
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
[backend-comparison] Rework burnbench output to be nicer and more compact #1568
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1568 /- ##
==========================================
- Coverage 86.53% 86.34% -0.19%
==========================================
Files 684 686 2
Lines 78248 78418 170
==========================================
Hits 67713 67713
- Misses 10535 10705 170 ☔ View full report in Codecov by Sentry. |
5be8325
to
96695c9
Compare
4cdfc19
to
2605ad6
Compare
// let total_combinations = backends.len() * benches.len(); | ||
// println!("Running {} benchmark(s)...\n", total_combinations); |
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.
Dead code?
for bench in benches.iter() { | ||
for backend in backends.iter() { | ||
let bench_str = bench.to_string(); | ||
let backend_str = backend.to_string(); | ||
println!( | ||
"{}Benchmarking {} on {}{}", | ||
filler, bench_str, backend_str, filler | ||
); | ||
let url = format!("{}benchmarks", super::USER_BENCHMARK_SERVER_URL); | ||
let nice_processor: Option<Arc<NiceProcessor>> = runner_pb.clone().map(|pb| { | ||
Arc::new(NiceProcessor::new( | ||
bench_str.clone(), | ||
backend_str.clone(), |
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.
Would it be possible to refactor the inside of the two for loops into another function? That would be easier to read.
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.
Done
#[derive(Clone, Default)] | ||
struct CountTracker { | ||
pub failed: u64, | ||
pub succeeded: u64, | ||
} | ||
|
||
#[derive(Clone)] | ||
struct ThreadSafeTracker { | ||
inner: Arc<Mutex<CountTracker>>, | ||
} |
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 would use two AtomicU64
instead of locking, though I'm unsure why we need inner mutability here.
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.
That's a bit more complicated than that but maybe I am mistaken.
I need to pass the tracker down to progress bar style but I also need to pass it to my runner so it can update its state. I tried with an Arc like that:
pub(crate) struct RunnerProgressBar {
pb: ProgressBar,
tracker: Arc<Mutex<CountTracker>>,
}
impl RunnerProgressBar {
pub(crate) fn new(total: u64) -> Self {
let tracker = Arc::new(Mutex::new(CountTracker::default()));
let pb = ProgressBar::new(total);
pb.set_style(
ProgressStyle::default_spinner()
.template("\n{msg}\n{spinner}{wide_bar:.yellow/red} {pos}/{len} {counter}\n ")
.unwrap()
.with_key("counter", tracker.clone())
.progress_chars("▬▬―")
.tick_strings(&[
"🕛 ", "🕐 ", "🕑 ", "🕒 ", "🕓 ", "🕔 ", "🕕 ", "🕖 ", "🕗 ", "🕘 ", "🕙 ",
"🕚 ",
]),
);
Self {
pb,
tracker: tracker.clone(),
}
}
But then the Tracker trait is not implemented for the Arc'd CountTracker type and did not find an easy way to do it so I wrapped it.
Thinking about it, maybe I can avoid owning the tracker at the runner level if the indicatif interface gives me access to the tracker somehow. I'll take a look at this.
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 refactored the tracker and use a ref counted Atomic64. Pretty happy about it, thank you for the suggestion. It is much cleaner. Note that I used the weakest form of ordering as I am not familiar with the concept yet and atomicity only seems to just fined.
12d3978
to
b81feb9
Compare
Also simplified with only one processor for both stdout and stderr since now we also need to process stdout to detect uploading.
@nathanielsimard ready for another round. |
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.
LGTM
b81feb9
to
98733d4
Compare
To be merged after: #1567
Pull Request Template
Checklist
run-checks all
script has been executed.Related Issues/PRs
n/a
Changes
Add a
--verbose
parameter which corresponds to normal output of cargo build and cargo bench commands.By default (i.e. without the
--verbose
flag) a progress bar with status is displayed instead of just plain logs. The failed benchmarks are also now added in the reports.Progress Bar:
Results OK:
Results with failure:
Testing