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

migrate all "incremental tasks" to "on-demand queries" #40746

Closed
50 of 63 tasks
nikomatsakis opened this issue Mar 22, 2017 · 3 comments
Closed
50 of 63 tasks

migrate all "incremental tasks" to "on-demand queries" #40746

nikomatsakis opened this issue Mar 22, 2017 · 3 comments
Labels
A-incr-comp Area: Incremental compilation C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Mar 22, 2017

As part of the plan to adopt the red-green algorithm for incremental, we want to make it so that users don't spawn tasks themselves (i.e., with in_task and with_task). Rather, they perform queries (via the new on-demand query mechanism). To this end, we have to migrate all of the existing tasks into queries.

I've started going through the full list of tasks (derived by rg \.{in,with}_task\() and making notes on how to migrate each one. This issue is intended to serve as a "punchlist" for these migrations. Note that the filenames and line-numbers cited here may have changed.

Queries that compute shared values

These are tasks that ought to be converted into new queries that yield results. Right now, these functions often return a value that winds up stored in the tcx. These values would be removed and replaced with maps.

Example PR: #40746

  • src/librustc_privacy/lib.rs:1187: let _task = tcx.dep_graph.in_task(DepNode::Privacy);
  • src/librustc/middle/reachable.rs:365: let _task = tcx.dep_graph.in_task(DepNode::Reachability);
  • src/librustc_borrowck/borrowck/mod.rs:68: tcx.dep_graph.with_task(DepNode::BorrowCheck(body_owner_def_id)
    • writes to used_mut_nodes, so probably wants to return this set
    • the lint would be modified to request the borrowck query for the current function
  • src/librustc/middle/region.rs:1260: let _task = map.dep_graph.in_task(DepNode::RegionResolveCrate);

The following tasks currently execute before a tcx is built, but they could be easily converted into
queries that are requested after tcx is built. The main reason they are the way they are was to
avoid a gratuitious refcell (but using the refcell map seems fine):

  • src/librustc/middle/entry.rs:60: let _task = hir_map.dep_graph.in_task(DepNode::EntryPoint);
  • src/librustc/middle/resolve_lifetime.rs:262: let _task = hir_map.dep_graph.in_task(DepNode::ResolveLifetimes);
  • src/librustc/middle/lang_items.rs:239: let _task = map.dep_graph.in_task(DepNode::CollectLanguageItems);

"Always execute" passes

Lints

see #42511

Stability

  • The stability index is somewhat complex:
    • src/librustc/middle/stability.rs:386: let _task = tcx.dep_graph.in_task(DepNode::StabilityIndex);
    • src/librustc/middle/stability.rs:400: let _task = hir_map.dep_graph.in_task(DepNode::StabilityIndex);
    • src/librustc/middle/stability.rs:664: let _task = tcx.dep_graph.in_task(DepNode::StabilityIndex);
  • From what I can tell, what we do is to construct a Stability::Index for the current crate early on,
    store this in the tcx in a RefCell, and then gradually grow it to cache the results from other crates
  • Two possible translations:
    • each crate has a stability index, and you can query for stability_index(krate)
    • have very granular queries like stability(def_id), deprecation(def_id), and deprecation_entry(def_id)
      • for the local crate, these would request the stability-index, extract the relevant data
        • good example for uncached queries
      • for the extern crate, we'd go to metadata and cache it in the DepTrackingMap or something?

This may be worth breaking into a distinct issue.

Visitor stuff

This visitor pattern should be changed. Currently, we do something like:

tcx.visit_all_item_likes_in_krate(DepNode::RvalueCheck, &mut rvcx.as_deep_visitor())

This will walk all item-like and invoke a task on each one, with the id DepNode::RvalueCheck(_). In many of these cases though we can just convert these into calls to tcx.hir.krate().visit_all_item_likes(...). This will not spawn new tasks, but rather traverse all the items in one big task. This is actually fine most of the time, and in particular whenever the following conditions are met:

  • we don't want to incrementally skip any of this work (yet?)
  • we do not produce values that others need (which is generally true)

In other words, the compiler currently has a rather more fine-grained task structure that we necessarily want, and so we can often just flatten the structure and then come back later to make it deeper.

  • src/librustc/dep_graph/visit.rs:41: let _task = self.tcx.dep_graph.in_task(task_id.clone());
  • src/librustc/dep_graph/visit.rs:51: let _task = self.tcx.dep_graph.in_task(task_id.clone());
  • src/librustc/dep_graph/visit.rs:61: let _task = self.tcx.dep_graph.in_task(task_id.clone());

The following invocations of visit_all_item_likes_in_crate() can probably just be flattened:

  • src/librustc_passes/rvalues.rs:27: tcx.visit_all_item_likes_in_krate(DepNode::RvalueCheck, &mut rvcx.as_deep_visitor());
  • src/librustc_passes/consts.rs:461: tcx.visit_all_item_likes_in_krate(DepNode::CheckConst,
  • src/librustc/middle/intrinsicck.rs:28: tcx.visit_all_item_likes_in_krate(DepNode::IntrinsicCheck, &mut visitor.as_deep_visitor());
  • src/librustc/middle/stability.rs:427: tcx.visit_all_item_likes_in_krate(DepNode::StabilityCheck, &mut checker.as_deep_visitor());
  • src/librustc_typeck/coherence/overlap.rs:27: tcx.visit_all_item_likes_in_krate(DepNode::CoherenceOverlapCheckSpecial, &mut overlap);
  • src/librustc_typeck/coherence/orphan.rs:22: tcx.visit_all_item_likes_in_krate(DepNode::CoherenceOrphanCheck, &mut orphan);
  • src/librustc_typeck/coherence/inherent.rs:352: tcx.visit_all_item_likes_in_krate(DepNode::CoherenceCheckImpl,
  • src/librustc_typeck/coherence/inherent.rs:354: tcx.visit_all_item_likes_in_krate(DepNode::CoherenceOverlapCheckSpecial,
  • src/librustc_typeck/impl_wf_check.rs:66: tcx.visit_all_item_likes_in_krate(DepNode::WfCheck, &mut ImplWfCheck { tcx: tcx });
  • src/librustc_typeck/collect.rs:92: tcx.visit_all_item_likes_in_krate(DepNode::CollectItem, &mut visitor.as_deep_visitor());
  • src/librustc_typeck/check/mod.rs:570: tcx.visit_all_item_likes_in_krate(DepNode::WfCheck, &mut visit.as_deep_visitor());
  • src/librustc_typeck/check/mod.rs:576: tcx.visit_all_item_likes_in_krate(DepNode::TypeckItemType,
  • src/librustc_const_eval/check_match.rs:61: tcx.visit_all_item_likes_in_krate(DepNode::MatchCheck,

The following might require a bit more thought:

  • src/librustc_mir/mir_map.rs:71: tcx.visit_all_item_likes_in_krate(DepNode::Mir, &mut GatherCtors {
  • src/librustc_typeck/variance/terms.rs:112: tcx.visit_all_item_likes_in_krate(|def_id| VarianceDepNode(def_id), &mut terms_cx);
  • src/librustc_typeck/variance/constraints.rs:68: tcx.visit_all_item_likes_in_krate(VarianceDepNode, &mut constraint_cx);

Trait selection

The general plan here is use the anonymous nodes infrastructure. Sketchy details are available in this outline. This is worth breaking into a distinct issue.

  • src/librustc/traits/select.rs:413: let _task = tcx.dep_graph.in_task(dep_node);
    • Trait selection. This really wants to use the "anonymous nodes"

Internal graph manipulation

These can be ignored for now, as they are internal to the dep-graph mechanism and so we'll update them once the other work is done.

  • src/librustc_incremental/persist/load.rs:439: let _task = tcx.dep_graph.in_task(target_node);
  • src/librustc_incremental/persist/load.rs:195: tcx.dep_graph.with_task(n, (), (), create_node);
  • src/librustc/dep_graph/dep_tracking_map.rs:134: let _task = graph.in_task(M::to_dep_node(&key));
  • src/librustc/dep_graph/graph.rs:110: let _task = self.in_task(key);
  • src/librustc/ty/maps.rs:271: let _task = tcx.dep_graph.in_task(Self::to_dep_node(&key));

Docs that will need to be rewritten or deleted

  • src/librustc/dep_graph/README.md:67:You set the current task by calling dep_graph.in_task(node). For example:
  • src/librustc/dep_graph/README.md:70:let _task = tcx.dep_graph.in_task(DepNode::Privacy);
  • src/librustc/dep_graph/README.md:83:let _n1 = tcx.dep_graph.in_task(DepNode::N1);
  • src/librustc/dep_graph/README.md:84:let _n2 = tcx.dep_graph.in_task(DepNode::N2);
  • src/librustc/dep_graph/README.md:102:let _n1 = tcx.dep_graph.in_task(DepNode::N1);
  • src/librustc/dep_graph/README.md:104:let _n2 = tcx.dep_graph.in_task(DepNode::N2);
  • src/librustc/dep_graph/README.md:167: let task = tcx.dep_graph.in_task(DepNode::ItemSignature(def_id));

Uncategorized

I didn't look at these yet. =)

  • src/librustc_typeck/check/mod.rs:543: tcx.dep_graph.with_task(DepNode::TypeckBodiesKrate, tcx, (), check_item_bodies_task);
  • src/librustc_mir/mir_map.rs:43: tcx.dep_graph.with_task(DepNode::MirKrate, tcx, (), build_mir_for_crate_task);
  • src/librustc_trans/base.rs:1125: tcx.dep_graph.with_task(dep_node,
  • src/librustc_trans/base.rs:1146: tcx.dep_graph.with_task(dep_node,
  • src/librustc/ty/mod.rs:1655: let _task = tcx.dep_graph.in_task(DepNode::SizedConstraint(self.did));
  • src/librustc_typeck/lib.rs:277: let _task = tcx.dep_graph.in_task(DepNode::CheckEntryFn);
  • src/librustc/mir/transform.rs:123: let _task = tcx.dep_graph.in_task(DepNode::Mir(def_id));
  • src/librustc_mir/transform/qualify_consts.rs:969: let _task = tcx.dep_graph.in_task(DepNode::Mir(def_id));
  • src/librustc_mir/transform/inline.rs:65: let _task = tcx.dep_graph.in_task(DepNode::Mir(def_id));
  • src/librustc_mir/transform/inline.rs:89: let _task = tcx.dep_graph.in_task(DepNode::Mir(def_id));
  • src/librustc_mir/transform/inline.rs:187: let _task = self.tcx.dep_graph.in_task(DepNode::Mir(callsite.caller));
  • src/librustc_mir/transform/inline.rs:256: let _task = self.tcx.dep_graph.in_task(DepNode::Mir(def_id));
  • src/librustc_mir/transform/inline.rs:433: let _task = self.tcx.dep_graph.in_task(DepNode::Mir(callsite.caller));
  • src/librustc_driver/derive_registrar.rs:19: let _task = hir_map.dep_graph.in_task(DepNode::PluginRegistrar);
  • src/librustc_plugin/build.rs:47: let _task = hir_map.dep_graph.in_task(DepNode::PluginRegistrar);
  • src/librustc_trans/trans_item.rs:77: let _task = ccx.tcx().dep_graph.in_task(DepNode::TransCrateItem(def_id)); // (*)
  • src/librustc_trans/trans_item.rs:93: let _task = ccx.tcx().dep_graph.in_task(
  • src/librustc_trans/base.rs:1034: let _task = tcx.dep_graph.in_task(DepNode::TransCrate);
  • src/librustc_trans/back/link.rs:199: let _task = sess.dep_graph.in_task(DepNode::LinkBinary);
  • src/librustc_metadata/index_builder.rs:121: let _task = self.tcx.dep_graph.in_task(DepNode::MetaData(id));
@steveklabnik steveklabnik added A-incr-comp Area: Incremental compilation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 23, 2017
@steveklabnik steveklabnik changed the title [incremental] migrate all "incremental tasks" to "on-demand queries" migrate all "incremental tasks" to "on-demand queries" Mar 23, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this issue Mar 25, 2017
…cess-levels, r=eddyb

"on-demandify" privacy and access levels

r? @eddyb
cc @cramertj rust-lang#40746
frewsxcv added a commit to frewsxcv/rust that referenced this issue Mar 25, 2017
…cess-levels, r=eddyb

"on-demandify" privacy and access levels

r? @eddyb
cc @cramertj rust-lang#40746
@aochagavia
Copy link
Contributor

@nikomatsakis I would like to help with this. Are there any beginner-friendly issues that you would be willing to mentor?

frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 6, 2017
…komatsakis

On demandify reachability

cc rust-lang#40746

I tried following this guidance from rust-lang#40746:
> The following tasks currently execute before a tcx is built, but they could be easily converted into queries that are requested after tcx is built. The main reason they are the way they are was to avoid a gratuitious refcell (but using the refcell map seems fine)...

but the result of moving `region_maps` out of `TyCtxt` and into a query caused a lot of churn, and seems like it could potentially result in a rather large performance hit, since it means a dep-graph lookup on every use of `region_maps` (rather than just a field access). Possibly `TyCtxt` could store a `RefCell<Option<RegionMap>>` internally and use that to prevent repeat lookups, but that feels like it's duplicating the work of the dep-graph. @nikomatsakis What did you have in mind for this?
bors added a commit that referenced this issue Apr 7, 2017
On demandify reachability

cc #40746

I tried following this guidance from #40746:
> The following tasks currently execute before a tcx is built, but they could be easily converted into queries that are requested after tcx is built. The main reason they are the way they are was to avoid a gratuitious refcell (but using the refcell map seems fine)...

but the result of moving `region_maps` out of `TyCtxt` and into a query caused a lot of churn, and seems like it could potentially result in a rather large performance hit, since it means a dep-graph lookup on every use of `region_maps` (rather than just a field access). Possibly `TyCtxt` could store a `RefCell<Option<RegionMap>>` internally and use that to prevent repeat lookups, but that feels like it's duplicating the work of the dep-graph. @nikomatsakis What did you have in mind for this?
TimNN added a commit to TimNN/rust that referenced this issue Apr 12, 2017
…c-loops, r=eddyb

remove unnecessary tasks

Remove various unnecessary tasks. All of these are "always execute" tasks that don't do any writes to tracked state (or else an assert would trigger, anyhow). In some cases, they issue lints or errors, but we''ll deal with that -- and anyway side-effects outside of a task don't cause problems for anything that I can see.

The one non-trivial refactoring here is the borrowck conversion, which adds the requirement to go from a `DefId` to a `BodyId`. I tried to make a useful helper here.

r? @eddyb

cc rust-lang#40746
cc @cramertj @michaelwoerister
@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Apr 12, 2017

@aochagavia I will create an issue just for you pulling out some of the work here. =)

frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 18, 2017
…visitors, r=eddyb

convert calls to `visit_all_item_likes_in_krate`

We no longer need to track the tasks in these cases since these
particular tasks have no outputs (except, potentially, errors...)  and
they always execute.

cc rust-lang#40746

r? @eddyb
frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 18, 2017
…visitors, r=eddyb

convert calls to `visit_all_item_likes_in_krate`

We no longer need to track the tasks in these cases since these
particular tasks have no outputs (except, potentially, errors...)  and
they always execute.

cc rust-lang#40746

r? @eddyb
@Mark-Simulacrum Mark-Simulacrum added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Jul 27, 2017
@alexcrichton
Copy link
Member

I think this is effectively done, so closing. If this is in error, though, please reopen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-incr-comp Area: Incremental compilation C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants