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.: Add new DepGraph implementation. #44772

Merged
merged 6 commits into from
Sep 24, 2017

Conversation

michaelwoerister
Copy link
Member

@michaelwoerister michaelwoerister commented Sep 22, 2017

This commits does a few things:

  1. It adds the new dep-graph implementation -- in addition to the old one. This way we can start testing the new implementation without switching all tests at once.
  2. It persists the new dep-graph (which includes query result fingerprints) to the incr. comp. caching directory and also loads this data.
  3. It removes support for loading fingerprints of metadata imported from other crates (except for when running autotests). This is not needed anymore with red/green. It could provide a performance advantage but that's yet to be determined. For now, as red/green is not fully implemented yet, the cross-crate incremental tests are disabled.

Note, this PR is based on top of soon-to-be-merged #44696 and only the last 4 commits are new:

- incr.comp.: Initial implemenation of append-only dep-graph. (c90147c)
- incr.comp.: Do some various cleanup. (8ce20c5)
- incr.comp.: Serialize and deserialize new DepGraph. (0e13c1a)
- incr.comp.: Remove support for loading metadata fingerprints. (270a134)
EDIT 2:
- incr.comp.: Make #[rustc_dirty/clean] test for fingerprint equality ... (d8f7ff9)

(EDIT: GH displays the commits in the wrong order for some reason)

Also note that this PR is expected to certainly result in performance regressions in the incr. comp. test cases, since we are adding quite a few things (a whole additional dep-graph, for example) without removing anything. End-to-end performance measurements will only make sense again after red/green is enabled and all the legacy tracking has been turned off.

EDIT 2: Pushed another commit that makes the #[rustc_dirty]/#[rustc_clean] based autotests compared query result fingerprints instead of testing DepNode existence.

@rust-highfive
Copy link
Collaborator

r? @eddyb

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

@michaelwoerister
Copy link
Member Author

r? @nikomatsakis

data.current.borrow_mut().read(v);

let mut current = data.current.borrow_mut();
debug_assert!(current.node_to_node_index.contains_key(&v),
Copy link
Contributor

@nikomatsakis nikomatsakis Sep 22, 2017

Choose a reason for hiding this comment

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

Why have both the debug assert and the bug!?

Also, what is going on here =) -- that is, when exactly are these getting allocated? This is not obvious to me.

@nikomatsakis
Copy link
Contributor

OK, I skimmed the commits. It all looks good to me. I left a question of something I don't understand, but r=me (I'm sure you can explain it to me async =)

@shepmaster shepmaster added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 22, 2017
@michaelwoerister
Copy link
Member Author

Why have both the debug assert and the bug!?

Oh, that seems to be an oversight.

Also, what is going on here =) -- that is, when exactly are these getting allocated? This is not obvious to me.

Nodes are only allocated via DepGraph::with_task and DepGraph::with_anon_task. For input nodes this is done eagerly at the moment, during HIR mapping (e.g.) and in load_dep_graph (e.g.). With full red/green this could be switched to lazy node allocation for all things that correspond to a query. Not for Hir nodes though, unless we make them query-based too. It does not make much of difference though. Both approaches are clean, IMO.

@bors
Copy link
Contributor

bors commented Sep 22, 2017

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

@michaelwoerister
Copy link
Member Author

@bors r=nikomatsakis

#44696 has landed and redundant debug_assert! removed. Let's give bors something to do over the weekend.

@bors
Copy link
Contributor

bors commented Sep 23, 2017

📌 Commit 89aec1e has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Sep 24, 2017

⌛ Testing commit 89aec1e with merge acb73db...

bors added a commit that referenced this pull request Sep 24, 2017
incr.comp.: Add new DepGraph implementation.

This commits does a few things:
1. It adds the new dep-graph implementation -- *in addition* to the old one. This way we can start testing the new implementation without switching all tests at once.
2. It persists the new dep-graph (which includes query result fingerprints) to the incr. comp. caching directory and also loads this data.
3. It removes support for loading fingerprints of metadata imported from other crates (except for when running autotests). This is not needed anymore with red/green. It could provide a performance advantage but that's yet to be determined. For now, as red/green is not fully implemented yet, the cross-crate incremental tests are disabled.

Note, this PR is based on top of soon-to-be-merged #44696 and only the last 4 commits are new:
```
- incr.comp.: Initial implemenation of append-only dep-graph. (c90147c)
- incr.comp.: Do some various cleanup. (8ce20c5)
- incr.comp.: Serialize and deserialize new DepGraph. (0e13c1a)
- incr.comp.: Remove support for loading metadata fingerprints. (270a134)
EDIT 2:
- incr.comp.: Make #[rustc_dirty/clean] test for fingerprint equality ... (d8f7ff9)
```
(EDIT: GH displays the commits in the wrong order for some reason)

Also note that this PR is expected to certainly result in performance regressions in the incr. comp. test cases, since we are adding quite a few things (a whole additional dep-graph, for example) without removing anything. End-to-end performance measurements will only make sense again after red/green is enabled and all the legacy tracking has been turned off.

EDIT 2: Pushed another commit that makes the `#[rustc_dirty]`/`#[rustc_clean]` based autotests compared query result fingerprints instead of testing `DepNode` existence.
@bors
Copy link
Contributor

bors commented Sep 24, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants