incr.comp.: Compute fingerprint for all query results. #44364
Conversation
src/librustc/ty/fast_reject.rs
Outdated
| // A version of fast_reject::SimplifiedType that allows for stable hashing and | ||
| // especially can act as a stable sorting key. | ||
| #[derive(Eq, PartialEq, Ord, PartialOrd, Clone)] | ||
| pub enum StableSimplifiedType { |
There was a problem hiding this comment.
I don't quite understand why we need two versions here -- can we just use this version? Glancing around the code, I didn't see any obvious reason why not.
There was a problem hiding this comment.
Right. Changing this would make constructing simplified types slightly more expensive because of having to look up the DefPathHash. I'm not sure how I feel about that.
michaelwoerister
force-pushed
the
hash-all-the-things2
branch
from
82491d5
to
a0dfbfc
Compare
|
@bors r=nikomatsakis I've updated SimplifiedType as discussed on IRC. |
|
📌 Commit a0dfbfc has been approved by |
|
@bors r- Forgot to push one change ... |
michaelwoerister
force-pushed
the
hash-all-the-things2
branch
from
a0dfbfc
to
0cf6000
Compare
|
@bors r=nikomatsakis |
|
📌 Commit 0cf6000 has been approved by |
alexcrichton
added
the
S-waiting-on-bors
|
I pushed ed4ba22 which moves fingerprint computation into the DepGraph. This should allow to prevent using red/green incorrectly and keeping the implementation and data all in one place. |
michaelwoerister
added
S-waiting-on-review
|
☔ The latest upstream changes (presumably #44142) made this pull request unmergeable. Please resolve the merge conflicts. |
michaelwoerister
force-pushed
the
hash-all-the-things2
branch
from
ed4ba22
to
080fd7a
Compare
|
@nikomatsakis re-r? on that latest commit (080fd7a)? |
|
@bors r+ |
|
📌 Commit 080fd7a has been approved by |
|
⌛ Testing commit 080fd7a2d2937ca946e45a33923eec841cd57ce1 with merge 4eee05e7a6577344559d85238952b85955a56fc4... |
|
💔 Test failed - status-travis |
|
@nikomatsakis These are the kinds of errors I was talking about. It's probably some |
michaelwoerister
force-pushed
the
hash-all-the-things2
branch
from
080fd7a
to
1fa5c3f
Compare
|
@bors r=nikomatsakis |
|
📌 Commit 1fa5c3f has been approved by |
|
⌛ Testing commit 1fa5c3f0a44ddfbc32361d49e4a370854362ec92 with merge 583b1bde159d687087bfb2e41d6e16405a0a70e3... |
- Don't hash traits in scope as part of HIR hashing any more. - Some queries returned DefIndexes from other crates. - Provide a generic way of stably hashing maps (not used everywhere yet).
…n't have to be hashed in downstream crates.
michaelwoerister
force-pushed
the
hash-all-the-things2
branch
from
4f3e51e
to
74d6b85
Compare
|
@bors r=nikomatsakis |
|
📌 Commit 4961a8e has been approved by |
|
⌛ Testing commit 4961a8e with merge 8171608fe7c8b1c0b889b044f55dc07dc734787e... |
…s2, r=nikomatsakis incr.comp.: Compute fingerprint for all query results. This PR enables query result fingerprinting in incremental mode. This is an essential piece of infrastructure for red/green tracking. We don't do anything with the fingerprints yet but merging the infrastructure should protect it from bit-rotting and will make it easier to start measuring its performance impact (and thus let us determine if we should switch to a faster hashing algorithm rather sooner than later). Note, this PR also includes the changes from rust-lang#43887 which I'm therefore closing. No need to re-review the first commit though. r? @nikomatsakis
…s2, r=nikomatsakis incr.comp.: Compute fingerprint for all query results. This PR enables query result fingerprinting in incremental mode. This is an essential piece of infrastructure for red/green tracking. We don't do anything with the fingerprints yet but merging the infrastructure should protect it from bit-rotting and will make it easier to start measuring its performance impact (and thus let us determine if we should switch to a faster hashing algorithm rather sooner than later). Note, this PR also includes the changes from rust-lang#43887 which I'm therefore closing. No need to re-review the first commit though. r? @nikomatsakis
|
@michaelwoerister this may have regressed a bunch of incremental benchmarks, although perhaps that was expected? |
|
Yes, that's expected. Since we don't use red/green yet, we are paying the maximal price while not using any benefits. But things will get better, once we are actually putting those hashes to good use and there's also quite a bit of room for optimization (e.g. #41215). But one of the reasons I wanted this to land asap was that I wanted to start collecting data on the performance impact of universal result hashing. Given that we have the worst case right now, it doesn't seem that bad. |
|
I'm so glad this finally landed, btw |
|
@michaelwoerister ok sounds great to me! |
This PR enables query result fingerprinting in incremental mode. This is an essential piece of infrastructure for red/green tracking. We don't do anything with the fingerprints yet but merging the infrastructure should protect it from bit-rotting and will make it easier to start measuring its performance impact (and thus let us determine if we should switch to a faster hashing algorithm rather sooner than later).
Note, this PR also includes the changes from #43887 which I'm therefore closing. No need to re-review the first commit though.
r? @nikomatsakis