Skip to content

Commit 49440ef

Browse files
nnethercotennethercote
authored
Merge pull request #1483 from nnethercote/remove-footnotes
Remove GitHub comment footer, and fix table spacing.
2 parents c305870 + 4dd030c commit 49440ef

File tree

2 files changed

+138
-93
lines changed

2 files changed

+138
-93
lines changed

‎site/src/comparison.rs

Lines changed: 132 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,12 @@ use collector::Bound;
1313
use serde::{Deserialize, Serialize};
1414

1515
use database::CommitType;
16+
use std::cmp;
1617
use std::collections::{HashMap, HashSet};
1718
use std::error::Error;
1819
use std::fmt::Write;
1920
use std::hash::Hash;
21+
use std::iter;
2022
use std::sync::Arc;
2123

2224
type BoxedError = Box<dyn Error + Send + Sync>;
@@ -95,7 +97,7 @@ pub async fn handle_triage(
9597
.clone()
9698
.summarize_by_category(&benchmark_map);
9799
let mut result = String::from("**Summary**:\n\n");
98-
write_summary_table(&primary, &secondary, false, true, &mut result);
100+
write_summary_table(&primary, &secondary, true, &mut result);
99101
result
100102
}
101103
None => String::from("**ERROR**: no data found for end bound"),
@@ -330,7 +332,7 @@ impl ArtifactComparisonSummary {
330332
let cmp = |b1: &TestResultComparison, b2: &TestResultComparison| {
331333
b1.relative_change()
332334
.partial_cmp(&b2.relative_change())
333-
.unwrap_or(std::cmp::Ordering::Equal)
335+
.unwrap_or(cmp::Ordering::Equal)
334336
};
335337
relevant_comparisons.sort_by(cmp);
336338

@@ -498,7 +500,7 @@ impl ArtifactComparisonSummary {
498500
let most_neg = self.most_negative_change().unwrap();
499501
let most_pos_abs = most_pos.relative_change().abs();
500502
let most_neg_abs = most_neg.relative_change().abs();
501-
if most_neg_abs.partial_cmp(&most_pos_abs) == Some(std::cmp::Ordering::Greater) {
503+
if most_neg_abs.partial_cmp(&most_pos_abs) == Some(cmp::Ordering::Greater) {
502504
Some(most_neg)
503505
} else {
504506
Some(most_pos)
@@ -547,7 +549,7 @@ async fn write_triage_summary(
547549
let link = &compare_link(start, end);
548550
write!(&mut result, " [(Comparison Link)]({})\n\n", link).unwrap();
549551

550-
write_summary_table(&primary, &secondary, false, true, &mut result);
552+
write_summary_table(&primary, &secondary, true, &mut result);
551553

552554
result
553555
}
@@ -556,7 +558,6 @@ async fn write_triage_summary(
556558
pub fn write_summary_table(
557559
primary: &ArtifactComparisonSummary,
558560
secondary: &ArtifactComparisonSummary,
559-
with_footnotes: bool,
560561
include_metric: bool,
561562
result: &mut String,
562563
) {
@@ -569,7 +570,7 @@ pub fn write_summary_table(
569570
.map(|m| format!("({})", m.metric.as_str()))
570571
})
571572
.flatten()
572-
.unwrap_or_else(|| String::from(" "));
573+
.unwrap_or_else(|| " ".to_string());
573574

574575
fn render_stat<F: FnOnce() -> Option<f64>>(count: usize, calculate: F) -> String {
575576
let value = if count > 0 { calculate() } else { None };
@@ -588,10 +589,10 @@ pub fn write_summary_table(
588589
}
589590

590591
// (label, mean, max, count)
591-
let mut column_data = vec![];
592+
let mut columns = vec![];
592593

593594
// label
594-
column_data.push(vec![
595+
columns.push(vec![
595596
"Regressions ❌ <br /> (primary)".to_string(),
596597
"Regressions ❌ <br /> (secondary)".to_string(),
597598
"Improvements ✅ <br /> (primary)".to_string(),
@@ -600,7 +601,7 @@ pub fn write_summary_table(
600601
]);
601602

602603
// mean
603-
column_data.push(vec![
604+
columns.push(vec![
604605
render_stat(primary.num_regressions, || {
605606
Some(primary.arithmetic_mean_of_regressions())
606607
}),
@@ -622,7 +623,7 @@ pub fn write_summary_table(
622623

623624
// range
624625
let rel_change = |r: Option<&TestResultComparison>| r.unwrap().relative_change() * 100.0;
625-
column_data.push(vec![
626+
columns.push(vec![
626627
render_range(primary.num_regressions, || {
627628
(
628629
rel_change(primary.smallest_regression()),
@@ -656,7 +657,7 @@ pub fn write_summary_table(
656657
]);
657658

658659
// count
659-
column_data.push(vec![
660+
columns.push(vec![
660661
primary.num_regressions.to_string(),
661662
secondary.num_regressions.to_string(),
662663
primary.num_improvements.to_string(),
@@ -668,40 +669,70 @@ pub fn write_summary_table(
668669
// easy to read for anyone who is viewing the Markdown source.
669670
let column_labels = [
670671
metric,
671-
format!("mean{}", if with_footnotes { "[^1]" } else { "" }),
672+
"mean".to_string(),
672673
"range".to_string(),
673-
format!("count{}", if with_footnotes { "[^2]" } else { "" }),
674+
"count".to_string(),
674675
];
675-
let counts: Vec<usize> = column_labels.iter().map(|s| s.chars().count()).collect();
676-
for column in &column_labels {
677-
write!(result, "| {} ", column).unwrap();
676+
677+
// Calculate the console width of a string, allowing for double-width
678+
// chars. The `unicode_width` crate does this properly, but with only two
679+
// emoji in use we can do it easily ourselves.
680+
let width = |s: &str| {
681+
s.chars()
682+
.map(|c| if c == '❌' || c == '✅' { 2 } else { 1 })
683+
.sum()
684+
};
685+
686+
// Get the width of each column.
687+
let column_widths: Vec<usize> = column_labels
688+
.iter()
689+
.zip(columns.iter())
690+
.map(|(column_label, column)| {
691+
// Get the maximum width of the column data cells and the column label.
692+
column
693+
.iter()
694+
.chain(iter::once(column_label))
695+
.map(|cell| width(cell))
696+
.max()
697+
.unwrap()
698+
})
699+
.collect();
700+
701+
// Write column labels.
702+
for (column_label, &column_width) in column_labels.iter().zip(column_widths.iter()) {
703+
write!(
704+
result,
705+
"| {}{} ",
706+
column_label,
707+
" ".repeat(column_width - width(column_label))
708+
)
709+
.unwrap();
678710
}
679711
result.push_str("|\n");
680-
for &count in &counts {
681-
write!(result, "|:{}:", "-".repeat(count)).unwrap();
712+
713+
// Write lines under the column labels.
714+
for &column_width in &column_widths {
715+
write!(result, "|:{}:", "-".repeat(column_width)).unwrap();
682716
}
683717
result.push_str("|\n");
684718

685-
for row in 0..5 {
686-
let row_data = column_data.iter().map(|rows| rows[row].clone());
687-
debug_assert_eq!(row_data.len(), column_labels.len());
688-
for (column, &count) in row_data.zip(&counts) {
689-
write!(result, "| {:<1$} ", column, count).unwrap();
719+
// Write the column data.
720+
for row_idx in 0..5 {
721+
let row = columns.iter().map(|rows| rows[row_idx].clone());
722+
assert_eq!(row.len(), column_labels.len());
723+
for (cell, &column_width) in row.zip(column_widths.iter()) {
724+
write!(
725+
result,
726+
"| {}{} ",
727+
cell,
728+
" ".repeat(column_width - width(&cell))
729+
)
730+
.unwrap();
690731
}
691732
result.push_str("|\n");
692733
}
693734
}
694735

695-
pub fn write_summary_table_footer(result: &mut String) {
696-
writeln!(
697-
result,
698-
r#"
699-
[^1]: *the arithmetic mean of the percent change*
700-
[^2]: *number of relevant changes*"#
701-
)
702-
.unwrap();
703-
}
704-
705736
/// Compare two bounds on a given stat
706737
///
707738
/// Returns Ok(None) when no data for the end bound is present
@@ -1051,7 +1082,7 @@ impl HistoricalData {
10511082
.zip(self.data.iter())
10521083
.map(|(d, &r)| d / r)
10531084
.collect::<Vec<_>>();
1054-
deltas.sort_by(|d1, d2| d1.partial_cmp(d2).unwrap_or(std::cmp::Ordering::Equal));
1085+
deltas.sort_by(|d1, d2| d1.partial_cmp(d2).unwrap_or(cmp::Ordering::Equal));
10551086
deltas
10561087
}
10571088

@@ -1253,15 +1284,15 @@ impl TestResultComparison {
12531284
}
12541285
}
12551286

1256-
impl std::cmp::PartialEq for TestResultComparison {
1287+
impl cmp::PartialEq for TestResultComparison {
12571288
fn eq(&self, other: &Self) -> bool {
12581289
self.benchmark == other.benchmark
12591290
&& self.profile == other.profile
12601291
&& self.scenario == other.scenario
12611292
}
12621293
}
12631294

1264-
impl std::cmp::Eq for TestResultComparison {}
1295+
impl cmp::Eq for TestResultComparison {}
12651296

12661297
impl std::hash::Hash for TestResultComparison {
12671298
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
@@ -1437,11 +1468,13 @@ mod tests {
14371468
(Category::Primary, 1.0, 3.0),
14381469
],
14391470
r#"
1440-
| Regressions ❌ <br /> (primary) | 146.7% | [100.0%, 200.0%] | 3 |
1441-
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1442-
| Improvements ✅ <br /> (primary) | - | - | 0 |
1443-
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1444-
| All ❌✅ (primary) | 146.7% | [100.0%, 200.0%] | 3 |
1471+
| (instructions:u) | mean | range | count |
1472+
|:----------------------------------:|:------:|:----------------:|:-----:|
1473+
| Regressions ❌ <br /> (primary) | 146.7% | [100.0%, 200.0%] | 3 |
1474+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1475+
| Improvements ✅ <br /> (primary) | - | - | 0 |
1476+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1477+
| All ❌✅ (primary) | 146.7% | [100.0%, 200.0%] | 3 |
14451478
"#
14461479
.trim_start(),
14471480
);
@@ -1456,11 +1489,13 @@ mod tests {
14561489
(Category::Primary, 4.0, 1.0),
14571490
],
14581491
r#"
1459-
| Regressions ❌ <br /> (primary) | - | - | 0 |
1460-
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1461-
| Improvements ✅ <br /> (primary) | -71.7% | [-80.0%, -60.0%] | 3 |
1462-
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1463-
| All ❌✅ (primary) | -71.7% | [-80.0%, -60.0%] | 3 |
1492+
| (instructions:u) | mean | range | count |
1493+
|:----------------------------------:|:------:|:----------------:|:-----:|
1494+
| Regressions ❌ <br /> (primary) | - | - | 0 |
1495+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1496+
| Improvements ✅ <br /> (primary) | -71.7% | [-80.0%, -60.0%] | 3 |
1497+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1498+
| All ❌✅ (primary) | -71.7% | [-80.0%, -60.0%] | 3 |
14641499
"#
14651500
.trim_start(),
14661501
);
@@ -1475,11 +1510,13 @@ mod tests {
14751510
(Category::Secondary, 4.0, 1.0),
14761511
],
14771512
r#"
1478-
| Regressions ❌ <br /> (primary) | - | - | 0 |
1479-
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1480-
| Improvements ✅ <br /> (primary) | - | - | 0 |
1481-
| Improvements ✅ <br /> (secondary) | -71.7% | [-80.0%, -60.0%] | 3 |
1482-
| All ❌✅ (primary) | - | - | 0 |
1513+
| (instructions:u) | mean | range | count |
1514+
|:----------------------------------:|:------:|:----------------:|:-----:|
1515+
| Regressions ❌ <br /> (primary) | - | - | 0 |
1516+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1517+
| Improvements ✅ <br /> (primary) | - | - | 0 |
1518+
| Improvements ✅ <br /> (secondary) | -71.7% | [-80.0%, -60.0%] | 3 |
1519+
| All ❌✅ (primary) | - | - | 0 |
14831520
"#
14841521
.trim_start(),
14851522
);
@@ -1494,11 +1531,13 @@ mod tests {
14941531
(Category::Secondary, 1.0, 3.0),
14951532
],
14961533
r#"
1497-
| Regressions ❌ <br /> (primary) | - | - | 0 |
1498-
| Regressions ❌ <br /> (secondary) | 146.7% | [100.0%, 200.0%] | 3 |
1499-
| Improvements ✅ <br /> (primary) | - | - | 0 |
1500-
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1501-
| All ❌✅ (primary) | - | - | 0 |
1534+
| (instructions:u) | mean | range | count |
1535+
|:----------------------------------:|:------:|:----------------:|:-----:|
1536+
| Regressions ❌ <br /> (primary) | - | - | 0 |
1537+
| Regressions ❌ <br /> (secondary) | 146.7% | [100.0%, 200.0%] | 3 |
1538+
| Improvements ✅ <br /> (primary) | - | - | 0 |
1539+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1540+
| All ❌✅ (primary) | - | - | 0 |
15021541
"#
15031542
.trim_start(),
15041543
);
@@ -1514,11 +1553,13 @@ mod tests {
15141553
(Category::Primary, 4.0, 1.0),
15151554
],
15161555
r#"
1517-
| Regressions ❌ <br /> (primary) | 150.0% | [100.0%, 200.0%] | 2 |
1518-
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1519-
| Improvements ✅ <br /> (primary) | -62.5% | [-75.0%, -50.0%] | 2 |
1520-
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1521-
| All ❌✅ (primary) | 43.8% | [-75.0%, 200.0%] | 4 |
1556+
| (instructions:u) | mean | range | count |
1557+
|:----------------------------------:|:------:|:----------------:|:-----:|
1558+
| Regressions ❌ <br /> (primary) | 150.0% | [100.0%, 200.0%] | 2 |
1559+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1560+
| Improvements ✅ <br /> (primary) | -62.5% | [-75.0%, -50.0%] | 2 |
1561+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1562+
| All ❌✅ (primary) | 43.8% | [-75.0%, 200.0%] | 4 |
15221563
"#
15231564
.trim_start(),
15241565
);
@@ -1536,11 +1577,13 @@ mod tests {
15361577
(Category::Primary, 4.0, 1.0),
15371578
],
15381579
r#"
1539-
| Regressions ❌ <br /> (primary) | 150.0% | [100.0%, 200.0%] | 2 |
1540-
| Regressions ❌ <br /> (secondary) | 100.0% | [100.0%, 100.0%] | 1 |
1541-
| Improvements ✅ <br /> (primary) | -62.5% | [-75.0%, -50.0%] | 2 |
1542-
| Improvements ✅ <br /> (secondary) | -66.7% | [-66.7%, -66.7%] | 1 |
1543-
| All ❌✅ (primary) | 43.8% | [-75.0%, 200.0%] | 4 |
1580+
| (instructions:u) | mean | range | count |
1581+
|:----------------------------------:|:------:|:----------------:|:-----:|
1582+
| Regressions ❌ <br /> (primary) | 150.0% | [100.0%, 200.0%] | 2 |
1583+
| Regressions ❌ <br /> (secondary) | 100.0% | [100.0%, 100.0%] | 1 |
1584+
| Improvements ✅ <br /> (primary) | -62.5% | [-75.0%, -50.0%] | 2 |
1585+
| Improvements ✅ <br /> (secondary) | -66.7% | [-66.7%, -66.7%] | 1 |
1586+
| All ❌✅ (primary) | 43.8% | [-75.0%, 200.0%] | 4 |
15441587
"#
15451588
.trim_start(),
15461589
);
@@ -1554,11 +1597,13 @@ mod tests {
15541597
(Category::Primary, 5.0, 6.0),
15551598
],
15561599
r#"
1557-
| Regressions ❌ <br /> (primary) | 20.0% | [20.0%, 20.0%] | 1 |
1558-
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1559-
| Improvements ✅ <br /> (primary) | -50.0% | [-50.0%, -50.0%] | 1 |
1560-
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1561-
| All ❌✅ (primary) | -15.0% | [-50.0%, 20.0%] | 2 |
1600+
| (instructions:u) | mean | range | count |
1601+
|:----------------------------------:|:------:|:----------------:|:-----:|
1602+
| Regressions ❌ <br /> (primary) | 20.0% | [20.0%, 20.0%] | 1 |
1603+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1604+
| Improvements ✅ <br /> (primary) | -50.0% | [-50.0%, -50.0%] | 1 |
1605+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1606+
| All ❌✅ (primary) | -15.0% | [-50.0%, 20.0%] | 2 |
15621607
"#
15631608
.trim_start(),
15641609
);
@@ -1572,11 +1617,13 @@ mod tests {
15721617
(Category::Primary, 6.0, 5.0),
15731618
],
15741619
r#"
1575-
| Regressions ❌ <br /> (primary) | 100.0% | [100.0%, 100.0%] | 1 |
1576-
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1577-
| Improvements ✅ <br /> (primary) | -16.7% | [-16.7%, -16.7%] | 1 |
1578-
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1579-
| All ❌✅ (primary) | 41.7% | [-16.7%, 100.0%] | 2 |
1620+
| (instructions:u) | mean | range | count |
1621+
|:----------------------------------:|:------:|:----------------:|:-----:|
1622+
| Regressions ❌ <br /> (primary) | 100.0% | [100.0%, 100.0%] | 1 |
1623+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1624+
| Improvements ✅ <br /> (primary) | -16.7% | [-16.7%, -16.7%] | 1 |
1625+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1626+
| All ❌✅ (primary) | 41.7% | [-16.7%, 100.0%] | 2 |
15801627
"#
15811628
.trim_start(),
15821629
);
@@ -1625,8 +1672,14 @@ mod tests {
16251672
let secondary = ArtifactComparisonSummary::summarize(secondary_comparisons);
16261673

16271674
let mut result = String::new();
1628-
write_summary_table(&primary, &secondary, true, true, &mut result);
1629-
let header = "| (instructions:u) | mean[^1] | range | count[^2] |\n|:----------------:|:--------:|:-----:|:---------:|\n";
1630-
assert_eq!(result, format!("{header}{expected}"));
1675+
write_summary_table(&primary, &secondary, true, &mut result);
1676+
// We don't use `assert_eq!` here because it stringifies the arguments,
1677+
// making the tables hard to read when printed.
1678+
if result != expected {
1679+
panic!(
1680+
"output mismatch:\nexpected:\n{}actual:\n{}",
1681+
expected, result
1682+
);
1683+
}
16311684
}
16321685
}

0 commit comments

Comments
 (0)