[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. |
syl20bnr
force-pushed
the
feat/nice-burnbench-output
branch
from
5be8325
to
96695c9
Compare
syl20bnr
force-pushed
the
feat/nice-burnbench-output
branch
from
4cdfc19
to
2605ad6
Compare
| // let total_combinations = backends.len() * benches.len(); | ||
| // println!("Running {} benchmark(s)...\n", total_combinations); |
| 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.
Would it be possible to refactor the inside of the two for loops into another function? That would be easier to read.
| #[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.
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.
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.
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.
syl20bnr
force-pushed
the
feat/nice-burnbench-output
branch
from
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. |
syl20bnr
force-pushed
the
feat/nice-burnbench-output
branch
from
b81feb9
to
98733d4
Compare
To be merged after: #1567
Pull Request Template
Checklist
run-checks allscript has been executed.Related Issues/PRs
n/a
Changes
Add a
--verboseparameter which corresponds to normal output of cargo build and cargo bench commands.By default (i.e. without the
--verboseflag) 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