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

Avoid reporting overflow in is_impossible_method #100705

Merged
merged 1 commit into from
Aug 26, 2022

Conversation

compiler-errors
Copy link
Member

Fixes #100620

We're evaluating a new predicate in a different param-env than it was checked during typeck, so be more careful about handling overflow errors. Instead of using FulfillmentCtxt, using InferCtxt::evaluate_obligation by itself will give us back the overflow error, so we can throw it away properly.

This may give us more false-positives, but it doesn't regress the <HashMap as Iterator>::rev example that originally motivated adding is_impossible_method in the first place.

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Aug 18, 2022
@rust-highfive
Copy link
Collaborator

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 18, 2022
@notriddle
Copy link
Contributor

r? compiler

@rust-highfive rust-highfive assigned oli-obk and unassigned jackh726 Aug 24, 2022
@weiznich
Copy link
Contributor

Any chance to merge this soon, as this is currently blocking a diesel release?

@oli-obk
Copy link
Contributor

oli-obk commented Aug 25, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Aug 25, 2022

📌 Commit c4a5b14 has been approved by oli-obk

It is now in the queue for this repository.

@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 25, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Aug 25, 2022
…i-obk

Avoid reporting overflow in `is_impossible_method`

Fixes rust-lang#100620

We're evaluating a new predicate in a different param-env than it was checked during typeck, so be more careful about handling overflow errors. Instead of using `FulfillmentCtxt`, using `InferCtxt::evaluate_obligation` by itself will give us back the overflow error, so we can throw it away properly.

This may give us more false-positives, but it doesn't regress the `<HashMap as Iterator>::rev` example that originally motivated adding `is_impossible_method` in the first place.
@compiler-errors
Copy link
Member Author

@bors p=1

this is also causing errors on diesel perf runs, so bumping the prio a bit

@bors
Copy link
Contributor

bors commented Aug 26, 2022

⌛ Testing commit c4a5b14 with merge 983f4da...

@bors
Copy link
Contributor

bors commented Aug 26, 2022

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 983f4da to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 26, 2022
@bors bors merged commit 983f4da into rust-lang:master Aug 26, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 26, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (983f4da): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
0.7% [0.6%, 0.8%] 6
Regressions ❌
(secondary)
0.3% [0.3%, 0.3%] 1
Improvements ✅
(primary)
-0.5% [-0.6%, -0.4%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.4% [-0.6%, 0.8%] 8

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
4.2% [2.2%, 6.8%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.8% [-2.8%, -2.8%] 1
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.4% [2.4%, 2.4%] 1
Improvements ✅
(primary)
-2.9% [-3.3%, -2.4%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.9% [-3.3%, -2.4%] 2

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@rustbot rustbot added the perf-regression Performance regression. label Aug 26, 2022
@Kobzol
Copy link
Contributor

Kobzol commented Aug 26, 2022

The diesel-1.4.8 benchmark still seems to fail even with this PR.

@compiler-errors
Copy link
Member Author

Ugh, seriously? I am just going to revert this is_impossible_method PR then until I can fix it correctly. 🤦

@Kobzol
Copy link
Contributor

Kobzol commented Aug 26, 2022

I'd be glad to be wrong, but the error is still there (this commit was already perf-tested) https://perf.rust-lang.org/status.html and when I tried 983f4daddf238d114c4adc4751c5528fc6695a5a locally with rustc-perf, it still failed.

@rylev
Copy link
Member

rylev commented Aug 26, 2022

Note: #100991 is tracking this and has some small additional context.

@compiler-errors
Copy link
Member Author

compiler-errors commented Aug 26, 2022

The primary perf regression on this PR seems to be reversed by #101039, though given the nature of this PR being rustdoc specific, I wouldn't be surprised if it were just noise. Also that PR fixes the diesel doc issue.

@rylev
Copy link
Member

rylev commented Aug 30, 2022

Marking this as triaged given the analysis above.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Aug 30, 2022
@compiler-errors compiler-errors deleted the issue-100620 branch August 11, 2023 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rustdoc fails to build diesel documentation on current nightly