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.

Sign up for GitHub

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

Jump to bottom

check_const: use the same ParamEnv as codegen for statics #52925

Merged
merged 2 commits into from
Aug 3, 2018
Merged

check_const: use the same ParamEnv as codegen for statics #52925

merged 2 commits into from
Aug 3, 2018

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jul 31, 2018 •
edited
Loading

Fixes at least part of #52849 (my CTFE-stress benchmark). Note that I do not know what I am doing here, this is just based on hints from @oli-obk.

r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 31, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Aug 1, 2018

@bors r+

papers over a recent compile time regression. We're figuring out a nice fix. Until then, this should do it.

@bors
Copy link
Contributor

bors commented Aug 1, 2018

📌 Commit 222dd17 has been approved by oli-obk

@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 Aug 1, 2018
@RalfJung
Copy link
Member Author

RalfJung commented Aug 1, 2018

@bors r-
we'll do a perf run first

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 1, 2018
@RalfJung
Copy link
Member Author

RalfJung commented Aug 1, 2018

@bors try

@bors
Copy link
Contributor

bors commented Aug 1, 2018

⌛ Trying commit 222dd17 with merge daf753a...

bors added a commit that referenced this pull request Aug 1, 2018
@bors
Auto merge of #52925 - RalfJung:sanity_check, r=<try>
daf753a 
check_const: use the same ParamEnv as codegen for statics

Fixes at least part of #52849 (my CTFE-stress benchmark). Note that I do not know what I am doing here, this is just based on hints from @oli-obk.

r? @oli-obk
@kennytm kennytm added S-waiting-on-perf Status: Waiting on a perf run to be completed. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 1, 2018
@kennytm
Copy link
Member

kennytm commented Aug 1, 2018

@rust-timer build daf753a

@rust-timer
Copy link
Collaborator

Success: Queued daf753a with parent e94df4a, comparison URL.

@bors
Copy link
Contributor

bors commented Aug 1, 2018

☀️ Test successful - status-travis
State: approved= try=True

@RalfJung
Copy link
Member Author

RalfJung commented Aug 1, 2018

perf seems to say it definitely helps. As expected, check is still much slower -- previously, check wouldn't even compute the statics; now it does. But also for opt and debug, there's still a very noticeable gap between pre-sanity_check and post-this-patch. However, that's in a benchmark marked ?.

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Aug 1, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Aug 2, 2018

@bors r+

@bors
Copy link
Contributor

bors commented Aug 2, 2018

📌 Commit 222dd17 has been approved by oli-obk

@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 Aug 2, 2018
cramertj added a commit to cramertj/rust that referenced this pull request Aug 2, 2018
@cramertj
Rollup merge of rust-lang#52925 - RalfJung:sanity_check, r=oli-obk
78560fa 
check_const: use the same ParamEnv as codegen for statics

Fixes at least part of rust-lang#52849 (my CTFE-stress benchmark). Note that I do not know what I am doing here, this is just based on hints from @oli-obk.

r? @oli-obk
@cramertj cramertj mentioned this pull request Aug 2, 2018
Closed
cramertj added a commit to cramertj/rust that referenced this pull request Aug 2, 2018
@cramertj
Rollup merge of rust-lang#52925 - RalfJung:sanity_check, r=oli-obk
977c015 
check_const: use the same ParamEnv as codegen for statics

Fixes at least part of rust-lang#52849 (my CTFE-stress benchmark). Note that I do not know what I am doing here, this is just based on hints from @oli-obk.

r? @oli-obk
@cramertj cramertj mentioned this pull request Aug 2, 2018
Closed
@bors
Copy link
Contributor

bors commented Aug 3, 2018

⌛ Testing commit 222dd17 with merge 4dae470...

bors added a commit that referenced this pull request Aug 3, 2018
@bors
Auto merge of #52925 - RalfJung:sanity_check, r=oli-obk
4dae470 
check_const: use the same ParamEnv as codegen for statics

Fixes at least part of #52849 (my CTFE-stress benchmark). Note that I do not know what I am doing here, this is just based on hints from @oli-obk.

r? @oli-obk
@RalfJung RalfJung mentioned this pull request Aug 3, 2018
Closed
@bors
Copy link
Contributor

bors commented Aug 3, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: oli-obk
Pushing 4dae470 to master...

@bors bors merged commit 222dd17 into rust-lang:master Aug 3, 2018
@RalfJung RalfJung mentioned this pull request Aug 3, 2018
Merged
@RalfJung RalfJung deleted the sanity_check branch August 16, 2018 10:53
@oli-obk oli-obk added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 25, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Aug 25, 2018

Nominating for beta as it partially addresses the perf regression in #52849 (comment)

@pietroalbini pietroalbini added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 25, 2018
@RalfJung RalfJung mentioned this pull request Aug 28, 2018
Merged
@oli-obk oli-obk mentioned this pull request Aug 30, 2018
Merged
@nikomatsakis
Copy link
Contributor

cc @rust-lang/compiler -- marking as beta-accepted since it is a small PR, approved by @oli-obk and @RalfJung, and affects a serious perf regression

@nikomatsakis nikomatsakis added beta-accepted Accepted for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Aug 30, 2018
@nikomatsakis
Copy link
Contributor

Backported in #53825

bors added a commit that referenced this pull request Aug 31, 2018
@bors
Auto merge of #53825 - oli-obk:beta_backport, r=RalfJung
1daa912 
[beta] const eval perf regression fix

backports #52925 (for #52849)

and additionally skips hashing on every evaluation step
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@oli-obk oli-obk

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@RalfJung @oli-obk @bors @kennytm @rust-timer @nikomatsakis @pietroalbini @rust-highfive