Skip to content

Commit 8120c94

Browse files
committed
Fix determination of whether or not to calculate variance
We were bailing if `variance_data.len() < MIN_PREVIOUS_COMMITS`, but `variance_data.len()` isn't a commit count, but the count of test cases for which we have at least one stat in the DB for the previous 50 to 100 commits. Bailing on this condition probably wasn't the intended behavior. Instead, make the determination of whether or not to compute variance for each test case individually. Only compute the variance for a test case if there are at least `MIN_PREVIOUS_COMMITS` data points for it.
1 parent fb335b3 commit 8120c94

File tree

1 file changed

+23
-16
lines changed

1 file changed

+23
-16
lines changed

site/src/comparison.rs

Lines changed: 23 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -392,9 +392,7 @@ async fn compare_given_commits(
392392
benchmark: test_case.0,
393393
profile: test_case.1,
394394
scenario: test_case.2,
395-
variance: variances
396-
.as_ref()
397-
.and_then(|v| v.data.get(&test_case).cloned()),
395+
variance: variances.data.get(&test_case).cloned(),
398396
results: (a, b),
399397
calc_new_sig,
400398
})
@@ -589,41 +587,50 @@ impl BenchmarkVariances {
589587
from: ArtifactId,
590588
master_commits: &[collector::MasterCommit],
591589
stat: String,
592-
) -> Result<Option<Self>, BoxedError> {
590+
) -> Result<Self, BoxedError> {
591+
let mut variance_data = HashMap::new();
592+
593+
let previous_commits = Arc::new(previous_commits(
594+
from,
595+
Self::NUM_PREVIOUS_COMMITS,
596+
master_commits,
597+
));
598+
599+
// Return early if we don't have enough data to calculate variance.
600+
if previous_commits.len() < Self::MIN_PREVIOUS_COMMITS {
601+
return Ok(Self {
602+
data: variance_data,
603+
});
604+
}
605+
593606
// get all crates, cache, and profile combinations for the given stat
594607
let query = selector::Query::new()
595608
.set::<String>(Tag::Benchmark, selector::Selector::All)
596609
.set::<String>(Tag::Scenario, selector::Selector::All)
597610
.set::<String>(Tag::Profile, selector::Selector::All)
598611
.set(Tag::Metric, selector::Selector::One(stat));
599612

600-
let previous_commits = Arc::new(previous_commits(
601-
from,
602-
Self::NUM_PREVIOUS_COMMITS,
603-
master_commits,
604-
));
605613
let mut previous_commit_series = ctxt
606614
.statistic_series(query, previous_commits.clone())
607615
.await?;
608616

609-
let mut variance_data: HashMap<(Benchmark, Profile, Scenario), BenchmarkVariance> =
610-
HashMap::new();
611617
for _ in previous_commits.iter() {
612618
for (test_case, stat) in statistics_from_series(&mut previous_commit_series) {
613619
variance_data.entry(test_case).or_default().push(stat);
614620
}
615621
}
616-
if variance_data.len() < Self::MIN_PREVIOUS_COMMITS {
617-
return Ok(None);
618-
}
622+
623+
// Only retain test cases for which we have enough data to calculate variance.
624+
variance_data.retain(|_, v| v.data.len() >= Self::MIN_PREVIOUS_COMMITS);
619625

620626
for ((bench, _, _), results) in variance_data.iter_mut() {
621627
debug!("Calculating variance for: {}", bench);
622628
results.calculate_description();
623629
}
624-
Ok(Some(Self {
630+
631+
Ok(Self {
625632
data: variance_data,
626-
}))
633+
})
627634
}
628635
}
629636

0 commit comments

Comments
 (0)