[WIP] Always verify incremental hash

[Aaron1011]

No description provided.

[rust-highfive]

r? @matthewjasper

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

[Aaron1011]

r? @ghost

[Aaron1011]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit 4c81dce603dbc5cdfc4bae156fca9ecdda12ae88 with merge f4a231fce8df9e7efa79597690860e9a0b28e97f...

[bors]

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

[rust-timer]

Queued f4a231fce8df9e7efa79597690860e9a0b28e97f with parent 2962e7c, future comparison URL.

[rust-timer]

Finished benchmarking try commit (f4a231fce8df9e7efa79597690860e9a0b28e97f): 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 rollup- to bors.

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 label: +S-waiting-on-review -S-waiting-on-perf

[jyn514]

@Aaron1011 what's the difference between this and #83007 ? Did that PR just miss one of the verify calls?

[Aaron1011]

@jyn514: #83007 was a more minimal version of this, to reduce the performance impact. Based on how many bugs that that PR has already found, I suspect that there might be more bugs lurking in the HashStable impls.

[Aaron1011]

I would really like to be able to land this, but the perf impact is terrible.

I had hoped that switching to HighwayHash would help, but it actually appears to decrease performance: #84229

[Mark-Simulacrum]

On larger crates the perf impact is only ~5%, so it's not too horrible. I think I'd personally like us to squash known bugs before landing this (e.g., the syn failure), so we're not just accumulating more bugs, but other than that it seems OK.

Is there a possibility we can optimize the incremental verification a bit?

It looks like it does several things:

• checks if dep node is green - this is a hash table lookup and an array lookup
• computes the hash - not really optimizable?
• looks up the old hash - again, this is a hash table lookup and an array lookup

At least the first of these seems.. not necessary? Both of the hash table lookups are also with the same key, I think, so we could optimize out the double hashing there with some manual work.

I don't know if it makes sense to do a perf run which just computes the hash but doesn't actually compare it - maybe wrapping in black box or something - to check if that's where all of the overhead is coming from. It probably is, but even shaving off a few dozen instructions from each call might make a nice difference here?

[Aaron1011]

I think I'd personally like us to squash known bugs before landing this (e.g., the syn failure), so we're not just accumulating more bugs

This change would only reveal existing bugs that were currently going undetected. If you think that this PR might cause additional ICEs, then I would argue that that's a reason to land it sooner - those bugs could be causing miscompilations.

[Mark-Simulacrum]

Yeah, that's a good point. FWIW I am personally willing to r+ this PR "as-is" (I'd like to see the description updated with some details on why) -- I do think the performance isn't ideal but it's also not that bad in most cases.

[Aaron1011]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit 2893057d60411eab7a939c81f8c081e3a7c61266 with merge d156ee0e47b013725257045a343c9f0863cae288...

[bors]

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

[rust-timer]

Queued d156ee0e47b013725257045a343c9f0863cae288 with parent b0c818c, future comparison URL.

[rust-timer]

Finished benchmarking try commit (d156ee0e47b013725257045a343c9f0863cae288): 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 rollup- to bors.

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 label: +S-waiting-on-review -S-waiting-on-perf

[bjorn3]

For clap-rs-check incr-patched this slows down death_check by 55x.

[jyn514]

One alternative to somehow improving the perf is to land this as is but for only one or two nightlies. Then people will use it and report ICEs but it won't be a regression for anyone on stable.

[Mark-Simulacrum]

Yeah, I'm not a big fan of temporary enabling, though we could try it.

I did have a thought about switching the hash algorithm for this verification (for example, to fxhash) -- in theory, that should still catch many cases of bugs: we won't error on collisions, but I would expect those to be fairly rare. I suspect this is pretty painful to implement though...

I've currently gone for an alternative solution, which is checking only a fraction of the hashes (in expectation, every 256th, though we can alter arbitrarily). This should pretty significantly reduce the cost of verification while still (I think) being deterministic and hopefully catching most bugs over time. We can play with the exact ratio here based on perf results.

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit be102b8eaeecec51aaa4ce688a1b92e19ef714f1 with merge 25fee14d1c50219333de853ad1d11d901c3f9a8e...

[bors]

☀️ Try build successful - checks-actions
Build commit: 25fee14d1c50219333de853ad1d11d901c3f9a8e (25fee14d1c50219333de853ad1d11d901c3f9a8e)

[rust-timer]

Queued 25fee14d1c50219333de853ad1d11d901c3f9a8e with parent 337e156, future comparison URL.

[rust-timer]

Finished benchmarking commit (25fee14d1c50219333de853ad1d11d901c3f9a8e): comparison url.

Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.

• Small improvement in instruction counts (up to -0.3% on incr-unchanged builds of issue-88862)
• Moderate regression in instruction counts (up to 1.0% on incr-unchanged builds of clap-rs)

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]

@bors try @rust-timer queue

Dropped the perf opt from this PR so we can more faithfully evaluate this change, and increased the number of affected hashes to 1/32nd.

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit 59dbb8c with merge 97d5f8ae1b0d7436d56cadb5e405aadafc64a86f...

[bors]

☀️ Try build successful - checks-actions
Build commit: 97d5f8ae1b0d7436d56cadb5e405aadafc64a86f (97d5f8ae1b0d7436d56cadb5e405aadafc64a86f)

[rust-timer]

Queued 97d5f8ae1b0d7436d56cadb5e405aadafc64a86f with parent dd757b9, future comparison URL.

[rust-timer]

Finished benchmarking commit (97d5f8ae1b0d7436d56cadb5e405aadafc64a86f): comparison url.

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

• Very large regression in instruction counts (up to 6.4% 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]

Closing in favor of #90361, which just has the same commits I force pushed here but is a fresh PR for fresh discussion.