Skip to content

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

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
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
Collaborator

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
Collaborator

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
Collaborator

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
Collaborator

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
Collaborator

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
Collaborator

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
# for free to join this conversation on GitHub. Already have an account? # 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