Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

incr.comp.: Hash more pieces of crate metadata to detect changes there. #41709

Merged
merged 2 commits into from
May 9, 2017

Conversation

michaelwoerister
Copy link
Member

@michaelwoerister michaelwoerister commented May 2, 2017

This PR adds incr. comp. hashes for non-Entry pieces of data in crate metadata.

The first part of it I like: EntryBuilder is refactored into the more generally applicable IsolatedEncoder which provides means of encoding something into metadata while also feeding the encoded data into an incr. comp. hash. We already did this for Entry, now we are doing it for various other pieces of data too, like the set of exported symbols and so on. The hashes generated there are persisted together with the per-Entry hashes and are also used for dep-graph dirtying the same way.

The second part of the PR I'm not entirely happy with: In order to make sure that we don't forget registering a read to the new DepNodes introduced here, I added the Tracked<T> struct. This struct wraps a value and requires a DepNode when accessing the wrapped value. This makes it harder to overlook adding read edges in the right places and works just fine.
However, crate metadata is already used in places where there is no tcx yet or even in places where no cnum has been assigned -- this makes it harder to apply this feature consistently or implement it ergonomically. The result is not too bad but there's a bit more code churn and a bit more opportunity to get something wrong than I would have liked. On the other hand, wrapping things in Tracked<T> already has revealed some bugs, so there's definitely some value in it.

This is still a work in progress:

  • I need to write some test cases.
  • Accessing the CodeMap should really be dependency tracked too, especially with the new path-remapping feature.

cc @nikomatsakis

@rust-highfive
Copy link
Collaborator

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

@bors
Copy link
Contributor

bors commented May 3, 2017

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

@aidanhs aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 3, 2017
@michaelwoerister michaelwoerister force-pushed the close-metadata-ich-holes branch 2 times, most recently from 1df872d to e3e4fde Compare May 4, 2017 12:39
@michaelwoerister michaelwoerister changed the title (WIP) incr.comp.: Hash more pieces of crate metadata to detect changes there. incr.comp.: Hash more pieces of crate metadata to detect changes there. May 4, 2017
@michaelwoerister
Copy link
Member Author

This should be ready for review now.

One interesting thing in the changes here is that some DepNode variants have a D as identifier, although a CrateNum would be sufficient. This is done because D fields are properly handled during DefPath retracing while CrateNums are not. By convention, I always used a DefId with the DefIndex set to CRATE_DEF_INDEX when only the CrateNum was interesting.

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned arielb1 May 4, 2017
@michaelwoerister
Copy link
Member Author

I'm not sure if the errors on Travis are legit. I'll need to test locally.

@bors
Copy link
Contributor

bors commented May 6, 2017

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

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 7, 2017
@michaelwoerister
Copy link
Member Author

All checks passing now.

@michaelwoerister michaelwoerister added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 8, 2017
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Gotta' few questions

// Hook up the codemap with a callback that allows it to register FileMap
// accesses with the dependency graph.
let cm_depgraph = dep_graph.clone();
let codemap_dep_tracking_callback = Box::new(move |filemap: &FileMap| {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see why you were unhappy.

This makes me wonder: I think that eventually we probably want some way to "hide" data in the tcx (e.g., maybe using privacy), so that it is only exposed via query. This would be a way to have stuff like random bits in the session be treated universally by the red-green system.

(For stuff that's hashed initially, like command-line-options, we might want to make some public accessor methods I guess.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, some refactorings like that would be nice.

}
DepNode::FileMap(..) => {
// These don't make a semantic
// difference, filter them out.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I follow this -- we use the SVH as a shorthand for "anything changed at all in this crate", you're saying that changes in the filemap shouldn't (e.g.) trigger downstream rebuilds when we are being conservative?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added this filter in order to appease the svh-related tests in run-pass. There are a few of them asserting that adding a comment (for example) does not change the SVH. These tests are still around from when the SVH tried to encode binary compatibility of crates, I think. The SVH has gotten a bit stricter since, but it's probably good that it's not too sensitive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, those tests seem pretty .. rotten. But ok.

@@ -491,6 +493,8 @@ impl Decodable for FileMap {
Ok(FileMap {
name: name,
name_was_remapped: name_was_remapped,
// `crate_of_origin` has to be set by the importer.
crate_of_origin: 0xEFFF_FFFF,
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, what's this? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry :)
It's a number that is unlikely to ever be a valid CrateNum. 0xFFFF_FFFF is already taken for some other special purpose. I should introduce an INVALID_CRATE const.

@nikomatsakis
Copy link
Contributor

ok well r=me

@michaelwoerister
Copy link
Member Author

@bors r=nikomatsakis

I added the INVALID_CRATE constant only to remember that it can't be used in libsyntax_pos :P.

@bors
Copy link
Contributor

bors commented May 9, 2017

📌 Commit 115602b has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented May 9, 2017

⌛ Testing commit 115602b with merge f3fc547...

bors added a commit that referenced this pull request May 9, 2017
…ikomatsakis

incr.comp.: Hash more pieces of crate metadata to detect changes there.

This PR adds incr. comp. hashes for non-`Entry` pieces of data in crate metadata.

The first part of it I like: `EntryBuilder` is refactored into the more generally applicable `IsolatedEncoder` which provides means of encoding something into metadata while also feeding the encoded data into an incr. comp. hash. We already did this for `Entry`, now we are doing it for various other pieces of data too, like the set of exported symbols and so on. The hashes generated there are persisted together with the per-`Entry` hashes and are also used for dep-graph dirtying the same way.

The second part of the PR I'm not entirely happy with: In order to make sure that we don't forget registering a read to the new `DepNodes` introduced here, I added the `Tracked<T>` struct. This struct wraps a value and requires a `DepNode` when accessing the wrapped value. This makes it harder to overlook adding read edges in the right places and works just fine.
However, crate metadata is already used in places where there is no `tcx` yet or even in places where no `cnum` has been assigned -- this makes it harder to apply this feature consistently or implement it ergonomically. The result is not too bad but there's a bit more code churn and a bit more opportunity to get something wrong than I would have liked. On the other hand, wrapping things in `Tracked<T>` already has revealed some bugs, so there's definitely some value in it.

This is still a work in progress:
- [x] I need to write some test cases.
- [x] Accessing the CodeMap should really be dependency tracked too, especially with the new path-remapping feature.

cc @nikomatsakis
@bors
Copy link
Contributor

bors commented May 9, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing f3fc547 to master...

@bors bors merged commit 115602b into rust-lang:master May 9, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 10, 2017
…ookup, r=nikomatsakis

ICH: Handle case of removed FileMaps.

This PR fixes a bug introduced in rust-lang#41709 where removing a source file between compilation sessions would cause an ICE:
https://travis-ci.org/rust-icci/crossbeam/jobs/230582234#L633

r? @nikomatsakis
frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 11, 2017
…ookup, r=nikomatsakis

ICH: Handle case of removed FileMaps.

This PR fixes a bug introduced in rust-lang#41709 where removing a source file between compilation sessions would cause an ICE:
https://travis-ci.org/rust-icci/crossbeam/jobs/230582234#L633

r? @nikomatsakis
frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 11, 2017
…ookup, r=nikomatsakis

ICH: Handle case of removed FileMaps.

This PR fixes a bug introduced in rust-lang#41709 where removing a source file between compilation sessions would cause an ICE:
https://travis-ci.org/rust-icci/crossbeam/jobs/230582234#L633

r? @nikomatsakis
@king6cong
Copy link
Contributor

#42101
this issue should be related to this commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants