incr.comp.: Compute fingerprint for all query results. by michaelwoerister · Pull Request #44364 · rust-lang/rust (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation55 Commits12 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

This was referenced

Sep 6, 2017

Choose a reason for hiding this comment

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

Awesome. I left one nit -- I don't think we need to have two versions of simplified type. r=me otherwise.

// 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 {

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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.

@bors r=nikomatsakis

I've updated SimplifiedType as discussed on IRC.

📌 Commit a0dfbfc has been approved by nikomatsakis

@bors r-

Forgot to push one change ...

📌 Commit 0cf6000 has been approved by nikomatsakis

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.

☔ The latest upstream changes (presumably #44142) made this pull request unmergeable. Please resolve the merge conflicts.

Choose a reason for hiding this comment

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

r=me on latest commit

📌 Commit 080fd7a has been approved by nikomatsakis

⌛ Testing commit 080fd7a2d2937ca946e45a33923eec841cd57ce1 with merge 4eee05e7a6577344559d85238952b85955a56fc4...

@nikomatsakis These are the kinds of errors I was talking about. It's probably some DefIndex that should really be a DefId.

📌 Commit 1fa5c3f has been approved by nikomatsakis

⌛ 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.

📌 Commit 4961a8e has been approved by nikomatsakis

⌛ Testing commit 4961a8e with merge 8171608fe7c8b1c0b889b044f55dc07dc734787e...

alexcrichton added a commit to alexcrichton/rust that referenced this pull request

Sep 18, 2017

…s2, r=nikomatsakis

incr.comp.: Compute fingerprint for all query results.

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

bors added a commit that referenced this pull request

Sep 18, 2017

alexcrichton added a commit to alexcrichton/rust that referenced this pull request

Sep 18, 2017

…s2, r=nikomatsakis

incr.comp.: Compute fingerprint for all query results.

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

bors added a commit that referenced this pull request

Sep 18, 2017

Rollup of 11 pull requests

Successful merges: #44364, #44466, #44537, #44548, #44640, #44651, #44657, #44661, #44668, #44671, #44675
Failed merges:

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 :D 🎉 🎉

eddyb mentioned this pull request

Sep 20, 2017

Labels

S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.