Skip to content

Refactor site to make it easier to introduce runtime benchmarks #1608

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 6 commits into from
Jun 5, 2023
Merged

Refactor site to make it easier to introduce runtime benchmarks #1608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8d9b7f7
Refactor compare API data to allow easier compile/runtime separation
Kobzol Jun 3, 2023
a2490fe
Remove unused code
Kobzol Jun 3, 2023
3ad5bf1
Apply clippy fixes
Kobzol Jun 3, 2023
77c70db
Simplify compile time benchmark querying
Kobzol Jun 3, 2023
b6e7004
Simplify compile time benchmark query result gathering
Kobzol Jun 3, 2023
58920cb
Fix all remaining Clippy issues
Kobzol Jun 5, 2023
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
8 changes: 4 additions & 4 deletions collector/benchlib/src/benchmark.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,12 +110,12 @@ impl BenchmarkGroup {
pub fn passes_filter(name: &str, exclude: Option<&str>, include: Option<&str>) -> bool {
match (exclude, include) {
(Some(exclude), Some(include)) => {
let included = include.split(",").any(|filter| name.starts_with(filter));
let excluded = exclude.split(",").any(|filter| name.starts_with(filter));
let included = include.split(',').any(|filter| name.starts_with(filter));
let excluded = exclude.split(',').any(|filter| name.starts_with(filter));
included && !excluded
}
(None, Some(include)) => include.split(",").any(|filter| name.starts_with(filter)),
(Some(exclude), None) => !exclude.split(",").any(|filter| name.starts_with(filter)),
(None, Some(include)) => include.split(',').any(|filter| name.starts_with(filter)),
(Some(exclude), None) => !exclude.split(',').any(|filter| name.starts_with(filter)),
(None, None) => true,
}
}
Expand Down
8 changes: 4 additions & 4 deletions collector/src/benchmark/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ impl Benchmark {
}
}

let mut patches: Vec<_> = patches.into_iter().map(|p| Patch::new(p)).collect();
let mut patches: Vec<_> = patches.into_iter().map(Patch::new).collect();
patches.sort_by_key(|p| p.index);

let config_path = path.join("perf-config.json");
Expand Down Expand Up @@ -205,7 +205,7 @@ impl Benchmark {
let iterations = iterations.unwrap_or(self.config.runs);

let profiles: Vec<Profile> = profiles
.into_iter()
.iter()
.copied()
.filter(|profile| !self.config.excluded_profiles.contains(profile))
.collect();
Expand All @@ -216,7 +216,7 @@ impl Benchmark {
}

let scenarios: Vec<Scenario> = scenarios
.into_iter()
.iter()
.copied()
.filter(|scenario| !self.config.excluded_scenarios.contains(scenario))
.collect();
Expand Down Expand Up @@ -346,7 +346,7 @@ impl Benchmark {
processor,
Scenario::IncrPatched,
&scenario_str,
Some(&patch),
Some(patch),
)
.run_rustc(true)?;
}
Expand Down
4 changes: 2 additions & 2 deletions collector/src/benchmark/patch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ impl Patch {
assert!(path.is_file());
let (index, name) = {
let file_name = path.file_name().unwrap().to_string_lossy();
let mut parts = file_name.split("-");
let mut parts = file_name.split('-');
let index = parts.next().unwrap().parse().unwrap_or_else(|e| {
panic!(
"{:?} should be in the format 000-name.patch, \
Expand Down Expand Up @@ -61,7 +61,7 @@ impl Patch {
log::debug!("applying {} to {:?}", self.name, dir);

let mut cmd = Command::new("git");
cmd.current_dir(dir).args(&["apply"]).arg(&*self.path);
cmd.current_dir(dir).args(["apply"]).arg(&*self.path);

command_output(&mut cmd)?;

Expand Down
4 changes: 2 additions & 2 deletions collector/src/bin/bootstrap-rustc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ fn run() -> i32 {
let status = cmd.status().expect("spawned");

if status.success() {
return 0;
0
} else {
return 1;
1
}
}

Expand Down
95 changes: 51 additions & 44 deletions collector/src/bin/collector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use collector::benchmark::{
use collector::{runtime, utils, CollectorCtx, CollectorStepBuilder};
use database::{ArtifactId, Commit, CommitType, Connection, Pool};
use rayon::iter::{IndexedParallelIterator, IntoParallelRefIterator, ParallelIterator};
use std::cmp::Ordering;
use std::ffi::OsStr;
use std::fs;
use std::fs::File;
Expand Down Expand Up @@ -57,6 +58,7 @@ impl BenchmarkErrors {
}
}

#[allow(clippy::too_many_arguments)]
fn bench(
rt: &mut Runtime,
mut conn: Box<dyn Connection>,
Expand Down Expand Up @@ -326,6 +328,7 @@ fn cg_annotate(cgout: &Path, path: &Path) -> anyhow::Result<()> {
Ok(())
}

#[allow(clippy::too_many_arguments)]
fn profile(
compiler: Compiler,
id: &str,
Expand All @@ -348,8 +351,7 @@ fn profile(
let benchmark_id = format!("{} ({}/{})", benchmark.name, i + 1, benchmarks.len());
eprintln!("Executing benchmark {benchmark_id}");
let mut processor = ProfileProcessor::new(profiler, out_dir, id);
let result =
benchmark.measure(&mut processor, &profiles, &scenarios, compiler, Some(1));
let result = benchmark.measure(&mut processor, profiles, scenarios, compiler, Some(1));
eprintln!("Finished benchmark {benchmark_id}");

if let Err(ref s) = result {
Expand Down Expand Up @@ -400,7 +402,7 @@ impl TypedValueParser for ProfileArgParser {
let profiles: Result<Vec<Profile>, _> = value
.to_str()
.unwrap()
.split(",")
.split(',')
.map(|item| clap::value_parser!(Profile).parse_ref(cmd, arg, OsStr::new(item)))
.collect();

Expand All @@ -410,7 +412,7 @@ impl TypedValueParser for ProfileArgParser {

fn possible_values(&self) -> Option<Box<dyn Iterator<Item = PossibleValue> + '_>> {
let values = Profile::value_variants()
.into_iter()
.iter()
.filter_map(|item| item.to_possible_value())
.chain([PossibleValue::new("All")]);
Some(Box::new(values))
Expand Down Expand Up @@ -438,7 +440,7 @@ impl TypedValueParser for ScenarioArgParser {
let scenarios: Result<Vec<Scenario>, _> = value
.to_str()
.unwrap()
.split(",")
.split(',')
.map(|item| clap::value_parser!(Scenario).parse_ref(cmd, arg, OsStr::new(item)))
.collect();

Expand All @@ -448,7 +450,7 @@ impl TypedValueParser for ScenarioArgParser {

fn possible_values(&self) -> Option<Box<dyn Iterator<Item = PossibleValue> + '_>> {
let values = Scenario::value_variants()
.into_iter()
.iter()
.filter_map(|item| item.to_possible_value())
.chain([PossibleValue::new("All")]);
Some(Box::new(values))
Expand Down Expand Up @@ -711,7 +713,7 @@ fn main_result() -> anyhow::Result<i32> {
let pool = Pool::open(&db.db);

let suite = runtime::discover_benchmarks(&toolchain, &runtime_benchmark_dir)?;
let artifact_id = ArtifactId::Tag(toolchain.id.clone());
let artifact_id = ArtifactId::Tag(toolchain.id);
let (conn, collector) = rt.block_on(async {
let mut conn = pool.connection().await;
let collector = CollectorStepBuilder::default()
Expand Down Expand Up @@ -744,7 +746,7 @@ fn main_result() -> anyhow::Result<i32> {
let pool = database::Pool::open(&db.db);

let toolchain = get_local_toolchain(
&profiles,
profiles,
&local.rustc,
opts.rustdoc.as_deref(),
local.cargo.as_deref(),
Expand All @@ -771,8 +773,8 @@ fn main_result() -> anyhow::Result<i32> {
let res = bench(
&mut rt,
conn,
&profiles,
&scenarios,
profiles,
scenarios,
Compiler::from_toolchain(&toolchain, &target_triple),
&benchmarks,
Some(iterations),
Expand All @@ -792,7 +794,7 @@ fn main_result() -> anyhow::Result<i32> {
println!("processing artifacts");
let client = reqwest::blocking::Client::new();
let response: collector::api::next_artifact::Response = client
.get(&format!("{}/perf/next_artifact", site_url))
.get(format!("{}/perf/next_artifact", site_url))
.send()?
.json()?;
let next = if let Some(c) = response.artifact {
Expand All @@ -815,7 +817,7 @@ fn main_result() -> anyhow::Result<i32> {
&compile_benchmark_dir,
)?;

client.post(&format!("{}/perf/onpush", site_url)).send()?;
client.post(format!("{}/perf/onpush", site_url)).send()?;
}
NextArtifact::Commit {
commit,
Expand Down Expand Up @@ -853,7 +855,7 @@ fn main_result() -> anyhow::Result<i32> {
collector,
);

client.post(&format!("{}/perf/onpush", site_url)).send()?;
client.post(format!("{}/perf/onpush", site_url)).send()?;

res.fail_if_nonzero()?;
}
Expand Down Expand Up @@ -912,8 +914,8 @@ fn main_result() -> anyhow::Result<i32> {
let mut get_toolchain_and_profile =
|rustc: &str, suffix: &str| -> anyhow::Result<String> {
let toolchain = get_local_toolchain(
&profiles,
&rustc,
profiles,
rustc,
opts.rustdoc.as_deref(),
local.cargo.as_deref(),
local.id.as_deref(),
Expand All @@ -926,8 +928,8 @@ fn main_result() -> anyhow::Result<i32> {
profiler,
&out_dir,
&benchmarks,
&profiles,
&scenarios,
profiles,
scenarios,
&mut errors,
);
Ok(toolchain.id)
Expand All @@ -950,20 +952,23 @@ fn main_result() -> anyhow::Result<i32> {
&id2,
&out_dir,
&benchmarks,
&profiles,
&scenarios,
profiles,
scenarios,
&mut errors,
);
if diffs.len() > 1 {
eprintln!("Diffs:");
for diff in diffs {
eprintln!("{}", diff.to_string_lossy());
match diffs.len().cmp(&1) {
Ordering::Equal => {
let short = out_dir.join("cgann-diff-latest");
std::fs::copy(&diffs[0], &short).expect("copy to short path");
eprintln!("Original diff at: {}", diffs[0].to_string_lossy());
eprintln!("Short path: {}", short.to_string_lossy());
}
_ => {
eprintln!("Diffs:");
for diff in diffs {
eprintln!("{}", diff.to_string_lossy());
}
}
} else if diffs.len() == 1 {
let short = out_dir.join("cgann-diff-latest");
std::fs::copy(&diffs[0], &short).expect("copy to short path");
eprintln!("Original diff at: {}", diffs[0].to_string_lossy());
eprintln!("Short path: {}", short.to_string_lossy());
}
}
} else {
Expand All @@ -983,8 +988,8 @@ fn main_result() -> anyhow::Result<i32> {
.unwrap();
let last_sha = String::from_utf8(last_sha.stdout).expect("utf8");
let last_sha = last_sha.split_whitespace().next().expect(&last_sha);
let commit = get_commit_or_fake_it(&last_sha).expect("success");
let mut sysroot = Sysroot::install(commit.sha.to_string(), &target_triple)?;
let commit = get_commit_or_fake_it(last_sha).expect("success");
let mut sysroot = Sysroot::install(commit.sha, &target_triple)?;
sysroot.preserve(); // don't delete it

// Print the directory containing the toolchain.
Expand Down Expand Up @@ -1021,7 +1026,7 @@ async fn init_compile_collector(
) -> (Box<dyn Connection>, CollectorCtx) {
let mut conn = pool.connection().await;
let collector = CollectorStepBuilder::default()
.record_compile_benchmarks(&benchmarks, bench_rustc)
.record_compile_benchmarks(benchmarks, bench_rustc)
.start_collection(conn.as_mut(), artifact_id)
.await;
(conn, collector)
Expand All @@ -1035,7 +1040,7 @@ fn bench_published_artifact(
benchmark_dir: &Path,
) -> anyhow::Result<()> {
let status = Command::new("rustup")
.args(&["install", "--profile=minimal", &toolchain])
.args(["install", "--profile=minimal", &toolchain])
.status()
.context("rustup install")?;
if !status.success() {
Expand Down Expand Up @@ -1071,7 +1076,7 @@ fn bench_published_artifact(
let cargo = which("cargo")?;

// Exclude benchmarks that don't work with a stable compiler.
let mut benchmarks = get_compile_benchmarks(&benchmark_dir, None, None, None)?;
let mut benchmarks = get_compile_benchmarks(benchmark_dir, None, None, None)?;
benchmarks.retain(|b| b.category().is_stable());

let artifact_id = ArtifactId::Tag(toolchain);
Expand All @@ -1091,7 +1096,7 @@ fn bench_published_artifact(
rustdoc: Some(Path::new(rustdoc.trim())),
cargo: Path::new(cargo.trim()),
is_nightly: false,
triple: &target_triple,
triple: target_triple,
},
&benchmarks,
Some(3),
Expand All @@ -1102,7 +1107,7 @@ fn bench_published_artifact(
Ok(())
}

fn add_perf_config(directory: &PathBuf, category: Category) {
fn add_perf_config(directory: &Path, category: Category) {
let data = serde_json::json!({
"category": category.to_string()
});
Expand All @@ -1118,10 +1123,12 @@ fn add_perf_config(directory: &PathBuf, category: Category) {
fn check_target_dir(target_dir: &Path, force: bool) -> anyhow::Result<()> {
if target_dir.exists() {
if force {
std::fs::remove_dir_all(&target_dir).expect(&format!(
"Cannot remove previous directory at {}",
target_dir.display()
));
std::fs::remove_dir_all(target_dir).unwrap_or_else(|_| {
panic!(
"Cannot remove previous directory at {}",
target_dir.display()
)
});
} else {
return Err(anyhow::anyhow!(
"Directory {} already exists",
Expand All @@ -1138,9 +1145,9 @@ fn get_downloaded_crate_target(benchmark_dir: &Path, cmd: &DownloadCommand) -> P
// URLs in general can end with /, which we also want to remove to make sure that the
// last part of the URL is the repository name.
DownloadSubcommand::Git { url } => url
.trim_end_matches("/")
.trim_end_matches('/')
.trim_end_matches(".git")
.split("/")
.split('/')
.last()
.expect("Crate name could not be determined from git URL")
.to_string(),
Expand Down Expand Up @@ -1168,7 +1175,7 @@ fn download_from_git(target: &Path, url: &str) -> anyhow::Result<()> {
log::error!("Could not delete .git directory: {error:?}");
}

utils::fs::rename(&tmpdir, &target)?;
utils::fs::rename(&tmpdir, target)?;
Ok(())
}

Expand Down Expand Up @@ -1198,7 +1205,7 @@ fn download_from_crates_io(target_dir: &Path, krate: &str, version: &str) -> any
// under <crate-name>-<version> directory.
let unpacked_dir = tmpdir.path().join(format!("{krate}-{version}"));
generate_lockfile(&unpacked_dir);
utils::fs::rename(&unpacked_dir, &target_dir)?;
utils::fs::rename(&unpacked_dir, target_dir)?;

Ok(())
}
Expand Down Expand Up @@ -1235,7 +1242,7 @@ fn get_commit_or_fake_it(sha: &str) -> anyhow::Result<Commit> {
log::warn!("utilizing fake commit!");
Commit {
sha: sha.into(),
date: database::Date::ymd_hms(2000, 01, 01, 0, 0, 0),
date: database::Date::ymd_hms(2000, 1, 1, 0, 0, 0),
r#type: CommitType::Master,
}
}))
Expand Down
Loading