Merge pull request #515 from nnethercote/invert-skip-self-profile

Invert --skip-self-profile

Author: Mark-Simulacrum

ci/check-benchmarks.sh

@@ -9,7 +9,6 @@ git clone https://github.com/rust-lang-nursery/rustc-timing.git;
 cargo build -p collector;
 RUST_BACKTRACE=1 RUST_LOG=collector=trace,rust_sysroot=debug \
     cargo run -p collector --bin collector -- \
-    --skip-self-profile \
     --output-repo rustc-timing \
     --exclude servo,cargo,crates.io,packed-simd,sentry-cli,tuple-stress test_benchmarks;
 code=$?

collector/README.md

@@ -94,6 +94,8 @@ depending on the speed of your machine.
 
 The following options, if present, must appear before `bench_local` in the
 command.
+- `--self-profile` can be used to do self-profiling, which requires the
+  `measureme` tool.
 - `--filter $STR` can be used to run a subset of the benchmarks. `$STR` is a
   substring of the name of the benchmark(s) you wish to run.
 - `--exclude $STR` is the inverse of `--filter`. `$STR` is a substring of the
@@ -196,7 +198,15 @@ might be optimized.
 
 ### Profiling local builds
 
-To profile local builds:
+To self-profile a local build:
+```
+RUST_LOG=info ./target/release/collector --output-repo $OUTPUT_DIR --self-profile \
+    bench_local $PROFILER --rustc $RUSTC --cargo $CARGO $ID
+```
+
+Then view the results the same way as for the `bench_local` subcommand.
+
+To profile a local build with a different profiler:
 ```
 RUST_LOG=info ./target/release/collector --output-repo $OUTPUT_DIR \
     profile $PROFILER --rustc $RUSTC --cargo $CARGO $ID

collector/src/bin/rustc-perf-collector/execute.rs

@@ -310,14 +310,12 @@ pub struct MeasureProcessor {
     base_incr_stats: (Stats, Option<SelfProfile>),
     clean_incr_stats: (Stats, Option<SelfProfile>),
     patched_incr_stats: Vec<(Patch, (Stats, Option<SelfProfile>))>,
-
-    collecting_self_profile: bool,
-
-    should_collect_self_profile: bool,
+    is_first_collection: bool,
+    self_profile: bool,
 }
 
 impl MeasureProcessor {
-    pub fn new(collect_self_profile: bool) -> Self {
+    pub fn new(self_profile: bool) -> Self {
         // Check we have `perf` available.
         let has_perf = Command::new("perf").output().is_ok();
         assert!(has_perf);
@@ -327,28 +325,28 @@ impl MeasureProcessor {
             base_incr_stats: (Stats::new(), None),
             clean_incr_stats: (Stats::new(), None),
             patched_incr_stats: Vec::new(),
-            collecting_self_profile: true,
+            is_first_collection: true,
             // Command::new("summarize").status().is_ok()
-            should_collect_self_profile: collect_self_profile,
+            self_profile,
         }
     }
 }
 
 impl Processor for MeasureProcessor {
     fn profiler(&self) -> Profiler {
-        if self.collecting_self_profile && self.should_collect_self_profile {
+        if self.is_first_collection && self.self_profile {
             Profiler::PerfStatSelfProfile
         } else {
             Profiler::PerfStat
         }
     }
 
     fn start_first_collection(&mut self) {
-        self.collecting_self_profile = true;
+        self.is_first_collection = true;
     }
 
     fn finished_first_collection(&mut self) -> bool {
-        self.collecting_self_profile = false;
+        self.is_first_collection = false;
         true
     }
 

collector/src/bin/rustc-perf-collector/main.rs

@@ -150,7 +150,7 @@ where
 fn process_commits(
     out_repo: outrepo::Repo,
     benchmarks: &[Benchmark],
-    collect_self_profile: bool,
+    self_profile: bool,
 ) -> anyhow::Result<()> {
     println!("processing commits");
     let client = reqwest::Client::new();
@@ -180,7 +180,7 @@ fn process_commits(
                 &benchmarks,
                 3,
                 true,
-                collect_self_profile,
+                self_profile,
             ));
             if let Err(err) = result {
                 panic!("failed to record success: {:?}", err);
@@ -254,7 +254,7 @@ fn bench_commit(
     benchmarks: &[Benchmark],
     iterations: usize,
     call_home: bool,
-    collect_self_profile: bool,
+    self_profile: bool,
 ) -> CommitData {
     info!(
         "benchmarking commit {} ({}) for triple {}",
@@ -285,11 +285,11 @@ fn bench_commit(
     }
 
     let has_measureme = Command::new("summarize").output().is_ok();
-    if collect_self_profile {
+    if self_profile {
         assert!(
             has_measureme,
             "needs `summarize` in PATH for self profile.\n\
-             Pass --skip-self-profile` to opt out"
+             Omit --self-profile` to opt out"
         );
     }
 
@@ -298,8 +298,7 @@ fn bench_commit(
             continue;
         }
 
-        // Only collect self-profile if we have summarize in the path
-        let mut processor = execute::MeasureProcessor::new(collect_self_profile);
+        let mut processor = execute::MeasureProcessor::new(self_profile);
         let result =
             benchmark.measure(&mut processor, build_kinds, run_kinds, compiler, iterations);
         let result = match result {
@@ -398,7 +397,7 @@ fn main_result() -> anyhow::Result<i32> {
        (@arg exclude: --exclude +takes_value "Ignore all benchmarks that contain this")
        (@arg sync_git: --("sync-git") "Synchronize repository with remote")
        (@arg output_repo: --("output-repo") +required +takes_value "Output repository/directory")
-       (@arg skip_self_profile: --("skip-self-profile") "Skip self-profile")
+       (@arg self_profile: --("self-profile") "Collect self-profile")
 
        (@subcommand bench_commit =>
            (about: "benchmark a bors merge from AWS")
@@ -456,7 +455,7 @@ fn main_result() -> anyhow::Result<i32> {
     let exclude = matches.value_of("exclude");
     let benchmarks = get_benchmarks(&benchmark_dir, filter, exclude)?;
     let use_remote = matches.is_present("sync_git");
-    let collect_self_profile = !matches.is_present("skip_self_profile");
+    let self_profile = matches.is_present("self_profile");
 
     let get_out_dir = || {
         let path = PathBuf::from(matches.value_of_os("output_repo").unwrap());
@@ -484,7 +483,7 @@ fn main_result() -> anyhow::Result<i32> {
                 &benchmarks,
                 3,
                 false,
-                collect_self_profile,
+                self_profile,
             ))?;
             Ok(0)
         }
@@ -524,7 +523,7 @@ fn main_result() -> anyhow::Result<i32> {
                 &benchmarks,
                 1,
                 false,
-                collect_self_profile,
+                self_profile,
             );
             get_out_repo(true)?.add_commit_data(&result)?;
             Ok(0)
@@ -537,7 +536,7 @@ fn main_result() -> anyhow::Result<i32> {
         }
 
         ("process", Some(_)) => {
-            process_commits(get_out_repo(false)?, &benchmarks, collect_self_profile)?;
+            process_commits(get_out_repo(false)?, &benchmarks, self_profile)?;
             Ok(0)
         }
 
@@ -622,7 +621,7 @@ fn main_result() -> anyhow::Result<i32> {
                     &benchmarks,
                     1,
                     false,
-                    collect_self_profile,
+                    self_profile,
                 );
             } else {
                 panic!("no commits");