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

Regression in compile times after LLVM was upgraded #31435

Closed
alexcrichton opened this issue Feb 5, 2016 · 14 comments
Closed

Regression in compile times after LLVM was upgraded #31435

alexcrichton opened this issue Feb 5, 2016 · 14 comments
Labels
I-compiletime Issue: Problems and improvements with respect to compile times. P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

LLVM was recently upgraded in #30448 which appears to have caused a regression
in compile times. First up, let's talk numbers. The compiler revisions in
question are:

  • 074f49a - just before the LLVM upgrade
  • 303892e - just after the LLVM upgrade

The nightly-2016-01-30 toolchain conveniently corresponds exactly to
303892e. The benchmarks here will all be
compiling libsyntax at these two revision in optimized mode. Note that
libsyntax did not change at all between these two revisions. For local
compilations I configured with:

./configure --enable-rpath --disable-optimize-tests --enable-ccache

and the C++ compiler version is:

$ g++ --version
g++ (Ubuntu 4.8.4-2ubuntu1~14.04) 4.8.4
Copyright (C) 2013 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Nightly compiler (nightly-2016-01-30)

$ multirust run nightly-2016-01-30 rustc -vV
rustc 1.8.0-nightly (303892ee1 2016-01-30)
$ time multirust run nightly-2016-01-30 rustc src/libsyntax/lib.rs -O > out
multirust run nightly-2016-01-30 rustc src/libsyntax/lib.rs -O -Z time-passes  129.85s user 1.31s system 99% cpu 2:11.18 total

Summary: 2:11.18s compile time, full output of time-passes

Locally compiled after LLVM upgrade (303892e)

$ ./x86_64-unknown-linux-gnu/stage2/bin/rustc -V
rustc 1.8.0-dev (303892ee1 2016-01-30)
$ time ./x86_64-unknown-linux-gnu/stage2/bin/rustc src/libsyntax/lib.rs -O -Z time-passes > out
./x86_64-unknown-linux-gnu/stage2/bin/rustc src/libsyntax/lib.rs -O -Z  > out  96.16s user 1.29s system 99% cpu 1:37.48 total

Summary: 1:37.48s compile time, full output of time-passes

Locally compiled before LLVM upgrade (074f49a)

$ ./x86_64-unknown-linux-gnu/stage2/bin/rustc -V
rustc 1.8.0-dev (074f49a35 2016-01-29)
$ time ./x86_64-unknown-linux-gnu/stage2/bin/rustc src/libsyntax/lib.rs -O -Z time-passes > out
./x86_64-unknown-linux-gnu/stage2/bin/rustc src/libsyntax/lib.rs -O -Z  > out  81.39s user 1.28s system 99% cpu 1:22.69 total

Summary: 1:22.69s compile time, full output of time-passes

Summary

So there are some fascinating data points here:

  • The nightlies we're shipping are 35% slower than a locally compiled copy -- Edit: nightly has LLVM assertions enabled where my local compilers did not
  • The LLVM upgrade made compilation 18% slower
  • Almost the entire compile time difference can be attributed to LLVM

Ok, so something looks clearly suspicious with LLVM! Let's try and take a closer
look. First, some data dumps of -Z time-llvm-passes

Unfortunately I don't have much of a takeaway from this. Maybe others might
thought?

Next steps

  • Should we revert the LLVM upgrade? This would unfortunately not be trivial.
  • Is there something here we can report to upstream LLVM? Is there a bug fix we
    can include in our local LLVM fork temporarily?
  • Is this analysis a red herring and perhaps there's something else going on
    here?
@alexcrichton alexcrichton added I-nominated regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 5, 2016
@alexcrichton
Copy link
Member Author

Nominating for triage as well

cc @rust-lang/compiler
cc @Manishearth
cc @nikomatsakis
cc @nrc
cc @dotdash

@nagisa
Copy link
Member

nagisa commented Feb 5, 2016

Locally compiled after LLVM upgrade (074f49a)

before?


Most of the time-intensive passes are universally slower. There’s more passes being run in the new LLVM, but all the new ones are pretty fast (timings around 0.000s).

@alexcrichton
Copy link
Member Author

@nagisa ah yeah oops that was a typo (corrected above)

Also I just realized that nightly compilers have LLVM assertions enabled whereas my local compilers did not, that would explain the discrepancy between those timings.

@Aatch
Copy link
Contributor

Aatch commented Feb 9, 2016

Looks like something to ping the LLVM guys about, at least to get some insight. Loop-invariant code motion seems like it's taken a significant hit, with the top 3 LICM passes all taking around twice the time now.

@dotdash
Copy link
Contributor

dotdash commented Feb 10, 2016

This is at least partially caused by the new AA handling which ends up being quadratic in the number of used passes. I've filed an LLVM bug at https://llvm.org/bugs/show_bug.cgi?id=26564

@nikomatsakis
Copy link
Contributor

triage: P-medium

Not much to do here besides try to keep an eye on how LLVM progresses in fixes this issue, right? (Rolling back LLVM doesn't really seem like a viable plan.)

@rust-highfive rust-highfive added P-medium Medium priority and removed I-nominated labels Feb 11, 2016
@Aatch
Copy link
Contributor

Aatch commented Feb 11, 2016

At the very least we could apply a temporary local patch if it's a quick hack vs. a longer proper fix.

@Manishearth
Copy link
Member

We could perhaps disable that pass? I'm not sure if it is a pass that's causing the problem.

@dotdash
Copy link
Contributor

dotdash commented Feb 12, 2016

We could remove TBAA which does nothing for us. That saves a few percent.
Am 12.02.2016 12:37 Vorm. schrieb "Manish Goregaokar" <
[email protected]>:

We could perhaps disable that pass? I'm not sure if it is a pass that's
causing the problem.


Reply to this email directly or view it on GitHub
#31435 (comment).

@alexcrichton
Copy link
Member Author

We've got a pretty long period of time before this reaches stable, so I'd be willing to give the LLVM bug report some more time to get some analysis on behalf of the LLVM folks. If this gets close to stable, however, backporting a change to just disable TBAA seems like it should definitely be worth it! In other words, ideally, there'd be an upstream patch to fix this regressions, we'd backport it to our branch, but failing that we can just locally disable the regression temporarily.

@jonas-schievink
Copy link
Contributor

This is perhaps a bit off-topic, but why are we running passes that don't do anything for us? Just to keep us close to the C/C++ pass pipeline?

@dotdash
Copy link
Contributor

dotdash commented Feb 20, 2016

Yes, as far as I'm aware that's it. The reason for that is that the default pass setup works reasonablly well so far, and having your own pass setup means more maintenance to keep compatible with different LLVM versions and that you don't automatically profit from improvements made to the default pass setup (obviously). That said, there have been various people who wanted to look into getting a more rust specific pass setup, it's just a ton of work and nothing has been completed (started?) yet.

Also, for this specific analysis, the extra time spent on TBAA was negligible, shouldn't have been much more than just checking for TBAA metadata, seeing that there is none and handing things over to the next AA analysis.

@sanxiyn sanxiyn added the I-compiletime Issue: Problems and improvements with respect to compile times. label Mar 18, 2016
@sanxiyn
Copy link
Member

sanxiyn commented Mar 18, 2016

AA fix landed upstream in LLVM r262490.

eddyb added a commit to eddyb/rust that referenced this issue Mar 19, 2016
Update LLVM to include a backport to restore AA performance

cc rust-lang#31435
r? @alexcrichton
@nikomatsakis
Copy link
Contributor

I'm not 100% sure what the status is here -- but it seems like we merged the fix from upstream, so going to close this as fixed. Feel free to re-open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-compiletime Issue: Problems and improvements with respect to compile times. P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants