Enable verification for 1/32th of queries loaded from disk

[Mark-Simulacrum]

This is a limited enabling of incremental verification for query results loaded from disk, which previously did not run without -Zincremental-verify-ich. If enabled for all queries, we see a probably unacceptable hit of ~50% in the worst case, so this pairs back the verification to a more limited set based on the hash key.

Per collected perf results, this is a regression of at most 7% on coercions opt incr-unchanged, and typically less than 0.5% on other benchmarks (largely limited to incr-unchanged). I believe this is acceptable performance to land, and we can either ratchet it up or down fairly easily.

We have no real sense of whether this will lead to a large amount of assertions in the wild, but since those assertions may lead to miscompilations today, it seems potentially warranted. We have a good bit of lead time until the next stable release, though the holiday season will also start soon; we may wish to discuss the timing of enabling this and weigh the desire to prevent (possible) miscompilations against assertions.

cc @rust-lang/wg-incr-comp

[rust-highfive]

r? @michaelwoerister

(rust-highfive has picked a reviewer for you, use r? to override)

[michaelwoerister]

Thanks, @Mark-Simulacrum! I'm in favor of doing this but I wonder if we should discuss it with the wider compiler team.

[michaelwoerister]

x % 32 == 0 and x & 0b11111 == 0 compile to the same assembly. I think x % 32 is easier to read.

(see https://play.rust-lang.org/?
version=stable&mode=release&edition=2021&gist=9fac0eb991024affb003e069ded6d334)

[michaelwoerister]

I think we should wrap the entire condition in unlikely!() as in if unlikely!(try_verify || ...) -- because we already know that try_verify is unlikely to be true too.

[michaelwoerister]

Nominating for discussion in the triage meeting. I think this is a good idea but it does cause a bit of a performance regression and, more importantly, we want to be prepared for new unstable fingerprint errors if they start to show up.

[michaelwoerister]

This would address (partially) #85783.

[est31]

How stable is the behaviour? If there is an assertion error but it's only in 1 of 32 cases, it seems to make bugs harder reproducible for users, especially as the user tries around and it doesn't reproduce.

[Mark-Simulacrum]

HashStable should be reliable enough that this is deterministically triggered, I believe, but it's also the case that anyone seeking to reproduce the behavior likely should enable -Zincremental-verify-ich to just verify all hashes (accepting the performance hit). I don't think that should break hash consistency for most cases.

[michaelwoerister]

If you get an assertion error that you want to reproduce you can re-run with -Zverify-incremental-ich so that the check gets run unconditionally. The ICE error message should maybe mention that.

[est31]

I'm just worried because even now a lot of the incr comp assertion failures manifest themselves to users say once per day of usage, or maybe even more rarely. The bug reports are formulated similarly vaguely, and one has to rely on the bug being hit often enough by enough people so that it can be pinned down. A flag that enables unconditional incr comp assertion checking is definitely helpful and gives an actionable tool to debug an incr comp issue.

[Mark-Simulacrum]

I don't think we should mention -Z flags in the error because in practice most users probably aren't on nightly (and checking for ability to use feature flags feels excessive). It's also true that AFAIK reproducing these is pretty difficult regardless, as @est31 mentions, and from what I've seen in bug reports if we don't get the incremental state packaged up by the user we often need to just "intelligently guess" for the most part. So I don't in practice think end-users being able to reproduce is likely to be that important: we may get less bug reports in the sense that a recompilation fixes it, but that's already often the case today (e.g., if you happen to save in your editor after starting the main compilation), so I don't know that this is a meaningful shift. Once we get that reproduction having rustc contributors/developers work with the code while minifying with the preexisting -Z flag seems fine.

I also am fairly sure that this should be deterministic in the sense that we're verifying the same results on each compilation, so it shouldn't be much of a change from before.

[michaelwoerister]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 49e7c99 with merge 4bcc562cf44466242182a4c92eae6f3002308e17...

[michaelwoerister]

Since we didn't get to discuss this in the triage meeting two times in a row now, I'm planning to just r+ this (if the performance hit is indeed acceptable). We are still some time away from the next release and this can be quickly reverted if necessary. We'll just have to keep an eye out for bug reports.

[bors]

☀️ Try build successful - checks-actions
Build commit: 4bcc562cf44466242182a4c92eae6f3002308e17 (4bcc562cf44466242182a4c92eae6f3002308e17)

[rust-timer]

Queued 4bcc562cf44466242182a4c92eae6f3002308e17 with parent 27143a9, future comparison URL.

[rust-timer]

Finished benchmarking commit (4bcc562cf44466242182a4c92eae6f3002308e17): comparison url.

Summary: This change led to very large relevant regressions 😿 in compiler performance.

• Very large regression in instruction counts (up to 5.8% on incr-unchanged builds of coercions)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

[Mark-Simulacrum]

Results do not significantly differ from the previous ones (see PR description for the link), and I think they're OK. We can reduce the impact with a smaller fraction being processed (for example, 1/64), but I think that there's a certain level of diminishing returns -- most affected benchmarks are incr-unchanged, and the few println or other patchsets are only slightly affected. I think the potential for catching bugs is higher the more we check, so I'd rather not reduce without strong motivation.

[michaelwoerister]

OK, I'm going to approve this now under the assumption that we will turn the check off again if it causes trouble (e.g. many false positives that aren't easily fixed).

This PR is known to introduce a large regression (up to 5.78% instruction count) for a small number of benchmarks, and small regression for many other incr-unchanged and incr-patched. This is the cost of doing additional verification to catch incremental bugs stemming from buggy serialization (see #85783).

@rustbot label: +perf-regression-triaged

The PR is still nominated for discussion in the triage meeting. It's trivial to revert so I don't want to block it any longer.

@bors r+

[bors]

📌 Commit 49e7c99 has been approved by michaelwoerister

[bors]

⌛ Testing commit 49e7c99 with merge 495322d...

[bors]

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing 495322d to master...

[rust-timer]

Finished benchmarking commit (495322d): comparison url.

Summary: This change led to very large relevant regressions 😿 in compiler performance.

• Very large regression in instruction counts (up to 6.3% on incr-unchanged builds of coercions)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression