add with_hash_task to generate DepNode deterministically
#100987
Closed
Conversation
This file contains 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
rustbot
added
the
T-compiler
|
r? @cjgillot (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
s/with_anon_task/with_hash_task, right? |
That's right, thanks! |
cjgillot
reviewed
Aug 27, 2022
| @@ -817,7 +821,19 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { | |||
| return Ok(cycle_result); | |||
| } | |||
|
|
|||
| let (result, dep_node) = self.in_task(|this| this.evaluate_stack(&stack)); | |||
| let (result, dep_node) = if cfg!(parallel_compiler) { | |||
There was a problem hiding this comment.
Why not use this branch in both parallel and serial code?
There was a problem hiding this comment.
Just wanted to minimize the impact on serial mode. We should indeed unify here for code brevity.
| // combining it with the per session random number `anon_id_seed`. This hash only need | ||
| // to map the dependencies to a single value on a per session basis. | ||
| let mut hasher = StableHasher::new(); | ||
| task_deps.reads.hash(&mut hasher); |
There was a problem hiding this comment.
Idea for a simpler solution: could we hash the DepNode instead of the DepNodeIndex to avoid order-dependence? Could this solve the issue without having to make client code do the hashing?
There was a problem hiding this comment.
This is good advice, but DepNode in TaskDeps only exists in the case of #[cfg(debug_assert)]. I'm using Hash bound now in where clause of with_task_hash, which can avoid hashing in client code.
|
I don't mean the Marking with rollup=never in case we need to bisect this. |
|
📌 Commit c5871422b9616a8790aba33aa94dac7cb6039a1c has been approved by It is now in the queue for this repository. |
bors
added
S-waiting-on-bors
|
⌛ Testing commit c5871422b9616a8790aba33aa94dac7cb6039a1c with merge 1e088860bbcd213f09533805bf528e60f6c06f01... |
|
💥 Test timed out |
bors
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
It looks like CI is having some trouble. Yea, doing some special handle for the parallel compiler might be a better solution. I think we can come up with a follow-up if needed. |
|
@cjgillot Could we do a retry and r+? |
|
☔ The latest upstream changes (presumably #104023) made this pull request unmergeable. Please resolve the merge conflicts. |
|
🔒 Merge conflict This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again. How do I rebase?Assuming
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Error message |
|
Hi @SparrowLii, I think this just needs to be rebased before it can be r+'d again. |
|
Switching to waiting on author to rebase as per this comment. @SparrowLii feel free to request a review with @rustbot author |
rustbot
added
S-waiting-on-author
SparrowLii
force-pushed
the
with_hash_task
branch
from
c587142
to
a4b1663
Compare
rustbot
added
the
A-query-system
|
@rustbot ready |
rustbot
added
S-waiting-on-review
How do the dependencies between the tasks differ? Do they have the same set, but just in a different order? |
|
I introduced the If the reason the sanity check fails is non-determinism, that could be addressed by letting |
|
☔ The latest upstream changes (presumably #103695) made this pull request unmergeable. Please resolve the merge conflicts. |
|
This needs a rebase again |
rustbot
added
S-waiting-on-author
|
@SparrowLii any updates on this? |
|
Not yet. This is a problem that is difficult to reproduce under parallel compilation and needs time to explore. Currently, we focus on solving the efficiency problem of parallel compilation, so have no time to explore it. |
|
@SparrowLii |
|
Actually we didn't encountered #50507 any more in in recent two years' development and tests, and didn;t got any feedback about this bug. So I guess we can just close this issue and this PR now |
|
Closing this as per #100987 (comment) |
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.
Fixes #50507
Updates #48685
In
evaluate_trait_predicate,DepGraph::with_anon_taskfunction is used to getEvaluationResultand generateDepNodeIndex. This is fine in the non-parallel compiler becauseDepGraph::with_anon_taskwill only be called once, writing the result toEvaluationCache.But in the parallel compiler, it is possible that the same two
evaluate_trait_predicatetasks start executing at the same time. There is no doubt that the two tasks will get the sameEvaluationResult, but they may produce differentDepNodeIndex.This PR specifies the way to generate hash values in
DepNodeby addingDepGraph::with_hash_taskto ensure that the same twoevaluate_trait_predicatetasks will get the sameDepNodeIndex