Use EvaluatedToOkModuloRegions when we get a coinductive match
#83913
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This forces every participant in the cycle to have a result `EvaluatedToOkModuloRegions` (or an error) as well, since their result will be influenced by the coinductive result. While I'm confident that this is sound (AFAIK, it's always sound to replace `EvaluatedToOk` with `EvaluatedToOkModuloRegions`), I don't really understand the coinductive cycle logic. This appears to fix the `syn` incremental crash, but there could be a different way of fixing it as well.
|
r? @lcnr (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit 3672602 with merge 5dbb81f33c342e785a59a73d7516283763759016... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
☀️ Try build successful - checks-actions |
|
Queued 5dbb81f33c342e785a59a73d7516283763759016 with parent d322385, future comparison URL. |
|
Finished benchmarking try commit (5dbb81f33c342e785a59a73d7516283763759016): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
rustbot
removed
the
S-waiting-on-perf
|
cc @nikomatsakis: The perf impact seems worse than the previous caching fix |
|
The perf results look quite similar to #83719 - it looks like all of the regressions from that PR may have come from just this change (as opposed to returning |
|
I haven't been able to come up with a minimal regression test for this. Howver, the perf results show that it fixed the |
|
I agree that this is sound, but it really doesn't very necessary. I guess @Aaron1011 I should invest some time into digging into this syn incremental crash. Can you give a bit more details about the coinductive cycle that seems to be leading to a problem, if you know? (Or other analysis you have done) |
|
I've been working with the https://github.com/Aaron1011/syn-crash repository from #83538 (comment) On the first run through (with an empty incremental cache), I get the following cycle: The two important lines are: The predicate In order to evaluate During the second compilation session, we do not appear to hit that coinductive cycle for some reason (I haven't yet figured out why). Instead, we hit this cycle: We end up evaluating this predicate to |
|
@Aaron1011 any chance we can chat during my trait office hours at some point? (9am-10am US ET most days) |
|
@nikomatsakis: Would this Wednesday work for you? |
|
@Aaron1011 sure |
|
@Aaron1011 Ping from triage, any updates on this after the chat above? |
crlf0710
added
S-waiting-on-author
|
I've been digging into this PR a bit, it's just taking me a while to really understand what's going on. I'd like an assessment of the urgency here, though. |
|
I've taken a look at the logs from the minimal reproduction in #83538 (comment). I think this is still the simplest fix to the problem. |
|
@nikomatsakis in terms of urgency I think we need some fix to the syn crash by the end of this cycle, and ideally one that's backportable to beta (1.53). I think setting a goal of fixing enough of the incremental bugs that we can reasonably enable incremental on 1.54 at the latest should be a priority though. |
|
I'm going to close this PR in favor of #85186 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This forces every participant in the cycle to have a result
EvaluatedToOkModuloRegions(or an error) as well, since their resultwill be influenced by the coinductive result.
While I'm confident that this is sound (AFAIK, it's always sound to
replace
EvaluatedToOkwithEvaluatedToOkModuloRegions), I don'treally understand the coinductive cycle logic. This appears to fix the
synincremental crash, but there could be a different way of fixing itas well.