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.: Switch to red/green change tracking, remove legacy system. #44901

Merged
merged 12 commits into from Oct 4, 2017

Conversation

@michaelwoerister
Copy link
Contributor

commented Sep 28, 2017

This PR finally switches incremental compilation to red/green tracking and completely removes the legacy dependency graph implementation -- which includes a few quite costly passes that are simply not needed with the new system anymore.

There's still some documentation to be done and there's certainly still lots of optimizing and tuning ahead -- but the foundation for red/green is in place with this PR. This has been in the making for a long time :)

r? @nikomatsakis
cc @alexcrichton, @rust-lang/compiler

@@ -79,9 +79,7 @@ pub fn provide_local(providers: &mut Providers) {
providers.is_exported_symbol = |tcx, id| {
// FIXME(#42293) needs red/green to not break a bunch of incremental
// tests
tcx.dep_graph.with_ignore(|| {

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Sep 28, 2017

Member

:D

(you can probably delete the FIXME above this as well)

This comment has been minimized.

Copy link
@michaelwoerister

michaelwoerister Sep 28, 2017

Author Contributor

:) Yes.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Sep 28, 2017

🎉 🎉 🎉 🎉 🎉 🎉 🌮 🎉 🎉 🎉 🎉 🎉 🎉 🎉


profq_msg!(tcx,
ProfileQueriesMsg::QueryBegin(
span.clone(),

This comment has been minimized.

Copy link
@kennytm

kennytm Sep 28, 2017

Member

Should be span.data(). Why needs the clone()?

[00:06:30] error[E0308]: mismatched types
[00:06:30]    --> /checkout/src/librustc/ty/maps/plumbing.rs:654:25
[00:06:30]     |
[00:06:30] 654 |                         span.clone(),
[00:06:30]     |                         ^^^^^^^^^^^^ expected struct `syntax_pos::SpanData`, found struct `syntax_pos::Span`
[00:06:30] ...
[00:06:30] 710 |         DepKind::RegionScopeTree => { force!(region_scope_tree, def_id!()); }
[00:06:30]     |                                       ------------------------------------- in this macro invocation
[00:06:30]     |
[00:06:30]     = note: expected type `syntax_pos::SpanData`
[00:06:30]                found type `syntax_pos::Span`
[00:06:30]     = help: here are some functions which might fulfill your needs:
[00:06:30]             - .data()
...
[00:06:32] error: aborting due to 110 previous errors
[00:06:32] 
[00:06:32] error: Could not compile `rustc`.

This comment has been minimized.

Copy link
@michaelwoerister

michaelwoerister Sep 28, 2017

Author Contributor

Looks like this needs a rebase...

@sgrif

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2017

Speaking of red/green
screen shot 2017-09-28 at 3 11 24 pm

that's a lot of both. ;)

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Sep 29, 2017

Let me know if you want to check perf (try build + ping me).

@michaelwoerister michaelwoerister force-pushed the michaelwoerister:on-demand-eval branch from a1d7f6c to b88c694 Sep 29, 2017

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2017

@Mark-Simulacrum Awesome, thank you!

@michaelwoerister michaelwoerister force-pushed the michaelwoerister:on-demand-eval branch from b88c694 to 762b735 Sep 29, 2017

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2017

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2017

⌛️ Trying commit 762b735 with merge b669ce1...

bors added a commit that referenced this pull request Sep 29, 2017

Auto merge of #44901 - michaelwoerister:on-demand-eval, r=<try>
incr.comp.: Switch to red/green change tracking, remove legacy system.

This PR finally switches incremental compilation to [red/green tracking](#42293) and completely removes the legacy dependency graph implementation -- which includes a few quite costly passes that are simply not needed with the new system anymore.

There's still some documentation to be done and there's certainly still lots of optimizing and tuning ahead -- but the foundation for red/green is in place with this PR. This has been in the making for a long time `:)`

r? @nikomatsakis
cc @alexcrichton, @rust-lang/compiler

@michaelwoerister michaelwoerister referenced this pull request Sep 29, 2017

Closed

Tracking Issue for incr. comp. red/green testing #44716

3 of 3 tasks complete
@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2017

☀️ Test successful - status-travis
State: approved= try=True

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2017

Thanks, @Mark-Simulacrum! The link does not seem to work for me though at the moment (I see no results listed).

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Sep 29, 2017

Should work now. I think perf.rlo was just not updating for some reason...

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2017

Thanks, @Mark-Simulacrum! Looks promising.

@michaelwoerister michaelwoerister force-pushed the michaelwoerister:on-demand-eval branch from 762b735 to 563d30e Oct 2, 2017

DepNodeColor::Red
};

assert!(data.colors.borrow_mut().insert(key, color).is_none());

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Oct 3, 2017

Contributor

Nit: Although I am often tempted to write an assert! like this, I tend to think that putting crucial side-effect operations inside of an assert! is bad-style. Too easy to overlook. I think I'd prefer to see something like:

let old_value = data.colors.borrow_mut().insert(key, color);
assert!(old_value.is_none(), "key {:?} executed twice", key);

This comment has been minimized.

Copy link
@michaelwoerister

michaelwoerister Oct 3, 2017

Author Contributor

Will do.

@nikomatsakis
Copy link
Contributor

left a comment

r=me with comments and nits addressed

f)
}

if let Some(dep_node_index) = tcx.dep_graph.try_mark_green(tcx, &dep_node) {

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Oct 3, 2017

Contributor

I'm curious, why not make try_mark_green check the cache, so we can collapse these two ifs into one? They seem sort of like fundamentally the same case (marked green already vs can mark green now)

This comment has been minimized.

Copy link
@michaelwoerister

michaelwoerister Oct 3, 2017

Author Contributor

Wouldn't that mean that we always load the result from the cache, even if it is never asked for?

debug_assert!(tcx.dep_graph.is_green(dep_node_index));

// We don't do any caching yet, so recompute
let (result, diagnostics) = tcx.cycle_check(span, Query::$name(key), || {

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Oct 3, 2017

Contributor

Nit: feels like this maybe wants a subroutine, kind of duplicates the code above.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Oct 3, 2017

Contributor

this happens later, I saw, sorry :)

// We should never get into the situation of having to force this from the DepNode.
// Since we cannot reconstruct the query key, we would always end up having to evaluate
// the first caller of this query that *is* reconstructible. This might very well be
// compile_codegen_unit() in which case we'd lose all re-use.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Oct 3, 2017

Contributor

I find this comment pretty unclear. What is "this", in the first sentence, referring to? What makes CodegenUnit worse or different from other kinds of depnodes?

This comment has been minimized.

Copy link
@michaelwoerister

michaelwoerister Oct 3, 2017

Author Contributor

Hm, yeah, I seem to have moved this comment out of context without updating it. CodegenUnit is different because there's no way to reconstruct the query-key from the DepNode, so the "parent query" would have to be re-done. That parent query is compile_codegen_unit(), so we would always have to recompile the codegen unit just to find out that it would not have been necessary.

I'll update the comment and try to make it clearer.

match dep_node.kind {
// These are inputs that are expected to be pre-allocated and that
// should therefore always be red or green already
DepKind::AllLocalTraitImpls |

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Oct 3, 2017

Contributor

This match feels mildly unfortunate -- i.e., it feels like we ought to be able to derive this information from other macros and not require it to be "re-entered" here. But oh well, I haven't thought about how to actually do that. Might be nice to file a FIXME issue about it though.

This comment has been minimized.

Copy link
@michaelwoerister

michaelwoerister Oct 3, 2017

Author Contributor

Yeah, it's definitely not pretty. But I didn't want spend too much time on making this macro generated before we have a proof of concept implementation of red/green. I'll add a FIXME.

@@ -644,15 +664,26 @@ pub(super) struct CurrentDepGraph {
edges: IndexVec<DepNodeIndexNew, Vec<DepNodeIndexNew>>,
node_to_node_index: FxHashMap<DepNode, DepNodeIndexNew>,

anon_id_seed: Fingerprint,

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Oct 3, 2017

Contributor

this feels like it merits a nice comment

This comment has been minimized.

Copy link
@michaelwoerister

michaelwoerister Oct 3, 2017

Author Contributor

Agreed :)

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2017

Thanks for the review, @nikomatsakis!

@michaelwoerister michaelwoerister force-pushed the michaelwoerister:on-demand-eval branch from 563d30e to 0454a41 Oct 4, 2017

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2017

@bors r=nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2017

📌 Commit 0454a41 has been approved by nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2017

⌛️ Testing commit 0454a41 with merge d7e73e4...

bors added a commit that referenced this pull request Oct 4, 2017

Auto merge of #44901 - michaelwoerister:on-demand-eval, r=nikomatsakis
incr.comp.: Switch to red/green change tracking, remove legacy system.

This PR finally switches incremental compilation to [red/green tracking](#42293) and completely removes the legacy dependency graph implementation -- which includes a few quite costly passes that are simply not needed with the new system anymore.

There's still some documentation to be done and there's certainly still lots of optimizing and tuning ahead -- but the foundation for red/green is in place with this PR. This has been in the making for a long time `:)`

r? @nikomatsakis
cc @alexcrichton, @rust-lang/compiler
@bors

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2017

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

@bors bors merged commit 0454a41 into rust-lang:master Oct 4, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@alexcrichton

This comment has been minimized.

Copy link
Member

commented Oct 5, 2017

@michaelwoerister

I think we've recovered on all perf benchmarks except two. Everything else either improved since the original regression or was within 5% that I figured it would come out in the wash.

Interestingly I think those two benchmarks are the "from scratch build up incremental first time" benchmark, right?

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2017

perf.rlo is showing me only flat lines right now :/

I think I'll write up a tracking issue for possible performance improvements.

kennytm added a commit to kennytm/rust that referenced this pull request Oct 8, 2017

Rollup merge of rust-lang#45018 - michaelwoerister:fix-dep-node-debug…
…-recursion, r=eddyb

incr.comp.: Fix infinite recursion in Debug implementation of DepNode

Small bug fix. Depends on rust-lang#44901 to land first.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.