Skip to content

Use the summary tables in triage report #1273

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 4, 2022
Merged

Use the summary tables in triage report #1273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
158 changes: 57 additions & 101 deletions site/src/comparison.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@ use crate::db::{ArtifactId, Benchmark, Lookup, Profile, Scenario};
use crate::github;
use crate::load::SiteCtxt;
use crate::selector::{self, Tag};
use std::cmp::Ordering;

use collector::category::Category;
use collector::Bound;
use serde::Serialize;

use std::cmp::Ordering;
use std::collections::{HashMap, HashSet};
use std::error::Error;
use std::hash::Hash;
Expand Down Expand Up @@ -71,7 +72,7 @@ pub async fn handle_triage(
);

// handle results of comparison
populate_report(&comparison, &mut report).await;
populate_report(ctxt, &comparison, &mut report).await;

// Check that there is a next commit and that the
// after commit is not equal to `end`
Expand Down Expand Up @@ -142,6 +143,7 @@ pub async fn handle_compare(
}

async fn populate_report(
ctxt: &SiteCtxt,
comparison: &Comparison,
report: &mut HashMap<Option<Direction>, Vec<String>>,
) {
Expand All @@ -153,7 +155,7 @@ async fn populate_report(
.entry(confidence.is_definitely_relevant().then(|| direction))
.or_default();

entry.push(summary.write(comparison).await)
entry.push(write_summary(ctxt, comparison).await)
}
}
}
Expand Down Expand Up @@ -206,11 +208,7 @@ impl ComparisonSummary {
};
comparisons.sort_by(cmp);

let errors_in = comparison
.new_errors
.keys()
.map(|k| k.as_str().to_owned())
.collect::<Vec<_>>();
let errors_in = comparison.new_errors.keys().cloned().collect::<Vec<_>>();

Some(ComparisonSummary {
comparisons,
Expand Down Expand Up @@ -296,16 +294,6 @@ impl ComparisonSummary {
self.num_regressions
}

/// The most relevant changes (based on size)
pub fn most_relevant_changes<'a>(&'a self) -> [Option<&TestResultComparison>; 2] {
match self.direction() {
Some(Direction::Improvement) => [self.largest_improvement(), None],
Some(Direction::Regression) => [self.largest_regression(), None],
Some(Direction::Mixed) => [self.largest_improvement(), self.largest_regression()],
None => [None, None],
}
}

/// Arithmetic mean of all improvements as a percent
pub fn arithmetic_mean_of_improvements(&self) -> f64 {
self.arithmetic_mean(self.improvements())
Expand Down Expand Up @@ -382,66 +370,32 @@ impl ComparisonSummary {
_ => ComparisonConfidence::MaybeRelevant,
}
}
}

async fn write(&self, comparison: &Comparison) -> String {
let mut result = if let Some(pr) = comparison.b.pr {
let title = github::pr_title(pr).await;
format!(
"{} [#{}](https://github.com/rust-lang/rust/pull/{})\n",
title, pr, pr
)
} else {
String::from("<Unknown Change>\n")
};
let start = &comparison.a.artifact;
let end = &comparison.b.artifact;
let link = &compare_link(start, end);

self.write_summary_lines(&mut result, Some(link));
async fn write_summary(ctxt: &SiteCtxt, comparison: &Comparison) -> String {
use std::fmt::Write;
let mut result = if let Some(pr) = comparison.b.pr {
let title = github::pr_title(pr).await;
format!(
"{} [#{}](https://github.com/rust-lang/rust/pull/{})",
title, pr, pr
)
} else {
String::from("<Unknown Change>")
};
let start = &comparison.a.artifact;
let end = &comparison.b.artifact;
let link = &compare_link(start, end);
write!(&mut result, " [(Comparison Link)]({})\n", link).unwrap();

result
}
let benchmark_map = ctxt.get_benchmark_category_map().await;
let (primary, secondary) = comparison.clone().summarize_by_category(benchmark_map);
let primary = primary.unwrap_or_else(ComparisonSummary::empty);
let secondary = secondary.unwrap_or_else(ComparisonSummary::empty);

pub fn write_summary_lines(&self, result: &mut String, link: Option<&str>) {
use std::fmt::Write;
if self.num_regressions > 1 {
writeln!(
result,
"- Arithmetic mean of relevant regressions: {:.1}%",
self.arithmetic_mean_of_regressions()
)
.unwrap();
}
if self.num_improvements > 1 {
writeln!(
result,
"- Arithmetic mean of relevant improvements: {:.1}%",
self.arithmetic_mean_of_improvements()
)
.unwrap();
}
if self.num_improvements > 0 && self.num_regressions > 0 {
writeln!(
result,
"- Arithmetic mean of all relevant changes: {:.1}%",
self.arithmetic_mean_of_changes()
)
.unwrap();
}
for change in self.most_relevant_changes().iter().filter_map(|s| *s) {
write!(result, "- ").unwrap();
change.summary_line(result, link)
}
write_summary_table(&primary, &secondary, &mut result);

if !self.errors_in.is_empty() {
write!(
result,
"- Benchmark(s) {} started failing to build",
self.errors_in.join(", ")
)
.unwrap();
}
}
result
}

/// Writes a Markdown table containing summary of relevant results.
Expand Down Expand Up @@ -800,6 +754,7 @@ impl From<ArtifactDescription> for api::comparison::ArtifactDescription {
}

// A comparison of two artifacts
#[derive(Clone)]
pub struct Comparison {
pub a: ArtifactDescription,
pub b: ArtifactDescription,
Expand Down Expand Up @@ -836,6 +791,34 @@ impl Comparison {
pub fn next(&self, master_commits: &[collector::MasterCommit]) -> Option<String> {
next_commit(&self.b.artifact, master_commits).map(|c| c.sha.clone())
}

/// Splits comparison into primary and secondary summaries based on benchmark category
pub fn summarize_by_category(
self,
map: HashMap<Benchmark, Category>,
) -> (Option<ComparisonSummary>, Option<ComparisonSummary>) {
let (primary, secondary) = self
.statistics
.into_iter()
.partition(|s| map.get(&s.benchmark()) == Some(&Category::Primary));

let primary = Comparison {
a: self.a.clone(),
b: self.b.clone(),
statistics: primary,
new_errors: self.new_errors.clone(),
};
let secondary = Comparison {
a: self.a,
b: self.b,
statistics: secondary,
new_errors: self.new_errors,
};
(
ComparisonSummary::summarize_comparison(&primary),
ComparisonSummary::summarize_comparison(&secondary),
)
}
}

/// The historical data for a certain benchmark
Expand Down Expand Up @@ -1111,33 +1094,6 @@ impl TestResultComparison {
let (a, b) = self.results;
(b - a) / a
}

fn direction(&self) -> Direction {
if self.relative_change() > 0.0 {
Direction::Regression
} else {
Direction::Improvement
}
}

pub fn summary_line(&self, summary: &mut String, link: Option<&str>) {
use std::fmt::Write;
let percent = self.relative_change() * 100.0;
writeln!(
summary,
"Largest {} in {}: {:.1}% on `{}` builds of `{} {}`",
self.direction(),
match link {
Some(l) => format!("[instruction counts]({})", l),
None => "instruction counts".into(),
},
percent,
self.scenario,
self.benchmark,
self.profile
)
.unwrap();
}
}

impl std::cmp::PartialEq for TestResultComparison {
Expand Down
66 changes: 5 additions & 61 deletions site/src/github.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,13 @@
use crate::api::github::Issue;
use crate::comparison::{
write_summary_table, Comparison, ComparisonConfidence, ComparisonSummary, Direction,
};
use crate::comparison::{write_summary_table, ComparisonConfidence, ComparisonSummary, Direction};
use crate::load::{Config, SiteCtxt, TryCommit};

use anyhow::Context as _;
use database::{ArtifactId, Benchmark, QueuedCommit};
use database::{ArtifactId, QueuedCommit};
use reqwest::header::USER_AGENT;
use serde::{Deserialize, Serialize};

use collector::category::Category;
use std::collections::{HashMap, HashSet};
use std::collections::HashSet;
use std::{fmt::Write, sync::Arc, time::Duration};

type BoxedError = Box<dyn std::error::Error + Send + Sync>;
Expand Down Expand Up @@ -651,21 +648,6 @@ compiler perf.{next_steps}
)
}

pub type BenchmarkMap = HashMap<Benchmark, Category>;

async fn get_benchmark_map(ctxt: &SiteCtxt) -> BenchmarkMap {
let benchmarks = ctxt.pool.connection().await.get_benchmarks().await;
benchmarks
.into_iter()
.map(|bench| {
(
bench.name.as_str().into(),
Category::from_db_representation(&bench.category).unwrap(),
)
})
.collect()
}

async fn categorize_benchmark(
ctxt: &SiteCtxt,
commit_sha: String,
Expand All @@ -684,8 +666,8 @@ async fn categorize_benchmark(
_ => return (String::from("ERROR categorizing benchmark run!"), None),
};

let benchmark_map = get_benchmark_map(ctxt).await;
let (primary, secondary) = split_comparison(comparison, benchmark_map);
let benchmark_map = ctxt.get_benchmark_category_map().await;
let (primary, secondary) = comparison.summarize_by_category(benchmark_map);

const DISAGREEMENT: &str = "If you disagree with this performance assessment, \
please file an issue in [rust-lang/rustc-perf](https://github.com/rust-lang/rustc-perf/issues/new).";
Expand Down Expand Up @@ -772,44 +754,6 @@ fn generate_short_summary(
}
}

/// Splits comparison into primary and secondary summaries based on benchmark category
fn split_comparison(
comparison: Comparison,
map: BenchmarkMap,
) -> (Option<ComparisonSummary>, Option<ComparisonSummary>) {
let mut primary = HashSet::new();
let mut secondary = HashSet::new();

for statistic in comparison.statistics {
let category: Category = map
.get(&statistic.benchmark())
.copied()
.unwrap_or_else(|| Category::Secondary);
if let Category::Primary = category {
primary.insert(statistic);
} else {
secondary.insert(statistic);
}
}

let primary = Comparison {
a: comparison.a.clone(),
b: comparison.b.clone(),
statistics: primary,
new_errors: comparison.new_errors.clone(),
};
let secondary = Comparison {
a: comparison.a,
b: comparison.b,
statistics: secondary,
new_errors: comparison.new_errors,
};
(
ComparisonSummary::summarize_comparison(&primary),
ComparisonSummary::summarize_comparison(&secondary),
)
}

/// Whether a comparison is relevant enough to show
fn comparison_is_relevant(confidence: ComparisonConfidence, is_master_commit: bool) -> bool {
if is_master_commit {
Expand Down
23 changes: 17 additions & 6 deletions site/src/load.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,19 @@
use arc_swap::{ArcSwap, Guard};
use std::collections::HashSet;
use std::collections::{HashMap, HashSet};
use std::fs;
use std::ops::RangeInclusive;
use std::path::Path;
use std::sync::Arc;
use std::time::Instant;

use arc_swap::{ArcSwap, Guard};
use chrono::{Duration, Utc};
use log::error;
use serde::{Deserialize, Serialize};

use crate::api::github;
use crate::db;
use collector::{Bound, MasterCommit};
use collector::{category::Category, Bound, MasterCommit};
use database::Date;

use crate::api::github;
use collector;
use database::Pool;
pub use database::{ArtifactId, Benchmark, Commit};

Expand Down Expand Up @@ -217,6 +215,19 @@ impl SiteCtxt {
)
}

pub async fn get_benchmark_category_map(&self) -> HashMap<Benchmark, Category> {
let benchmarks = self.pool.connection().await.get_benchmarks().await;
benchmarks
.into_iter()
.map(|bench| {
(
bench.name.as_str().into(),
Category::from_db_representation(&bench.category).unwrap(),
)
})
.collect()
}

/// Get cached master-branch Rust commits.
/// Returns cached results immediately, but if the cached value is older than one minute,
/// updates in a background task for next time.
Expand Down