Skip to content

Properly re-use def path hash in incremental mode#79721

Merged
bors merged 1 commit intorust-lang:masterfrom
Aaron1011:fix/reuse-def-path-hash
Dec 9, 2020
Merged

Properly re-use def path hash in incremental mode#79721
bors merged 1 commit intorust-lang:masterfrom
Aaron1011:fix/reuse-def-path-hash

Conversation

@Aaron1011
Copy link
Contributor

@Aaron1011 Aaron1011 commented Dec 5, 2020

Fixes #79661

In incremental compilation mode, we update a DefPathHash -> DefId
mapping every time we create a DepNode for a foreign DefId.
This mapping is written out to the on-disk incremental cache, and is
read by the next compilation session to allow us to lazily decode
DefIds.

When we decode a DepNode from the current incremental cache, we need
to ensure that any previously-recorded DefPathHash -> DefId mapping
gets recorded in the new mapping that we write out. However, PR #74967
didn't do this in all cases, leading to us being unable to decode a
DefPathHash in certain circumstances.

This PR refactors some of the code around DepNode deserialization to
prevent this kind of mistake from happening again.

@Aaron1011
Properly re-use def path hash in incremental mode
c294640 
Fixes rust-lang#79661

In incremental compilation mode, we update a `DefPathHash -> DefId`
mapping every time we create a `DepNode` for a foreign `DefId`.
This mapping is written out to the on-disk incremental cache, and is
read by the next compilation session to allow us to lazily decode
`DefId`s.

When we decode a `DepNode` from the current incremental cache, we need
to ensure that any previously-recorded `DefPathHash -> DefId` mapping
gets recorded in the new mapping that we write out. However, PR rust-lang#74967
didn't do this in all cases, leading to us being unable to decode a
`DefPathHash` in certain circumstances.

This PR refactors some of the code around `DepNode` deserialization to
prevent this kind of mistake from happening again.
@rust-highfive
Copy link
Contributor

rust-highfive commented Dec 5, 2020

r? @oli-obk

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 5, 2020
@Aaron1011
Copy link
Contributor Author

Aaron1011 commented Dec 5, 2020

cc @pnkfelix - you reviewed the initial implementation in #74967

bjorn3
bjorn3 reviewed Dec 5, 2020
View reviewed changes
@jyn514 jyn514 added A-incr-comp Area: Incremental compilation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 5, 2020
@dmit dmit mentioned this pull request Dec 9, 2020
Closed
wesleywiser
wesleywiser approved these changes Dec 9, 2020
View reviewed changes
Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

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

Just one small nit.

compiler/rustc_query_system/src/dep_graph/dep_node.rs
#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Encodable, Decodable)]
pub struct DepNode<K> {
pub kind: K,
// Important - whenever a `DepNode` is constructed, we need to make
Copy link
Member

@wesleywiser wesleywiser Dec 9, 2020

Choose a reason for hiding this comment

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

Nit: This comment seems like it applies to the DepNode type as a whole and not just the hash field so I would move this to above the struct declaration.

@wesleywiser
Copy link
Member

wesleywiser commented Dec 9, 2020

Let's do a perf run before merging

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

rust-timer commented Dec 9, 2020

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Dec 9, 2020

⌛ Trying commit c294640 with merge 5c94f1727f83b41b9a3c5b59145fdc2b1ddf8a02...

@bors
Copy link
Collaborator

bors commented Dec 9, 2020

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

@rust-timer
Copy link
Collaborator

rust-timer commented Dec 9, 2020

Queued 5c94f1727f83b41b9a3c5b59145fdc2b1ddf8a02 with parent 1700ca0, future comparison URL.

@rust-timer
Copy link
Collaborator

rust-timer commented Dec 9, 2020

Finished benchmarking try commit (5c94f1727f83b41b9a3c5b59145fdc2b1ddf8a02): 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 modify labels: +S-waiting-on-review -S-waiting-on-perf

@oli-obk
Copy link
Contributor

oli-obk commented Dec 9, 2020

r? @wesleywiser

@rust-highfive rust-highfive assigned wesleywiser and unassigned oli-obk Dec 9, 2020
@wesleywiser
Copy link
Member

wesleywiser commented Dec 9, 2020

@bors r+

Thanks for jumping on this so quickly @Aaron1011!

@bors
Copy link
Collaborator

bors commented Dec 9, 2020

📌 Commit c294640 has been approved by wesleywiser

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 9, 2020
@spastorino
Copy link
Member

spastorino commented Dec 9, 2020

@bors p=1

It fixes a potentially P-critical issue

@bors
Copy link
Collaborator

bors commented Dec 9, 2020

⌛ Testing commit c294640 with merge fa55f66...

@bors
Copy link
Collaborator

bors commented Dec 9, 2020

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing fa55f66 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 9, 2020
@bors bors merged commit fa55f66 into rust-lang:master Dec 9, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 9, 2020
This was referenced Dec 9, 2020
Merged
Merged
@yujingaya yujingaya mentioned this pull request Dec 10, 2020
Closed
@hellow554 hellow554 mentioned this pull request Dec 10, 2020
Closed
@vext01
Copy link
Contributor

vext01 commented Dec 11, 2020

Thank you for this fix :)

@camelid camelid mentioned this pull request Dec 11, 2020
Closed
@tgnottingham tgnottingham mentioned this pull request Dec 11, 2020
Merged
@rylev
Copy link
Member

rylev commented Dec 15, 2020

@Aaron1011 @wesleywiser this change had moderate regressions in several benchmarks. Since this was not directly addressed, I assume this was because the fix was important enough that the moderate regressions are acceptable?

@rylev rylev mentioned this pull request Dec 15, 2020
Merged
@wesleywiser
Copy link
Member

wesleywiser commented Dec 15, 2020

@rylev yeah, that's exactly it! The test cases that are regressed were significantly improved by the original pr (#74967) so overall, we're still ahead by quite a bit and this is necessary for correctness.

@jyn514 jyn514 mentioned this pull request Dec 15, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@wesleywiser wesleywiser wesleywiser approved these changes

Assignees

@wesleywiser wesleywiser

Labels

A-incr-comp Area: Incremental compilation merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.50.0

Development

Successfully merging this pull request may close these issues.

thread 'rustc' panicked at 'called Option::unwrap() on a None value', compiler/rustc_middle/src/ty/query/mod.rs:235:5