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

Remove const eval loop detector #70087

Merged
merged 4 commits into from
Mar 24, 2020

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Mar 17, 2020

Now that there is a configurable instruction limit for CTFE (see #67260), we can replace the loop detector with something much simpler. See #66946 for more discussion about this. Although the instruction limit is nightly-only, the only practical way to reach the default limit uses nightly-only features as well (although CTFE will still execute code using such features inside an array initializer on stable).

This will at the very least require a crater run, since it will result in an error wherever the "long running const eval" warning appeared before. We may need to increase the default for const_eval_limit to work around this.

Resolves #54384 cc #49980
r? @oli-obk cc @RalfJung

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 17, 2020
@rust-highfive

This comment has been minimized.

@ecstatic-morse ecstatic-morse force-pushed the remove-const-eval-loop-detector branch from d9e1918 to 044dc6e Compare March 18, 2020 03:28
@RalfJung
Copy link
Member

@bors try

@bors
Copy link
Contributor

bors commented Mar 18, 2020

⌛ Trying commit 044dc6e with merge f88e9a4452e792c29632101b348a1df9876d45cd...

@bors
Copy link
Contributor

bors commented Mar 18, 2020

☀️ Try build successful - checks-azure
Build commit: f88e9a4452e792c29632101b348a1df9876d45cd (f88e9a4452e792c29632101b348a1df9876d45cd)

@RalfJung
Copy link
Member

@craterbot check
(first time doing this, let's see if it works :D )

@craterbot
Copy link
Collaborator

🔒 Error: you're not allowed to interact with this bot.

🔑 If you are a member of the Rust team and need access, add yourself to the whitelist.
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@RalfJung
Copy link
Member

@Centril well looks like I still need your help to start a crater run, sorry.

@Centril
Copy link
Contributor

Centril commented Mar 18, 2020

Weird... cc @pietroalbini

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-70087 created and queued.
🤖 Automatically detected try build f88e9a4452e792c29632101b348a1df9876d45cd
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 18, 2020
@craterbot
Copy link
Collaborator

🚧 Experiment pr-70087 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

|
LL | n = if n % 2 == 0 { n/2 } else { 3*n + 1 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: see issue #49146 <https://github.com/rust-lang/rust/issues/49146> for more information
= help: add `#![feature(const_if_match)]` to the crate attributes to enable

warning: Constant evaluating a complex constant, this might take some time
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be useful to keep around.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean at like 1% or 10% of the step limit? Not sure how to choose a good number for that, but anything above 10% of the step limit seems useless since you're hitting the step limit soon, and anything below 30s will likely be annoying more often than not. It's very hard to silence this warning (it may be getting emitted in a different crate from the one currently being evaluated).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anecdotally, the default step limit only takes a few seconds to trigger, so I don't think preserving the warning is helpful.

@craterbot
Copy link
Collaborator

🎉 Experiment pr-70087 is completed!
📊 0 regressed and 1 fixed (96097 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Mar 21, 2020
@ecstatic-morse ecstatic-morse marked this pull request as ready for review March 22, 2020 19:55
@ecstatic-morse
Copy link
Contributor Author

There's no nice way to add a note to a particular InterpError, so I included const_eval_limit in the message itself for now.

@ecstatic-morse ecstatic-morse force-pushed the remove-const-eval-loop-detector branch from fa847a0 to b5636b8 Compare March 22, 2020 20:07
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. r=me if you want.

@ecstatic-morse
Copy link
Contributor Author

@bors r=RalfJung

@bors
Copy link
Contributor

bors commented Mar 23, 2020

📌 Commit b5636b8 has been approved by RalfJung

@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 Mar 23, 2020
Centril added a commit to Centril/rust that referenced this pull request Mar 23, 2020
…op-detector, r=RalfJung

Remove const eval loop detector

Now that there is a configurable instruction limit for CTFE (see rust-lang#67260), we can replace the loop detector with something much simpler. See rust-lang#66946 for more discussion about this. Although the instruction limit is nightly-only, the only practical way to reach the default limit uses nightly-only features as well (although CTFE will still execute code using such features inside an array initializer on stable).

This will at the very least require a crater run, since it will result in an error wherever the "long running const eval" warning appeared before. We may need to increase the default for `const_eval_limit` to work around this.

Resolves rust-lang#54384 cc rust-lang#49980
r? @oli-obk cc @RalfJung
This was referenced Mar 23, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 24, 2020
Rollup of 11 pull requests

Successful merges:

 - rust-lang#67761 (Move the dep_graph construction to a dedicated crate.)
 - rust-lang#69740 (Replace some desc logic in librustc_lint with article_and_desc)
 - rust-lang#69981 (Evaluate repeat expression lengths as late as possible)
 - rust-lang#70087 (Remove const eval loop detector)
 - rust-lang#70242 (Improve E0308 error message wording)
 - rust-lang#70264 (Fix invalid suggestion on `&mut` iterators yielding `&` references)
 - rust-lang#70267 (get rid of ConstPropUnsupported; use ZST marker structs instead)
 - rust-lang#70277 (Remove `ReClosureBound`)
 - rust-lang#70283 (Add regression test for rust-lang#70155.)
 - rust-lang#70294 (Account for bad placeholder types in where clauses)
 - rust-lang#70309 (Clean up E0452 explanation)

Failed merges:

r? @ghost
@bors bors merged commit 72c99f2 into rust-lang:master Mar 24, 2020
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Apr 23, 2020
…RalfJung

Remove outdated reference to interpreter snapshotting

This should have been a part of rust-lang#70087.

r? @RalfJung
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 23, 2020
…RalfJung

Remove outdated reference to interpreter snapshotting

This should have been a part of rust-lang#70087.

r? @RalfJung
@ecstatic-morse ecstatic-morse deleted the remove-const-eval-loop-detector branch June 2, 2020 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove CTFE loop detector
8 participants