Skip to content

Add extended stats to GH summary table #1322

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 5 commits into from
May 19, 2022
Merged

Add extended stats to GH summary table #1322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e1087d2
Separate footer from summary table
Kobzol May 15, 2022
33a3461
Include more stats in GH summary table
Kobzol May 15, 2022
935c4da
Turn Stat into an enum
Kobzol May 16, 2022
98b4aba
Always add Max RSS and cycles to PR summary
Kobzol May 18, 2022
a77669f
Refactor stat summaries
Kobzol May 18, 2022
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
3 changes: 2 additions & 1 deletion site/src/api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ pub mod bootstrap {
}

pub mod comparison {
use crate::comparison::Stat;
use collector::Bound;
use database::{BenchmarkData, Date};
use serde::{Deserialize, Serialize};
Expand All @@ -139,7 +140,7 @@ pub mod comparison {
pub struct Request {
pub start: Bound,
pub end: Bound,
pub stat: String,
pub stat: Stat,
}

#[derive(Debug, Clone, Serialize, Deserialize)]
Expand Down
94 changes: 50 additions & 44 deletions site/src/comparison.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,12 @@ use crate::selector::{self, Tag};

use collector::category::Category;
use collector::Bound;
use serde::Serialize;
use serde::{Deserialize, Serialize};

use std::cmp::Ordering;
use std::collections::{HashMap, HashSet};
use std::error::Error;
use std::fmt::Write;
use std::hash::Hash;
use std::sync::Arc;

Expand Down Expand Up @@ -44,11 +45,11 @@ pub async fn handle_triage(
let mut before = start.clone();

let mut num_comparisons = 0;
let stat = "instructions:u".to_owned();
let stat = Stat::Instructions;
let benchmark_map = ctxt.get_benchmark_category_map().await;
loop {
let comparison =
match compare_given_commits(before, next.clone(), stat.clone(), ctxt, &master_commits)
match compare_given_commits(before, next.clone(), stat, ctxt, &master_commits)
.await
.map_err(|e| format!("error comparing commits: {}", e))?
{
Expand Down Expand Up @@ -171,14 +172,46 @@ async fn populate_report(
(None, None) => return,
};

let include_in_triage = deserves_attention(&primary, &secondary);
let include_in_triage = deserves_attention_icount(&primary, &secondary);

if include_in_triage {
let entry = report.entry(direction).or_default();
entry.push(write_triage_summary(comparison, &primary, &secondary).await);
}
}

#[derive(Copy, Clone, Debug, PartialEq, Serialize, Deserialize)]
pub enum Stat {
#[serde(rename = "instructions:u")]
Instructions,
#[serde(rename = "cycles:u")]
Cycles,
#[serde(rename = "faults")]
Faults,
#[serde(rename = "max-rss")]
MaxRSS,
#[serde(rename = "task-clock")]
TaskClock,
#[serde(rename = "wall-time")]
WallTime,
#[serde(rename = "cpu-clock")]
CpuClock,
}

impl Stat {
pub fn as_str(&self) -> &'static str {
match self {
Self::Instructions => "instructions:u",
Self::Cycles => "cycles:u",
Self::Faults => "faults",
Self::MaxRSS => "max-rss",
Self::TaskClock => "task-clock",
Self::WallTime => "wall-time",
Self::CpuClock => "cpu-clock",
}
}
}

/// A summary of a given comparison
///
/// This summary only includes changes that are significant and relevant (as determined by a change's magnitude).
Expand Down Expand Up @@ -366,7 +399,7 @@ impl ArtifactComparisonSummary {
///
/// For example, this can be used to determine if artifact comparisons with regressions should be labeled with the
/// `perf-regression` GitHub label or should be shown in the perf triage report.
pub(crate) fn deserves_attention(
pub(crate) fn deserves_attention_icount(
primary: &ArtifactComparisonSummary,
secondary: &ArtifactComparisonSummary,
) -> bool {
Expand All @@ -388,7 +421,6 @@ async fn write_triage_summary(
primary: &ArtifactComparisonSummary,
secondary: &ArtifactComparisonSummary,
) -> String {
use std::fmt::Write;
let mut result = if let Some(pr) = comparison.b.pr {
let title = github::pr_title(pr).await;
format!(
Expand All @@ -415,8 +447,6 @@ pub fn write_summary_table(
with_footnotes: bool,
result: &mut String,
) {
use std::fmt::Write;

fn render_stat<F: FnOnce() -> Option<f64>>(count: usize, calculate: F) -> String {
let value = if count > 0 { calculate() } else { None };
value
Expand Down Expand Up @@ -538,16 +568,16 @@ pub fn write_summary_table(
}),
largest_change,
]);
}

if with_footnotes {
writeln!(
result,
r#"
pub fn write_summary_table_footer(result: &mut String) {
writeln!(
result,
r#"
[^1]: *number of relevant changes*
[^2]: *the arithmetic mean of the percent change*"#
)
.unwrap();
}
)
.unwrap();
}

/// Compare two bounds on a given stat
Expand All @@ -556,7 +586,7 @@ pub fn write_summary_table(
pub async fn compare(
start: Bound,
end: Bound,
stat: String,
stat: Stat,
ctxt: &SiteCtxt,
) -> Result<Option<ArtifactComparison>, BoxedError> {
let master_commits = &ctxt.get_master_commits().commits;
Expand All @@ -568,7 +598,7 @@ pub async fn compare(
async fn compare_given_commits(
start: Bound,
end: Bound,
stat: String,
stat: Stat,
ctxt: &SiteCtxt,
master_commits: &[collector::MasterCommit],
) -> Result<Option<ArtifactComparison>, BoxedError> {
Expand All @@ -587,7 +617,7 @@ async fn compare_given_commits(
.set::<String>(Tag::Benchmark, selector::Selector::All)
.set::<String>(Tag::Scenario, selector::Selector::All)
.set::<String>(Tag::Profile, selector::Selector::All)
.set(Tag::Metric, selector::Selector::One(stat.clone()));
.set(Tag::Metric, selector::Selector::One(stat.as_str()));

// `responses` contains series iterators. The first element in the iterator is the data
// for `a` and the second is the data for `b`
Expand Down Expand Up @@ -834,7 +864,7 @@ impl HistoricalDataMap {
ctxt: &SiteCtxt,
from: ArtifactId,
master_commits: &[collector::MasterCommit],
stat: String,
stat: Stat,
) -> Result<Self, BoxedError> {
let mut historical_data = HashMap::new();

Expand All @@ -856,7 +886,7 @@ impl HistoricalDataMap {
.set::<String>(Tag::Benchmark, selector::Selector::All)
.set::<String>(Tag::Scenario, selector::Selector::All)
.set::<String>(Tag::Profile, selector::Selector::All)
.set(Tag::Metric, selector::Selector::One(stat));
.set(Tag::Metric, selector::Selector::One(stat.as_str()));

let mut previous_commit_series = ctxt
.statistic_series(query, previous_commits.clone())
Expand Down Expand Up @@ -1272,9 +1302,6 @@ mod tests {
| count[^1] | 3 | 0 | 0 | 0 | 3 |
| mean[^2] | 146.7% | N/A | N/A | N/A | 146.7% |
| max | 200.0% | N/A | N/A | N/A | 200.0% |

[^1]: *number of relevant changes*
[^2]: *the arithmetic mean of the percent change*
"#
.trim_start(),
);
Expand All @@ -1294,9 +1321,6 @@ mod tests {
| count[^1] | 0 | 0 | 3 | 0 | 3 |
| mean[^2] | N/A | N/A | -71.7% | N/A | -71.7% |
| max | N/A | N/A | -80.0% | N/A | -80.0% |

[^1]: *number of relevant changes*
[^2]: *the arithmetic mean of the percent change*
"#
.trim_start(),
);
Expand All @@ -1316,9 +1340,6 @@ mod tests {
| count[^1] | 0 | 0 | 0 | 3 | 0 |
| mean[^2] | N/A | N/A | N/A | -71.7% | N/A |
| max | N/A | N/A | N/A | -80.0% | N/A |

[^1]: *number of relevant changes*
[^2]: *the arithmetic mean of the percent change*
"#
.trim_start(),
);
Expand All @@ -1338,9 +1359,6 @@ mod tests {
| count[^1] | 0 | 3 | 0 | 0 | 0 |
| mean[^2] | N/A | 146.7% | N/A | N/A | N/A |
| max | N/A | 200.0% | N/A | N/A | N/A |

[^1]: *number of relevant changes*
[^2]: *the arithmetic mean of the percent change*
"#
.trim_start(),
);
Expand All @@ -1361,9 +1379,6 @@ mod tests {
| count[^1] | 2 | 0 | 2 | 0 | 4 |
| mean[^2] | 150.0% | N/A | -62.5% | N/A | 43.8% |
| max | 200.0% | N/A | -75.0% | N/A | 200.0% |

[^1]: *number of relevant changes*
[^2]: *the arithmetic mean of the percent change*
"#
.trim_start(),
);
Expand All @@ -1386,9 +1401,6 @@ mod tests {
| count[^1] | 2 | 1 | 2 | 1 | 4 |
| mean[^2] | 150.0% | 100.0% | -62.5% | -66.7% | 43.8% |
| max | 200.0% | 100.0% | -75.0% | -66.7% | 200.0% |

[^1]: *number of relevant changes*
[^2]: *the arithmetic mean of the percent change*
"#
.trim_start(),
);
Expand All @@ -1407,9 +1419,6 @@ mod tests {
| count[^1] | 1 | 0 | 1 | 0 | 2 |
| mean[^2] | 20.0% | N/A | -50.0% | N/A | -15.0% |
| max | 20.0% | N/A | -50.0% | N/A | -50.0% |

[^1]: *number of relevant changes*
[^2]: *the arithmetic mean of the percent change*
"#
.trim_start(),
);
Expand All @@ -1428,9 +1437,6 @@ mod tests {
| count[^1] | 1 | 0 | 1 | 0 | 2 |
| mean[^2] | 100.0% | N/A | -16.7% | N/A | 41.7% |
| max | 100.0% | N/A | -16.7% | N/A | 100.0% |

[^1]: *number of relevant changes*
[^2]: *the arithmetic mean of the percent change*
"#
.trim_start(),
);
Expand Down
Loading