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

7% regression in compile time of tuple-stress benchmark #44677

Closed
alexcrichton opened this issue Sep 18, 2017 · 8 comments
Closed

7% regression in compile time of tuple-stress benchmark #44677

alexcrichton opened this issue Sep 18, 2017 · 8 comments
Labels
C-bug Category: This is a bug. I-compiletime Issue: Problems and improvements with respect to compile times. P-low Low 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

According to perf.rust-lang.org graphs #44275 caused a ~8% regression in compile time for this benchmark

cc @eddyb (an old nemesis!)

@alexcrichton alexcrichton added I-compiletime Issue: Problems and improvements with respect to compile times. 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 Sep 18, 2017
@eddyb
Copy link
Member

eddyb commented Sep 18, 2017

Yeah, we knew it would take a hit, but without absolute graphs it didn't seem that bad and we just took the risk.

cc @nikomatsakis Do you know of any caching missing here? We do cache the result of evaluation, is this just the cost of renormalizing over and over again?

@Mark-Simulacrum Mark-Simulacrum added the C-bug Category: This is a bug. label Sep 18, 2017
@nikomatsakis
Copy link
Contributor

@eddyb I'm not sure! Would have to profile I guess.

@nikomatsakis
Copy link
Contributor

triage: P-high

Calling this P-high until we decide if (a) it is isolated or has a pervasive effect and (b) if we know the cause and can readily fix. It'd be great for someone to do some comparative profiling!

@rust-highfive rust-highfive added the P-high High priority label Sep 21, 2017
@nikomatsakis
Copy link
Contributor

Note: the sources to the benchmark can be found in https://github.com/rust-lang-nursery/rustc-benchmarks

@eddyb
Copy link
Member

eddyb commented Sep 21, 2017

I believe this might be src/librustc_passes/consts.rs at fault, since it evaluates everything in there, and it's probably interning ty::Const, not any of the deferred array length evaluation changes.
All of this should get better with miri, I would think, and I expect the effect to be isolated.

@nikomatsakis
Copy link
Contributor

Based on @eddyb's comment, lowering priority until miri is merged.

triage: P-low

@rust-highfive rust-highfive added P-low Low priority and removed P-high High priority labels Sep 21, 2017
@Mark-Simulacrum
Copy link
Member

@michaelwoerister
Copy link
Member

Discussed in triage. Closing since there isn't anything actionable here really.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. I-compiletime Issue: Problems and improvements with respect to compile times. P-low Low 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

6 participants