Trying to address an incremental compilation issues #126409
Conversation
|
rustbot has assigned @michaelwoerister. Use |
rustbot
added
A-query-system
|
Thanks for the PR, @pacak! I'll take a look asap. |
|
Looking at https://github.com/rust-lang/rust/issues?q=is%3Aissue+is%3Aopen+label%3AA-incr-comp - I think this pull request should help to close quite a few of open incremental compilation issues. I'll go though the list later. |
|
The I'm wondering if there is an obvious way to write a test for this. |
|
Your test case from #107226 (comment) seems good to add. Making the I'm wondering about some of the error stack traces though, e.g. here. Normally, we should not encounter a removed |
|
There's probably a bug sitting somewhere, but not having random panic when
you try to print stuff makes sense. If anything - we can look into adding
an assertion that traversed nodes are safe with some better error if they
are not.
…On Mon, Jun 17, 2024, 10:38 Michael Woerister ***@***.***> wrote:
Your test case from #107226 (comment)
<#107226 (comment)>
seems good to add.
Making the DefPathHash -> (Local)DefId mappings return an Option seems
good, especially with respect to not crashing the DepNode debug impl.
I'm wondering about some of the error stack traces though, e.g. here
<#125945 (comment)>.
Normally, we should not encounter a removed DefPathHash during
red-green-marking because at that point we should have come upon a red node
signifying the removal and thus stopped going any further. But in these
backtraces we do encounter these nodes and the query traces all have
something with trait-solving in them. Trait-solving is the only area where
we do manual dependency tracking and I'm wondering if this points to a bug
in that manual tracking or if gracefully failing to reconstruct the
dep-node is the valid way to handle this.
—
Reply to this email directly, view it on GitHub
<#126409 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAQFI5FICFR2RM4MPGM62DZH3YHBAVCNFSM6AAAAABJINBXHSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZTGU4TQMRRG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Agreed 🙂 |
The way it is implemented currently try_force_from_dep_node returns true as long as there's a function to force the query. It wasn't this way from the beginning, earlier version was producing forcing result and it was changed in rust-lang#89978, I couldn't find any comments addressing this change. One way it can fail is by failing to recover the query in DepNodeParams::recover - when we are trying to query something that no longer exists in the current environment
|
I rebased on top of master, addressed your suggestions to naming/comments, added a test and confirmed that the test fails before changes to At some point I'll look into requests in trait solving code, but it might take time. |
There was a problem hiding this comment.
Thank you, @pacak, this is a great find! Would you mind adding the doc comment suggested below? Then we can merge this.
Thank you! That would be great. Please ping me when you do so. |
local_def_path_hash_to_def_id is used by Debug impl for DepNode and it looks for DefPathHash inside the current compilation. During incremental compilation we are going through nodes that belong to a previous compilation and might not be present and a simple attempt to print such node with tracing::debug (try_mark_parent_green does it for example) results in a otherwise avoidable panic Panic was added in rust-lang#82183, specifically in 2b60338, with a comment "We only use this mapping for cases where we know that it must succeed.", but I'm not sure if this property holds when we traverse nodes from the old compilation in order to figure out if they are valid or not
Done. I believe this pull request should close a lot of existing incremental compilation tickets, I listed two of them but there's more and they don't have reproducers so I wasn't able to test them. Do you think they can be closed as well? I don't have a concrete list, but going though tickets labeled with incremental compilation ICEs and looking at stack traces shows them.
Sure. |
Let's merge the PR and once it is in nightly, ask for confirmation on the respective issues. |
|
Let's not roll this up, for better bisectability. |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (1d96de2): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary 3.0%, secondary 3.5%)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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 690.663s -> 691.418s (0.11%) |
This pull request contains two independent changes, one makes it so when
try_force_from_dep_nodefails to recover a query - it marks the node as "red" instead of "green" and the second one makes Debug impl forDepNodeless panicky if it encounters something from the previous compilation that doesn't map to anything in the current one.I'm not 100% confident that this is the correct approach, but so far I managed to find a bunch of comments suggesting that some things are allowed to fail in a certain way and changes I made are allowing for those things to fail this way and it fixes all the small reproducers I managed to find.
Compilation panic this pull request avoids is caused by an automatically generated code on an associated type and it is not happening if something else marks it as outdated first (or close like that, but scenario is quite obscure).
Fixes #107226
Fixes #125367