Auto merge of #31304 - nikomatsakis:incr-comp-read-from-hir-map, r=michaelwoerister

This change also modifies the dep graph infrastructure to keep track of the number of active tasks, so that even if we are not building the full dep-graph, we still get assertions when there is no active task and one does something that would add a read/write edge. This is particularly helpful since, if the assertions are *not* active, you wind up with the error happening in the message processing thread, which is too late to know the correct backtrace.

~~Before landing, I need to do some performance measurements. Those are underway.~~

See measurements below. No real effect on time.

r? @michaelwoerister

Author: bors

src/librustc/dep_graph/mod.rs

@@ -40,8 +40,21 @@ pub enum DepNode {
     Hir(DefId),
 
     // Represents different phases in the compiler.
+    CrateReader,
+    CollectLanguageItems,
+    CheckStaticRecursion,
+    ResolveLifetimes,
+    RegionResolveCrate,
+    CheckLoops,
+    PluginRegistrar,
+    StabilityIndex,
     CollectItem(DefId),
     Coherence,
+    EffectCheck,
+    Liveness,
+    Resolve,
+    EntryPoint,
+    CheckEntryFn,
     CoherenceCheckImpl(DefId),
     CoherenceOverlapCheck(DefId),
     CoherenceOverlapCheckSpecial(DefId),
@@ -116,6 +129,13 @@ impl DepGraph {
         }
     }
 
+    /// True if we are actually building a dep-graph. If this returns false,
+    /// then the other methods on this `DepGraph` will have no net effect.
+    #[inline]
+    pub fn enabled(&self) -> bool {
+        self.data.enabled()
+    }
+
     pub fn query(&self) -> DepGraphQuery {
         self.data.query()
     }

src/librustc/dep_graph/thread.rs

@@ -19,6 +19,7 @@
 //! allocated (and both have a fairly large capacity).
 
 use rustc_data_structures::veccell::VecCell;
+use std::cell::Cell;
 use std::sync::mpsc::{self, Sender, Receiver};
 use std::thread;
 
@@ -39,6 +40,13 @@ pub enum DepMessage {
 pub struct DepGraphThreadData {
     enabled: bool,
 
+    // Local counter that just tracks how many tasks are pushed onto the
+    // stack, so that we still get an error in the case where one is
+    // missing. If dep-graph construction is enabled, we'd get the same
+    // error when processing tasks later on, but that's annoying because
+    // it lacks precision about the source of the error.
+    tasks_pushed: Cell<usize>,
+
     // current buffer, where we accumulate messages
     messages: VecCell<DepMessage>,
 
@@ -59,18 +67,26 @@ impl DepGraphThreadData {
         let (tx1, rx1) = mpsc::channel();
         let (tx2, rx2) = mpsc::channel();
         let (txq, rxq) = mpsc::channel();
+
         if enabled {
             thread::spawn(move || main(rx1, tx2, txq));
         }
+
         DepGraphThreadData {
             enabled: enabled,
+            tasks_pushed: Cell::new(0),
             messages: VecCell::with_capacity(INITIAL_CAPACITY),
             swap_in: rx2,
             swap_out: tx1,
             query_in: rxq,
         }
     }
 
+    #[inline]
+    pub fn enabled(&self) -> bool {
+        self.enabled
+    }
+
     /// Sends the current batch of messages to the thread. Installs a
     /// new vector of messages.
     fn swap(&self) {
@@ -100,13 +116,40 @@ impl DepGraphThreadData {
     /// the buffer is full, this may swap.)
     #[inline]
     pub fn enqueue(&self, message: DepMessage) {
+        // Regardless of whether dep graph construction is enabled, we
+        // still want to check that we always have a valid task on the
+        // stack when a read/write/etc event occurs.
+        match message {
+            DepMessage::Read(_) | DepMessage::Write(_) =>
+                if self.tasks_pushed.get() == 0 {
+                    self.invalid_message("read/write but no current task")
+                },
+            DepMessage::PushTask(_) | DepMessage::PushIgnore =>
+                self.tasks_pushed.set(self.tasks_pushed.get() + 1),
+            DepMessage::PopTask(_) | DepMessage::PopIgnore =>
+                self.tasks_pushed.set(self.tasks_pushed.get() - 1),
+            DepMessage::Query =>
+                (),
+        }
+
         if self.enabled {
-            let len = self.messages.push(message);
-            if len == INITIAL_CAPACITY {
-                self.swap();
-            }
+            self.enqueue_enabled(message);
         }
     }
+
+    // Outline this fn since I expect it may want to be inlined
+    // separately.
+    fn enqueue_enabled(&self, message: DepMessage) {
+        let len = self.messages.push(message);
+        if len == INITIAL_CAPACITY {
+            self.swap();
+        }
+    }
+
+    // Outline this too.
+    fn invalid_message(&self, string: &str) {
+        panic!("{}; see src/librustc/dep_graph/README.md for more information", string)
+    }
 }
 
 /// Definition of the depgraph thread.

src/librustc/front/map/collector.rs

@@ -79,6 +79,7 @@ impl<'ast> NodeCollector<'ast> {
 
     fn create_def(&mut self, node_id: NodeId, data: DefPathData) -> DefIndex {
         let parent_def = self.parent_def();
+        debug!("create_def(node_id={:?}, data={:?}, parent_def={:?})", node_id, data, parent_def);
         self.definitions.create_def_with_parent(parent_def, node_id, data)
     }
 
@@ -115,10 +116,13 @@ impl<'ast> Visitor<'ast> for NodeCollector<'ast> {
     /// deep walking so that we walk nested items in the context of
     /// their outer items.
     fn visit_nested_item(&mut self, item: ItemId) {
+        debug!("visit_nested_item: {:?}", item);
         self.visit_item(self.krate.item(item.id))
     }
 
     fn visit_item(&mut self, i: &'ast Item) {
+        debug!("visit_item: {:?}", i);
+
         // Pick the def data. This need not be unique, but the more
         // information we encapsulate into
         let def_data = match i.node {

src/librustc/front/map/mod.rs

@@ -14,6 +14,8 @@ use self::MapEntry::*;
 use self::collector::NodeCollector;
 pub use self::definitions::{Definitions, DefKey, DefPath, DefPathData, DisambiguatedDefPathData};
 
+use dep_graph::{DepGraph, DepNode};
+
 use middle::cstore::InlinedItem;
 use middle::cstore::InlinedItem as II;
 use middle::def_id::DefId;
@@ -228,19 +230,22 @@ impl<'ast> MapEntry<'ast> {
 
 /// Stores a crate and any number of inlined items from other crates.
 pub struct Forest {
-    pub krate: Crate,
+    krate: Crate,
+    pub dep_graph: DepGraph,
     inlined_items: TypedArena<InlinedParent>
 }
 
 impl Forest {
-    pub fn new(krate: Crate) -> Forest {
+    pub fn new(krate: Crate, dep_graph: DepGraph) -> Forest {
         Forest {
             krate: krate,
+            dep_graph: dep_graph,
             inlined_items: TypedArena::new()
         }
     }
 
     pub fn krate<'ast>(&'ast self) -> &'ast Crate {
+        self.dep_graph.read(DepNode::Krate);
         &self.krate
     }
 }
@@ -252,6 +257,10 @@ pub struct Map<'ast> {
     /// The backing storage for all the AST nodes.
     pub forest: &'ast Forest,
 
+    /// Same as the dep_graph in forest, just available with one fewer
+    /// deref. This is a gratuitious micro-optimization.
+    pub dep_graph: DepGraph,
+
     /// NodeIds are sequential integers from 0, so we can be
     /// super-compact by storing them in a vector. Not everything with
     /// a NodeId is in the map, but empirically the occupancy is about
@@ -267,6 +276,60 @@ pub struct Map<'ast> {
 }
 
 impl<'ast> Map<'ast> {
+    /// Registers a read in the dependency graph of the AST node with
+    /// the given `id`. This needs to be called each time a public
+    /// function returns the HIR for a node -- in other words, when it
+    /// "reveals" the content of a node to the caller (who might not
+    /// otherwise have had access to those contents, and hence needs a
+    /// read recorded). If the function just returns a DefId or
+    /// NodeId, no actual content was returned, so no read is needed.
+    fn read(&self, id: NodeId) {
+        self.dep_graph.read(self.dep_node(id));
+    }
+
+    fn dep_node(&self, id0: NodeId) -> DepNode {
+        let map = self.map.borrow();
+        let mut id = id0;
+        loop {
+            match map[id as usize] {
+                EntryItem(_, item) => {
+                    let def_id = self.local_def_id(item.id);
+                    // NB                          ^~~~~~~
+                    //
+                    // You would expect that `item.id == id`, but this
+                    // is not always the case. In particular, for a
+                    // ViewPath item like `use self::{mem, foo}`, we
+                    // map the ids for `mem` and `foo` to the
+                    // enclosing view path item. This seems mega super
+                    // ultra wrong, but then who am I to judge?
+                    // -nmatsakis
+                    return DepNode::Hir(def_id);
+                }
+
+                EntryForeignItem(p, _) |
+                EntryTraitItem(p, _) |
+                EntryImplItem(p, _) |
+                EntryVariant(p, _) |
+                EntryExpr(p, _) |
+                EntryStmt(p, _) |
+                EntryLocal(p, _) |
+                EntryPat(p, _) |
+                EntryBlock(p, _) |
+                EntryStructCtor(p, _) |
+                EntryLifetime(p, _) |
+                EntryTyParam(p, _) =>
+                    id = p,
+
+                RootCrate |
+                RootInlinedParent(_) => // FIXME(#2369) clarify story about cross-crate dep tracking
+                    return DepNode::Krate,
+
+                NotPresent =>
+                    panic!("Walking parents from `{}` led to `NotPresent` at `{}`", id0, id),
+            }
+        }
+    }
+
     pub fn num_local_def_ids(&self) -> usize {
         self.definitions.borrow().len()
     }
@@ -309,26 +372,30 @@ impl<'ast> Map<'ast> {
     }
 
     pub fn krate(&self) -> &'ast Crate {
-        &self.forest.krate
+        self.forest.krate()
     }
 
     /// Retrieve the Node corresponding to `id`, panicking if it cannot
     /// be found.
     pub fn get(&self, id: NodeId) -> Node<'ast> {
         match self.find(id) {
-            Some(node) => node,
+            Some(node) => node, // read recorded by `find`
             None => panic!("couldn't find node id {} in the AST map", id)
         }
     }
 
     pub fn get_if_local(&self, id: DefId) -> Option<Node<'ast>> {
-        self.as_local_node_id(id).map(|id| self.get(id))
+        self.as_local_node_id(id).map(|id| self.get(id)) // read recorded by `get`
     }
 
     /// Retrieve the Node corresponding to `id`, returning None if
     /// cannot be found.
     pub fn find(&self, id: NodeId) -> Option<Node<'ast>> {
-        self.find_entry(id).and_then(|x| x.to_node())
+        let result = self.find_entry(id).and_then(|x| x.to_node());
+        if result.is_some() {
+            self.read(id);
+        }
+        result
     }
 
     /// Similar to get_parent, returns the parent node id or id if there is no
@@ -459,22 +526,25 @@ impl<'ast> Map<'ast> {
             _ => None
         };
         match abi {
-            Some(abi) => abi,
+            Some(abi) => {
+                self.read(id); // reveals some of the content of a node
+                abi
+            }
             None => panic!("expected foreign mod or inlined parent, found {}",
                           self.node_to_string(parent))
         }
     }
 
     pub fn get_foreign_vis(&self, id: NodeId) -> Visibility {
-        let vis = self.expect_foreign_item(id).vis;
-        match self.find(self.get_parent(id)) {
+        let vis = self.expect_foreign_item(id).vis; // read recorded by `expect_foreign_item`
+        match self.find(self.get_parent(id)) { // read recorded by `find`
             Some(NodeItem(i)) => vis.inherit_from(i.vis),
             _ => vis
         }
     }
 
     pub fn expect_item(&self, id: NodeId) -> &'ast Item {
-        match self.find(id) {
+        match self.find(id) { // read recorded by `find`
             Some(NodeItem(item)) => item,
             _ => panic!("expected item, found {}", self.node_to_string(id))
         }
@@ -521,7 +591,7 @@ impl<'ast> Map<'ast> {
     }
 
     pub fn expect_expr(&self, id: NodeId) -> &'ast Expr {
-        match self.find(id) {
+        match self.find(id) { // read recorded by find
             Some(NodeExpr(expr)) => expr,
             _ => panic!("expected expr, found {}", self.node_to_string(id))
         }
@@ -571,6 +641,11 @@ impl<'ast> Map<'ast> {
     fn with_path_next<T, F>(&self, id: NodeId, next: LinkedPath, f: F) -> T where
         F: FnOnce(PathElems) -> T,
     {
+        // This function reveals the name of the item and hence is a
+        // kind of read. This is inefficient, since it walks ancestors
+        // and we are walking them anyhow, but whatever.
+        self.read(id);
+
         let parent = self.get_parent(id);
         let parent = match self.find_entry(id) {
             Some(EntryForeignItem(..)) => {
@@ -602,6 +677,7 @@ impl<'ast> Map<'ast> {
     /// Given a node ID, get a list of attributes associated with the AST
     /// corresponding to the Node ID
     pub fn attrs(&self, id: NodeId) -> &'ast [ast::Attribute] {
+        self.read(id); // reveals attributes on the node
         let attrs = match self.find(id) {
             Some(NodeItem(i)) => Some(&i.attrs[..]),
             Some(NodeForeignItem(fi)) => Some(&fi.attrs[..]),
@@ -655,6 +731,7 @@ impl<'ast> Map<'ast> {
     }
 
     pub fn span(&self, id: NodeId) -> Span {
+        self.read(id); // reveals span from node
         self.opt_span(id)
             .unwrap_or_else(|| panic!("AstMap.span: could not find span for id {:?}", id))
     }
@@ -833,6 +910,7 @@ pub fn map_crate<'ast>(forest: &'ast mut Forest) -> Map<'ast> {
 
     Map {
         forest: forest,
+        dep_graph: forest.dep_graph.clone(),
         map: RefCell::new(map),
         definitions: RefCell::new(definitions),
     }

src/librustc/middle/effect.rs

@@ -12,6 +12,7 @@
 //! `unsafe`.
 use self::RootUnsafeContext::*;
 
+use dep_graph::DepNode;
 use middle::def::Def;
 use middle::ty::{self, Ty};
 use middle::ty::MethodCall;
@@ -182,6 +183,8 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EffectCheckVisitor<'a, 'tcx> {
 }
 
 pub fn check_crate(tcx: &ty::ctxt) {
+    let _task = tcx.dep_graph.in_task(DepNode::EffectCheck);
+
     let mut visitor = EffectCheckVisitor {
         tcx: tcx,
         unsafe_context: UnsafeContext::new(SafeContext),