incr.comp.: Switch to red/green change tracking, remove legacy system. by michaelwoerister · Pull Request #44901 · 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
Conversation35 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 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
alexcrichton, cramertj, euclio, AlexEne, wesleywiser, CYBAI, matthewhammer, 0x7CFE, Connicpu, discosultan, and 16 more reacted with thumbs up emoji nikomatsakis, pepyakin, goodmanjonathan, QuietMisdreavus, kennytm, hanna-kruppe, aturon, est31, BurntSushi, MaloJaffre, and 65 more reacted with hooray emoji alexcrichton, goodmanjonathan, hanna-kruppe, aturon, BurntSushi, cramertj, euclio, , AlexEne, kellyjensen, and 17 more reacted with heart emoji
| @@ -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(| |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:D
(you can probably delete the FIXME above this as well)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:) Yes.
🎉 🎉 🎉 🎉 🎉 🎉 🌮 🎉 🎉 🎉 🎉 🎉 🎉 🎉
kennytm, cramertj, pylaligand, mattico, regexident, CYBAI, mgattozzi, Nemikolh, ark-, kingoflolz, and 11 more reacted with laugh emoji
| profq_msg!(tcx, |
| ProfileQueriesMsg::QueryBegin( |
| span.clone(), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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`.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this needs a rebase...
Speaking of red/green
that's a lot of both. ;)
Eijebong, kennytm, michaelwoerister, alex, mgattozzi, messense, fbstj, kingoflolz, durka, Evrey, and 10 more reacted with laugh emoji
Let me know if you want to check perf (try build + ping me).
bors added a commit that referenced this pull request
incr.comp.: Switch to red/green change tracking, remove legacy system.
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
pcwalton, little-dude, fbstj, samnardoni, and obust reacted with thumbs up emoji mattico, wesleywiser, alexcrichton, , and mystal reacted with hooray emoji aravind-pg, cramertj, wesleywiser, and reacted with heart emoji
Thanks, @Mark-Simulacrum! The link does not seem to work for me though at the moment (I see no results listed).
Should work now. I think perf.rlo was just not updating for some reason...
| DepNodeColor::Red |
|---|
| }; |
| assert!(data.colors.borrow_mut().insert(key, color).is_none()); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me with comments and nits addressed
| f) |
|---|
| } |
| if let Some(dep_node_index) = tcx.dep_graph.try_mark_green(tcx, &dep_node) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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), | |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: feels like this maybe wants a subroutine, kind of duplicates the code above.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 | |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>, |
| node_to_node_index: FxHashMap<DepNode, DepNodeIndexNew>, |
| anon_id_seed: Fingerprint, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feels like it merits a nice comment
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed :)
📌 Commit 0454a41 has been approved by nikomatsakis
bors added a commit that referenced this pull request
incr.comp.: Switch to red/green change tracking, remove legacy system.
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
This was referenced
Oct 4, 2017
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?
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
…-recursion, r=eddyb
incr.comp.: Fix infinite recursion in Debug implementation of DepNode
Small bug fix. Depends on rust-lang#44901 to land first.