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.

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

Fix the 'bench_local' subcommand. #207

Merged
merged 1 commit into from Apr 7, 2018

Conversation

Projects
None yet
2 participants
@nnethercote
Contributor

nnethercote commented Apr 6, 2018

Because it is currently pretty much unusable.

This patch changes its invocation to this:

collector bench_local <ID> --cargo <CARGO> --rustc <RUSTC>

<ID> is an identifier used in the output file's name and contents, in the same
places that the commit SHA is used when benchmarking a particular commit.
<CARGO> and <RUSTC> are mandatory options specifying local versions of Cargo
and rustc.

Output is now produced in much the same way as the other subcommands,
with the exception that the directory named by --output-repo doesn't
have to be an actual rustc-timings repo; any directory will do. site
can be used to view the data in the output directory.

Details of note:

  • The patch fixes up the README.md and the usage docs a bit.

  • Sysroot is no longer used in bench_commit() or execute.rs. Instead we pass
    rustc_path, cargo_path, and (for bench_commit() only) triple, because
    that's all that is needed.

  • Sysroot::command() is no longer used by CargoProcess::base_command().
    It was only contributing a tiny bit of functionality (e.g. the CARGO
    env var), which CargoProcess::base_command() can do itself.

  • Repo::open() gains an additional parameter that tells it not to complain if
    the 'retries' file is missing; this is necessary now that ordinary
    directories can be used for output by 'bench_local'.

@Mark-Simulacrum

These changes look great! Thanks for making them. Other than the few nits and comments, I think this is ready to land.

placed.
`$RUSTC` should point to a rustc executable. For benchmarking purposes a stage
2 build is a bit better than a stage 1 build, but not critical.

This comment has been minimized.

@Mark-Simulacrum

Mark-Simulacrum Apr 6, 2018

Collaborator

We might want to mention that not all benchmarks will compile with a stage 1 build as they might need proc macros which won't work today at stage 1.

This comment has been minimized.

@nnethercote

nnethercote Apr 6, 2018

Contributor

Ok.

`$RUSTC` should point to a rustc executable. For benchmarking purposes a stage
2 build is a bit better than a stage 1 build, but not critical.
`$CARGO` should point to a Cargo executable.

This comment has been minimized.

@Mark-Simulacrum

Mark-Simulacrum Apr 6, 2018

Collaborator

I think it's worth clarifying whether we expect these to be relative or absolute paths here.

This comment has been minimized.

@nnethercote

nnethercote Apr 6, 2018

Contributor

Ok.

struct CargoProcess<'sysroot> {
sysroot: &'sysroot Sysroot,
struct CargoProcess<'a> {
rustc_path: &'a PathBuf,

This comment has been minimized.

@Mark-Simulacrum

Mark-Simulacrum Apr 6, 2018

Collaborator

hm, these should probably be &'a Path instead.

This comment has been minimized.

@nnethercote

nnethercote Apr 6, 2018

Contributor

I've done that, and likewise for a bunch of the rustc_path/cargo_path function parameters. It was much easier than I expected... there must be some kind of automatic PathBuf to Path coercions happening.

cmd
.env("SHELL", env::var_os("SHELL").unwrap_or_default())

This comment has been minimized.

@Mark-Simulacrum

Mark-Simulacrum Apr 6, 2018

Collaborator

I've had problems with some benchmarks in the past if the SHELL environment variable isn't passed down, but if this works, I'm fine with landing it...

This comment has been minimized.

@nnethercote

nnethercote Apr 6, 2018

Contributor

To be fair, I've only tested helloworld and parser! I will run all the benchmarks to see if SHELL or anything else is needed.

@@ -240,7 +244,7 @@ fn main_result() -> Result<i32, Error> {
)?;
let use_remote = matches.is_present("sync_git");
let out_repo = PathBuf::from(matches.value_of_os("output_repo").unwrap());
let mut out_repo = outrepo::Repo::open(out_repo, use_remote)?;
let mut out_repo = outrepo::Repo::open(out_repo, true, use_remote)?;

This comment has been minimized.

@Mark-Simulacrum

Mark-Simulacrum Apr 6, 2018

Collaborator

Could we condition the true being passed here to be only if we're running locally? I'd like to avoid accidentally clearing out some git directory on the perf collector...

This comment has been minimized.

@nnethercote

nnethercote Apr 6, 2018

Contributor

Sure.

Fix the 'bench_local' subcommand.
Because it is currently pretty much unusable.

This patch changes its invocation to this:

  collector bench_local <ID> --cargo <CARGO> --rustc <RUSTC>

<ID> is an identifier used in the output file's name and contents, in the same
places that the commit SHA is used when benchmarking a particular commit.
<CARGO> and <RUSTC> are mandatory options specifying local versions of Cargo
and rustc.

Output is now produced in much the same way as the other subcommands,
with the exception that the directory named by --output-repo doesn't
have to be an actual rustc-timings repo; any directory will do. `site`
can be used to view the data in the output directory.

Details of note:

- The patch fixes up the README.md and the usage docs a bit.

- Sysroot is no longer used in bench_commit() or execute.rs. Instead we pass
  `rustc_path`, `cargo_path`, and (for bench_commit() only) `triple`, because
  that's all that is needed.

- Sysroot::command() is no longer used by CargoProcess::base_command().
  It was only contributing a small amount of functionality (e.g. the
  PATH and CARGO env vars), which CargoProcess::base_command() can do
  itself.

- Repo::open() gains an additional parameter that tells it not to complain if
  the 'retries' file is missing; this is necessary now that ordinary
  directories can be used for output by 'bench_local'.
@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Apr 7, 2018

@Mark-Simulacrum: the new version fixes all the nits. I have tested on all the benchmarks now. You were right, PATH was needed for several, and a stage 2 compiler for several.

@Mark-Simulacrum Mark-Simulacrum merged commit 8cb770e into rust-lang-nursery:master Apr 7, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@nnethercote nnethercote deleted the nnethercote:redo-bench_local branch Apr 12, 2018

nnethercote added a commit to nnethercote/rustc-perf that referenced this pull request Apr 18, 2018

Reinstate the SHELL env var in Cargo invocations.
PR rust-lang-nursery#207 erroneously removed this, which broke the building of the
script-servo benchmark.

nnethercote added a commit to nnethercote/rustc-perf that referenced this pull request Apr 18, 2018

Reinstate the SHELL env var in Cargo invocations.
PR rust-lang-nursery#207 erroneously removed this, which broke the building of the
script-servo benchmark.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment