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

25% regression in serde compile time #47628

Closed
dtolnay opened this issue Jan 21, 2018 · 19 comments
Closed

25% regression in serde compile time #47628

dtolnay opened this issue Jan 21, 2018 · 19 comments
Assignees
Labels
A-incr-comp Area: Incremental compilation I-compiletime Issue: Problems and improvements with respect to compile times. P-medium Medium priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@dtolnay
Copy link
Member

dtolnay commented Jan 21, 2018

I narrowed this down to 1699293...c284f88. To reproduce:

rustup update nightly-2017-12-24 nightly-2017-12-25
git clone https://github.com/serde-rs/serde
cd serde/serde
git checkout v1.0.27
cargo update
cargo clean && time cargo +nightly-2017-12-24 build
cargo clean && time cargo +nightly-2017-12-25 build

For me nightly-2017-12-24 takes 7.4 seconds and nightly-2017-12-25 takes 9.2 seconds.

@dtolnay dtolnay added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Jan 21, 2018
@sfackler sfackler added I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 21, 2018
@arielb1
Copy link
Contributor

arielb1 commented Jan 22, 2018

A not-too-deep check reveals that trait selection might have slowed down. Seriously? (I didn't track it down to that commit, should probably bisect it to a single commit in that range).

@nikomatsakis
Copy link
Contributor

triage: P-high

Good to know what happened.

@rust-highfive rust-highfive added the P-high High priority label Jan 25, 2018
@nikomatsakis nikomatsakis self-assigned this Jan 25, 2018
@nikomatsakis
Copy link
Contributor

I'll try to bisect.

@nikomatsakis
Copy link
Contributor

Curious. I was not able to reproduce when building those commits from scratch, though I did see the perf difference with the nightly builds. I'll double-check. It could have to do with some of my config.toml options (e.g., debuginfo or debug-assertions), but that seems a bit surprising.

@nikomatsakis
Copy link
Contributor

I tried with no config.toml at all and still got basically the same timing for those two commits: 11.6s in both cases.

@nikomatsakis
Copy link
Contributor

cc @rust-lang/compiler -- can someone else try to reproduce from source?

@Zoxc
Copy link
Contributor

Zoxc commented Jan 30, 2018

I also built these two revisions from source. Both of my builds have the same timing of ~40 seconds for cargo build -j1. This is the same result as nightly-2017-12-25, while nightly-2017-12-24 builds in ~30 seconds.

@nikomatsakis
Copy link
Contributor

@Zoxc thanks! Not entirely sure what to make of that.

@nikomatsakis
Copy link
Contributor

cc @rust-lang/infra -- can we try using the PR-by-PR builds to narrow this down further or gain some insight? I'm not sure how to access those.

@kennytm
Copy link
Member

kennytm commented Jan 30, 2018

@nikomatsakis The problem isn't in rustc, but cargo. Note that starting from Dec 25 cargo passes -C incremental=... to rustc. This means the regression is introduced by rust-lang/cargo#4817 or some other PRs between rust-lang/cargo@930f9d9...e08f310.

~/serde ((v1.0.27)):160$ cargo clean && time cargo +nightly-2017-12-24 build -vv
   Compiling serde v1.0.27 (file:///~/serde/serde)
     Running `rustc --crate-name serde src/lib.rs --crate-type lib --emit=dep-info,link -C debuginfo=2 --cfg 'feature="default"' --cfg 'feature="std"' -C metadata=7c335e5d4a9ee66d -C extra-filename=-7c335e5d4a9ee66d --out-dir /~/serde/target/debug/deps -L dependency=/~/serde/target/debug/deps`
    Finished dev [unoptimized + debuginfo] target(s) in 9.50 secs

real	0m9.752s
user	0m9.463s
sys	0m0.409s

~/serde ((v1.0.27)):161$ cargo clean && time cargo +nightly-2017-12-25 build -vv
   Compiling serde v1.0.27 (file:///~/serde/serde)
     Running `rustc --crate-name serde serde/src/lib.rs --crate-type lib --emit=dep-info,link -C debuginfo=2 --cfg 'feature="default"' --cfg 'feature="std"' -C metadata=633312819f55a634 -C extra-filename=-633312819f55a634 --out-dir /~/serde/target/debug/deps -C incremental=/~/serde/target/debug/incremental -L dependency=/~/serde/target/debug/deps`
    Finished dev [unoptimized + debuginfo] target(s) in 12.42 secs

real	0m13.098s
user	0m11.421s
sys	0m0.814s

~/serde ((v1.0.27)):162$ cargo clean && time cargo +16992930835ce3376a4aaed42307726e1fc78e45 build -vv
   Compiling serde v1.0.27 (file:///~/serde/serde)
     Running `rustc --crate-name serde serde/src/lib.rs --crate-type lib --emit=dep-info,link -C debuginfo=2 --cfg 'feature="default"' --cfg 'feature="std"' -C metadata=7a2f2aaec6829696 -C extra-filename=-7a2f2aaec6829696 --out-dir /~/serde/target/debug/deps -C incremental=/~/serde/target/debug/incremental -L dependency=/~/serde/target/debug/deps`
    Finished dev [unoptimized + debuginfo] target(s) in 12.82 secs

real	0m13.442s
user	0m11.809s
sys	0m0.781s

Edit: Disabling incremental with CARGO_INCREMENTAL=0 does bring down the build time from ~11s to ~9s.

@dtolnay
Copy link
Member Author

dtolnay commented Jan 30, 2018

Is 25% within the expected range of slowdown from incremental compilation on a clean build?

@nikomatsakis
Copy link
Contributor

@dtolnay unknown, but it seems worth optimizing!

Question: what happens on subsequent compiles?

@kennytm kennytm added the A-incr-comp Area: Incremental compilation label Feb 1, 2018
@nikomatsakis
Copy link
Contributor

OK, so, here are latest measurements:

Nightly cargo clean && cargo build touch src/lib.rs && cargo build
2017-12-24 9.82 9.82
2017-12-25 11.63 4.35
2018-01-15 11.5 4.23
2018-01-20 11.28 4.15
2018-01-23 11.48 4.22
2018-01-25 10.57 4.12
2018-02-02 10.59 4.4
2018-02-04 10.70 4.3
2018-02-05 9.84 3.75
2018-02-07 9.77 3.73

Seems like something changed in between 2018-02-04 and 2018-02-05.

@nikomatsakis
Copy link
Contributor

Could be disabling ThinLTO?

@nikomatsakis
Copy link
Contributor

In any case, the regression appears to be gone, in an absolute sense. A quick glance at the profiles suggested we are spending a lot of time hashing, but I'd have to do a custom build with framepointers to get real data. I'm inclined to close this, though.

@nikomatsakis
Copy link
Contributor

(It could well be that ThinLTO was making hashing etc add a lot more overhead than it otherwise should.)

@nikomatsakis
Copy link
Contributor

triage: P-medium

Priority is not so hot. @michaelwoerister is going to do some final investigation and decide whether to close or what.

@rust-highfive rust-highfive added P-medium Medium priority and removed P-high High priority labels Feb 15, 2018
@pnkfelix
Copy link
Member

pnkfelix commented Mar 1, 2018

triage: @michaelwoerister will confirm that build times are back to normal. If so, he will close as "not a regression" (or at least no longer a regression...)

@michaelwoerister
Copy link
Member

I did some measurements with the current nightly and the versions from the OP:

> cargo clean && cargo build

2017-12-24 - 6.92 secs
2017-12-25 - 8.49 secs
2018-03-06 - 7.11 secs

This confirms that a default build has gotten slower between the first two nightlies and then faster again. However, I think, the slowdown is due to incremental compilation becoming the default between the two nightlies. Forcing incremental compilation to off shows that there little difference between them, and that the compiler has actually become a bit faster since:

> cargo clean && CARGO_INCREMENTAL=0 cargo build

2017-12-24 - 6.89 secs
2017-12-25 - 6.83 secs
2018-03-06 - 6.10 secs

Vice versa, forcing incr. comp. to on shows little difference between the December nightlies and that incr. comp. especially has become faster:

> cargo clean && CARGO_INCREMENTAL=1 cargo build

2017-12-24 - 8.57 secs
2017-12-25 - 8.46 secs
2018-03-06 - 7.12 secs

Hopefully you'll also see a benefit from incr. comp.:

touch src/lib.rs && cargo build
2017-12-24 - 6.85 secs
2017-12-25 - 3.54 secs
2018-03-06 - 2.94 secs

I think this explains the build time regression between the two nightlies. It is expected that clean incremental builds are slower. For the Dec 25 nightly the overhead is ~24% in my measurements, for the current nightly its ~16%. Hopefully we can bring this down further in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-incr-comp Area: Incremental compilation I-compiletime Issue: Problems and improvements with respect to compile times. P-medium Medium priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. 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