Turn -Z incremental-verify-ich on by default
#83007
Conversation
|
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 2595de5dfec7fd075f1f6789ac6f8f80c5ae51cc with merge e0c7bc5ed28e5116a211482f55d4561c7cf17451... |
|
☀️ Try build successful - checks-actions |
|
Queued e0c7bc5ed28e5116a211482f55d4561c7cf17451 with parent 066f01d, future comparison URL. |
|
Finished benchmarking try commit (e0c7bc5ed28e5116a211482f55d4561c7cf17451): 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
|
Huge regressions up to 330% for incr-unchanged and 58% for incr-patched. |
|
i think these perf regressions are still somewhat acceptable depending on how much we expect to catch with this 🤔 that's not a decision i want to make myself though, because this is not an area i have much expertise in. |
|
Is it possible to verify incremental compilation with a crater run? |
|
@matthiaskrgr: If you mean proactively trying to find ICEs caused by this change - this is issue rust-lang/crater#556, since we need to built a crate multiple times (the second time with an incremental cache). If you mean doing that instead of enabling this option, then no. There are an enormous number of combinations of (incremental cache, changed code), and we have no hope of testing any appreciable fraction of them using Crater. |
|
cc @rust-lang/wg-incr-comp |
|
The |
|
The |
|
@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 3223ba5f048da053ecd8d7c63fa853ae635f86a2 with merge 213521923e2df5e849eee21b917477d429078204... |
|
☀️ Try build successful - checks-actions |
|
Queued 213521923e2df5e849eee21b917477d429078204 with parent 61365c0, future comparison URL. |
|
Finished benchmarking try commit (213521923e2df5e849eee21b917477d429078204): 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
|
I wonder if it would be an option to just check one fingerprint in a thousand (in a deterministic way so things are reproducible). We would not have the performance regression and would have at least a chance of catching cases like these. I'm not sure I actually want to propose this |
|
As a result of this PR, we'll start getting some ICEs due to #80691 |
|
☀️ Try build successful - checks-actions |
|
Queued 122ea760160db6203a78fe170cd8011824ebe52b with parent 46a934a, future comparison URL. |
|
Finished benchmarking try commit (122ea760160db6203a78fe170cd8011824ebe52b): 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
|
The performance impact is much smaller if we only re-hash the result when the query gets re-computed (not when we just load an existing result from disk). I've confirmed that this causes the reproducer in #82920 to ICE instead of producing a miscompilation. @Mark-Simulacrum: I think we should land this PR in its current form (since the performance impact appears strictly lower than the previous perf run), since we know that it would have prevented at least one case of incr-comp-induced miscompilation. We can then consider whether or not we gain anything from re-hashing results loaded from disk. |
|
I think it would be good to add a note in the comments explaining the kind of bug that leads to a verification failure (likely similar to the comments on #82920 which narrowed down the cause of one bug). r=me with that done, and I agree that we can follow up with more extensive fixes or better performance in the future. |
Issue rust-lang#82920 showed that the kind of bugs caught by this flag have soundness implications. This causes performance regressions of up to 15.2% during incremental compilation, but this is necessary to catch miscompilations caused by bugs in query implementations.
Aaron1011
force-pushed
the
incr-verify-default
branch
from
b370bae to
7d7c81a
Compare
|
@bors r=Mark-Simulacrum |
|
📌 Commit 7d7c81a has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Issue #82920 showed that the kind of bugs caught by this flag have
soundness implications.