Skip to content

Commit

Permalink
Auto merge of #90361 - Mark-Simulacrum:always-verify, r=michaelwoerister
Browse files Browse the repository at this point in the history
Enable verification for 1/32th of queries loaded from disk

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](#84227 (comment)), 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`
  • Loading branch information
bors committed Nov 8, 2021
2 parents 3e3890c + 49e7c99 commit 495322d
Showing 1 changed file with 14 additions and 1 deletion.
15 changes: 14 additions & 1 deletion compiler/rustc_query_system/src/query/plumbing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -518,9 +518,22 @@ where
prof_timer.finish_with_query_invocation_id(dep_node_index.into());

if let Some(result) = result {

This comment has been minimized.

Copy link
@elvismunyikiiru

elvismunyikiiru Nov 8, 2021

FRAGMENT

let prev_fingerprint = tcx
.dep_context()
.dep_graph()
.prev_fingerprint_of(dep_node)
.unwrap_or(Fingerprint::ZERO);
// If `-Zincremental-verify-ich` is specified, re-hash results from
// the cache and make sure that they have the expected fingerprint.
if unlikely!(tcx.dep_context().sess().opts.debugging_opts.incremental_verify_ich) {
//
// If not, we still seek to verify a subset of fingerprints loaded
// from disk. Re-hashing results is fairly expensive, so we can't
// currently afford to verify every hash. This subset should still
// give us some coverage of potential bugs though.
let try_verify = prev_fingerprint.as_value().1 % 32 == 0;
if unlikely!(
try_verify || tcx.dep_context().sess().opts.debugging_opts.incremental_verify_ich
) {
incremental_verify_ich(*tcx.dep_context(), &result, dep_node, query);
}

Expand Down

1 comment on commit 495322d

@elvismunyikiiru
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FRAGMENT

Please sign in to comment.