Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Fix the 'bench_local' subcommand. #207
Conversation
|
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. |
Mark-Simulacrum
Apr 6, 2018
Member
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.
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.
nnethercote
Apr 6, 2018
Author
Collaborator
Ok.
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. |
Mark-Simulacrum
Apr 6, 2018
Member
I think it's worth clarifying whether we expect these to be relative or absolute paths here.
I think it's worth clarifying whether we expect these to be relative or absolute paths here.
nnethercote
Apr 6, 2018
Author
Collaborator
Ok.
Ok.
| struct CargoProcess<'sysroot> { | ||
| sysroot: &'sysroot Sysroot, | ||
| struct CargoProcess<'a> { | ||
| rustc_path: &'a PathBuf, |
Mark-Simulacrum
Apr 6, 2018
Member
hm, these should probably be &'a Path instead.
hm, these should probably be &'a Path instead.
nnethercote
Apr 6, 2018
Author
Collaborator
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.
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()) |
Mark-Simulacrum
Apr 6, 2018
Member
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...
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...
nnethercote
Apr 6, 2018
Author
Collaborator
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.
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)?; | |||
Mark-Simulacrum
Apr 6, 2018
Member
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...
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...
nnethercote
Apr 6, 2018
Author
Collaborator
Sure.
Sure.
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
force-pushed the
nnethercote:redo-bench_local
branch
from
2138671
to
a046183
|
@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
merged commit 8cb770e
into
rust-lang:master
PR rust-lang#207 erroneously removed this, which broke the building of the script-servo benchmark.
PR rust-lang#207 erroneously removed this, which broke the building of the script-servo benchmark.
Because it is currently pretty much unusable.
This patch changes its invocation to this:
<ID>is an identifier used in the output file's name and contents, in the sameplaces that the commit SHA is used when benchmarking a particular commit.
<CARGO>and<RUSTC>are mandatory options specifying local versions of Cargoand 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.
sitecan 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, becausethat'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'.