-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
have on_completion record subcycles #85186
have on_completion record subcycles #85186
Conversation
Your description of the issue agrees with my thoughts in #83538 (comment). However, I don't have a very good understanding of the co-inductive cycle handling code (which is why I went with the earlier fix). I'd like to have someone else look over this as well. |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 8c7b7a0 with merge 6fbb7707efd14220bfaa4efa905c7fff7c51cb46... |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
@Aaron1011 maybe it would be good to do a talk through session -- this relates to my goal of building up expertise around the trait system. I'll leave a comment on Zulip. |
8c7b7a0
to
aa83920
Compare
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit aa83920b7d1d58015fb19b7e71e2887c83e2babc with merge cd2b79ca5ca0c76854b71f4893e2495a6ce19722... |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
This attribute will cause us to invoke evaluate on every where clause of an invoked function and to generate an error with the result. Without this, it is very difficult to observe the effects of invoking the trait evaluator.
9b5b8df
to
c848ba6
Compare
OK, I finally found a way to test this issue in a satisfactory way. I've included tests both for the original problem and the ICE we found in rustdoc. I also spent some time talking over how it works with @jackh726, which you can watch here. r? @jackh726 |
c848ba6
to
f932dc2
Compare
This comment has been minimized.
This comment has been minimized.
Thanks @nikomatsakis for going over this with me (and for all those that watch the recording) This is a surprisingly simple change for an extremely complex issue. It is subtle though, so regardless of perf results, I want to @bors rollup=never That being said, r=me when perf is done. |
Finished benchmarking try commit (61b32d567658b13ad4d86cc5fa7c5a4b76bae0e0): 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 |
@bors r=jackh726 |
📌 Commit 89c58ea has been approved by |
Nominating for beta and stable backport. These incremental failures are a big problem. |
☀️ Test successful - checks-actions |
Both backports have been discussed and pushed to next week's T-compiler meeting (Zulip log) |
discussed in steering meeting about incr-comp fingerprints declining to backport for beta and likewise for stable. |
have on_completion record subcycles
Rework
on_completion
method so that it removes allprovisional cache entries that are "below" a completed
node (while leaving those entries that are not below
the node).
This corrects an imprecise result that could in turn lead
to an incremental compilation failure. Under the old
scheme, if you had:
then the provisional results for A, B, C, and D would all
be entangled. Thus, if A was
EvaluatedToOkModuloRegions
(because of that final condition), then the result for C and
D would also be demoted to "ok modulo regions".
In reality, though, the result for C depends only on C and itself,
and is not dependent on regions. If we happen to evaluate the
cycle starting from C, we would never reach A, and hence the
result would be "ok".
Under the new scheme, the provisional results for C and D
are moved to the permanent cache immediately and are not affected
by the result of A.
Fixes #83538
r? @Aaron1011