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

50-110% compilation time regressions due to miri #48846

Closed
12 tasks
oli-obk opened this issue Mar 8, 2018 · 8 comments · Fixed by #48864
Closed
12 tasks

50-110% compilation time regressions due to miri #48846

oli-obk opened this issue Mar 8, 2018 · 8 comments · Fixed by #48864
Labels
A-codegen Area: Code generation A-const-eval Area: constant evaluation (mir interpretation) C-enhancement Category: An issue proposing an enhancement or a PR with one. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-compiler-performance Working group: Compiler Performance

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Mar 8, 2018

It's not clear yet whether mir interpretation is the culprit or some changes that crept into the same PR.

Perf data: http://perf.rust-lang.org/compare.html?start=cdcca786468a71375584bb48e3093790c91084f7&end=c90f68224b069f5bb2a80e30e2737e4bb17c1466&stat=instructions:u

cc @nikomatsakis @michaelwoerister

Important regressions to investigate

  • tuple-stress | avg: 35.0% | min: 9.8% | max: 109.4%
    • clean-incremental is a >100% hit! The rest are small.
  • tuple-stress-opt | avg: 45.9% | min: 23.1% | max: 109.1%
    • same, clean-incremental is a >100% hit!
  • encoding-opt | avg: 48.9% | min: 7.0% | max: 97.6%
    • clean incremental-opt
    • patched incremental: println-opt
  • encoding | avg: 56.3% | min: 18.1% | max: 94.9%
    • clean is ~ 20%, as is baseline incremental
    • incremental cases are almost 100%
  • coercions-opt | avg: 46.9% | min: 31.8% | max: 74.6%
    • pretty universally bad, though incremental hurts worse
  • coercions | avg: 37.0% | min: 24.3% | max: 66.6%
    • pretty universally bad, though incremental hurts worse
  • html5ever-opt | avg: 18.3% | min: 5.6% | max: 41.3%
    • incremental
  • inflate | avg: 29.4% | min: 1.3% | max: 40.4%
    • universally bad, though incremental hurts worse
  • regex-opt | avg: -0.2% | min: -18.0% | max: 4.7%
    • patched incremental: byte frequencies-opt
  • script-servo-opt | avg: 4.8% | min: 0.7% | max: 12.1%
    • clean incremental-opt
  • script-servo | avg: 6.0% | min: 1.7% | max: 11.6%
    • clean incremental
  • inflate-opt | avg: 7.7% | min: 1.2% | max: 11.6%
    • incremental cases primarily
@oli-obk oli-obk added I-slow Issue: Problems and improvements with respect to performance of generated code. WG-compiler-performance Working group: Compiler Performance labels Mar 8, 2018
@michaelwoerister
Copy link
Member

I'm going to start with encoding ...

@nikomatsakis
Copy link
Contributor

It would be good to dig into those use cases -- in particular, if you open them, we can see which of them are incremental use cases and which are not.

@nikomatsakis
Copy link
Contributor

I just updated the tracking issue with that information. Most of them are incremental cases.

@michaelwoerister
Copy link
Member

OK, for encoding clean incremental it's pretty clear: 58% of the execution time are spent in TyCtxt::const_eval() called from monomorphize::collector::collect_const(). Does that ring a bell, @oli-obk? I haven't had time yet to really look at the code in question.

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 8, 2018

We might simply be encoding more constants now (that were previously encoded as HIR). I'll have to look at the exact logs (rustc_mir::interpret::const_eval=debug to just get the entry points). I'll check tomorrow morning

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 9, 2018

Ok. I found the issue. We don't seem to be loading constants from the cache. So the enormous regressions are from recomputing all constants, even if nothing changed in the code at all. That should be easy enough to fix

bors added a commit that referenced this issue Mar 13, 2018
Cache const eval queries

fixes #48846 (I think, still running more perf tests, but tuple-stress stops recomputing any constants)

r? @michaelwoerister
bors added a commit that referenced this issue Mar 14, 2018
Cache const eval queries

fixes #48846 (I think, still running more perf tests, but tuple-stress stops recomputing any constants)

r? @michaelwoerister
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 14, 2018

Only the incremental regressions were fixed. I'll investigate the others, too

New perf data: https://perf.rust-lang.org/compare.html?start=2018-03-08&end=02094271f28e88e64b66014726f1d996bf7acae3&stat=instructions%3Au

@oli-obk oli-obk reopened this Mar 14, 2018
alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 23, 2018
…er,eddyb

Remove slow HashSet during miri stack frame creation

fixes rust-lang#49237

probably has a major impact on rust-lang#48846

r? @michaelwoerister

cc @eddyb I know you kept telling me to use vectors instead of hash containers... Now I know why.
kennytm added a commit to kennytm/rust that referenced this issue Mar 24, 2018
…er,eddyb

Remove slow HashSet during miri stack frame creation

fixes rust-lang#49237

probably has a major impact on rust-lang#48846

r? @michaelwoerister

cc @eddyb I know you kept telling me to use vectors instead of hash containers... Now I know why.
kennytm added a commit to kennytm/rust that referenced this issue Mar 24, 2018
…er,eddyb

Remove slow HashSet during miri stack frame creation

fixes rust-lang#49237

probably has a major impact on rust-lang#48846

r? @michaelwoerister

cc @eddyb I know you kept telling me to use vectors instead of hash containers... Now I know why.
@jkordish jkordish added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-codegen Area: Code generation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 10, 2018
@oli-obk oli-obk added the A-const-eval Area: constant evaluation (mir interpretation) label Jun 30, 2018
@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 28, 2019

There have been a lot of perf optimizations for const eval over the last year. I think we're good now

@oli-obk oli-obk closed this as completed Jan 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-const-eval Area: constant evaluation (mir interpretation) C-enhancement Category: An issue proposing an enhancement or a PR with one. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-compiler-performance Working group: Compiler Performance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants