Skip to content
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

Utilize PGO for rustc linux dist builds #80262

Merged
merged 1 commit into from
Dec 23, 2020

Conversation

Mark-Simulacrum
Copy link
Member

This implements support for applying PGO to the rustc compilation step (not
standard library or any tooling, including rustdoc). Expanding PGO to more tools
is not terribly difficult but will involve more work and greater CI time
commitment.

For the same reason of avoiding greater implementation time commitment,
implementing for platforms outside of x86_64-unknown-linux-gnu is skipped.
In practice it should be quite simple to extend over time to more platforms. The
initial implementation is intentionally minimal here to avoid too much work
investment before we start seeing wins for a subset of Rust users.

The choice of workloads to profile here is somewhat arbitrary, but the general
rationale was to aim for a small set that largely avoided time regressions on
perf.rust-lang.org's full suite of crates. The set chosen is libcore, cargo (and
its dependencies), and a few ad-hoc stress tests from perf.rlo. The stress tests
are arguably the most controversial, but they benefit those cases (avoiding
regressions) and do not really remove wins from other benchmarks.

The primary next step after this PR lands is to implement support for PGO in
LLVM. It is unclear whether we can afford a full LLVM rebuild in CI, though, so
the approach taken there may need to be more staggered. rustc-only PGO seems
well affordable on linux at least, giving us up to 20% wall time wins on some
crates for 15 minutes of extra CI time (1 hour with this PR, up from 45 minutes).

The PGO data is uploaded to allow others to reuse it if attempting to reproduce
the CI build or potentially, in the future, on other platforms where an
off-by-one strategy is used for dist builds at minimal performance cost.

r? @michaelwoerister (but tell me if you don't want to / don't feel comfortable approving and we can find others)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 21, 2020
@camelid camelid added A-meta Area: Issues & PRs about the rust-lang/rust repository itself T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Dec 22, 2020
@michaelwoerister
Copy link
Member

Nice! I'll review asap.

cargo.rustflag(&format!("-Cprofile-use={}", path));
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll probably also need the following LLVM flags (in both the -generate and the -use case) so that we don't end up with absolute file system paths in profile data:

michaelwoerister@d5413a6#diff-ca6526d91d41c31fad645c46417e0f1ce61e324542994e8c8b72b1f7a08c91bdR898-R899

@michaelwoerister
Copy link
Member

The PGO parts look good to me (with the comment about absolute paths adressed). Since I don't know too much about the dist/infrastructure part and this affecting one of the most critical dist builds, maybe someone else can give the final go-ahead.

Implementing the LLVM part is blocked on sccache supporting cached -fprofile-use invocations. But then there should not be much additional complexity for PGOing LLVM too.

@Mark-Simulacrum
Copy link
Member Author

Since we added the LLVM arg to strip things want to confirm that didn't accidentally hurt our performance: @bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Contributor

bors commented Dec 22, 2020

⌛ Trying commit 3f1b83ed51fd6a8dd08d5aa13f57f76afb70c029 with merge 0c9f42674071faf0a9d86cf01ca48f294eb0797c...

Copy link
Member

@pietroalbini pietroalbini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a review for the CI bits.

src/ci/docker/host-x86_64/dist-x86_64-linux/Dockerfile Outdated Show resolved Hide resolved
src/ci/docker/host-x86_64/dist-x86_64-linux/pgo.sh Outdated Show resolved Hide resolved
src/ci/docker/host-x86_64/dist-x86_64-linux/pgo.sh Outdated Show resolved Hide resolved
src/bootstrap/dist.rs Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Dec 22, 2020

☀️ Try build successful - checks-actions
Build commit: 0c9f42674071faf0a9d86cf01ca48f294eb0797c (0c9f42674071faf0a9d86cf01ca48f294eb0797c)

@rust-timer
Copy link
Collaborator

Queued 0c9f42674071faf0a9d86cf01ca48f294eb0797c with parent 75e1acb, future comparison URL.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 22, 2020
@Mark-Simulacrum
Copy link
Member Author

r? @pietroalbini - I think I resolved all of your comments

@pietroalbini
Copy link
Member

Looks great! The only thing I'd change is moving the PGO script from src/ci/scripts/pgo.sh ot src/ci/pgo.sh: the scripts directory currently contains only the scripts that initialize CI builders.

r=me after that change

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
Checking which error codes lack tests...
Found 434 error codes
Found 0 error codes with no tests
Done!
tidy error: /checkout/src/ci/scripts/pgo.sh:16: line longer than 100 chars
some tidy checks failed

command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "/checkout/obj/build"
expected success, got: exit code: 1

This implements support for applying PGO to the rustc compilation step (not
standard library or any tooling, including rustdoc). Expanding PGO to more tools
is not terribly difficult but will involve more work and greater CI time
commitment.

For the same reason of avoiding greater time commitment, this currently avoids
implementing for platforms outside of x86_64-unknown-linux-gnu, though in
practice it should be quite simple to extend over time to more platforms. The
initial implementation is intentionally minimal here to avoid too much work
investment before we start seeing wins for a subset of Rust users.

The choice of workloads to profile here is somewhat arbitrary, but the general
rationale was to aim for a small set that largely avoided time regressions on
perf.rust-lang.org's full suite of crates. The set chosen is libcore, cargo (and
its dependencies), and a few ad-hoc stress tests from perf.rlo. The stress tests
are arguably the most controversial, but they benefit those cases (avoiding
regressions) and do not really remove wins from other benchmarks.

The primary next step after this PR lands is to implement support for PGO in
LLVM. It is unclear whether we can afford a full LLVM rebuild in CI, though, so
the approach taken there may need to be more staggered. rustc-only PGO seems
well affordable on linux at least, giving us up to 20% wall time wins on some
crates for 15 minutes of extra CI time (1 hour up from 45 minutes).

The PGO data is uploaded to allow others to reuse it if attempting to reproduce
the CI build or potentially, in the future, on other platforms where an
off-by-one strategy is used for dist builds at minimal performance cost.
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (0c9f42674071faf0a9d86cf01ca48f294eb0797c): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 22, 2020
@Mark-Simulacrum
Copy link
Member Author

@bors r=pietroalbini

@bors
Copy link
Contributor

bors commented Dec 22, 2020

📌 Commit a448f88 has been approved by pietroalbini

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 22, 2020
@bors
Copy link
Contributor

bors commented Dec 23, 2020

⌛ Testing commit a448f88 with merge f91eb622f1f1eec6370afd0a6a7a67a0fef8d735...

@rust-log-analyzer
Copy link
Collaborator

The job i686-msvc-1 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
[TIMING] StdLink { compiler: Compiler { stage: 0, host: TargetSelection { triple: "i686-pc-windows-msvc", file: None } }, target_compiler: Compiler { stage: 0, host: TargetSelection { triple: "i686-pc-windows-msvc", file: None } }, target: TargetSelection { triple: "i686-pc-windows-msvc", file: None } } -- 0.006
[TIMING] Std { target: TargetSelection { triple: "i686-pc-windows-msvc", file: None }, compiler: Compiler { stage: 0, host: TargetSelection { triple: "i686-pc-windows-msvc", file: None } } } -- 101.621
Building LLVM for i686-pc-windows-msvc
running: "cmake" "D:\\a\\rust\\rust\\src/llvm-project/llvm" "-G" "Ninja" "-DLLVM_ENABLE_ASSERTIONS=ON" "-DLLVM_TARGETS_TO_BUILD=AArch64;ARM;Hexagon;MSP430;Mips;NVPTX;PowerPC;RISCV;Sparc;SystemZ;WebAssembly;X86" "-DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD=AVR" "-DLLVM_INCLUDE_EXAMPLES=OFF" "-DLLVM_INCLUDE_TESTS=OFF" "-DLLVM_INCLUDE_DOCS=OFF" "-DLLVM_INCLUDE_BENCHMARKS=OFF" "-DLLVM_ENABLE_TERMINFO=OFF" "-DLLVM_ENABLE_LIBEDIT=OFF" "-DLLVM_ENABLE_BINDINGS=OFF" "-DLLVM_ENABLE_Z3_SOLVER=OFF" "-DLLVM_PARALLEL_COMPILE_JOBS=8" "-DLLVM_TARGET_ARCH=i686" "-DLLVM_DEFAULT_TARGET_TRIPLE=i686-pc-windows-msvc" "-DLLVM_ENABLE_ZLIB=ON" "-DLLVM_USE_CRT_DEBUG=MT" "-DLLVM_USE_CRT_RELEASE=MT" "-DLLVM_USE_CRT_RELWITHDEBINFO=MT" "-DLLVM_BUILD_32_BITS=ON" "-DLLVM_ENABLE_LIBXML2=OFF" "-DLLVM_VERSION_SUFFIX=-rust-1.50.0-nightly" "-DCMAKE_INSTALL_MESSAGE=LAZY" "-DCMAKE_C_COMPILER=D:/a/rust/rust/build/bootstrap/debug/sccache-plus-cl.exe" "-DCMAKE_CXX_COMPILER=D:/a/rust/rust/build/bootstrap/debug/sccache-plus-cl.exe" "-DCMAKE_C_FLAGS=-nologo -MT -Brepro --target=i686-pc-windows-msvc" "-DCMAKE_CXX_FLAGS=-nologo -MT -Brepro --target=i686-pc-windows-msvc" "-DCMAKE_INSTALL_PREFIX=D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\llvm" "-DCMAKE_ASM_FLAGS= -nologo -MT -Brepro" "-DCMAKE_ASM_COMPILER=C:/Program Files (x86)/Microsoft Visual Studio/2019/Enterprise/VC/Tools/MSVC/14.28.29333/bin/HostX64/x86/cl.exe" "-DCMAKE_BUILD_TYPE=Release"
CMake Error: The source directory "D:/a/rust/rust/src/llvm-project/llvm" does not exist.
Specify --help for usage, or press the help button on the CMake GUI.
command did not execute successfully, got: exit code: 1


build script failed, must exit now', C:\Users\runneradmin\.cargo\registry\src\github.com-1285ae84e5963aae\cmake-0.1.44\src\lib.rs:885:5
 finished in 0.088 seconds
failed to run: D:\a\rust\rust\build\bootstrap\debug\bootstrap test --stage 2 --exclude src/test/ui --exclude src/test/compile-fail --exclude src/tools/linkchecker
Build completed unsuccessfully in 0:03:40
Build completed unsuccessfully in 0:03:40
make: *** [Makefile:73: ci-subset-1] Error 1

@bors
Copy link
Contributor

bors commented Dec 23, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 23, 2020
@pietroalbini
Copy link
Member

@bors retry network failure

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 23, 2020
@bors
Copy link
Contributor

bors commented Dec 23, 2020

⌛ Testing commit a448f88 with merge 3ffea60...

@bors
Copy link
Contributor

bors commented Dec 23, 2020

☀️ Test successful - checks-actions
Approved by: pietroalbini
Pushing 3ffea60 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 23, 2020
@bors bors merged commit 3ffea60 into rust-lang:master Dec 23, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 23, 2020
@Mark-Simulacrum Mark-Simulacrum deleted the pgo-rustc branch March 1, 2021 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: Issues & PRs about the rust-lang/rust repository itself merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants