Auto merge of #50528 - whitfin:issue-50508, r=michaelwoerister

Remove attribute_cache from CrateMetadata

This PR will fix #50508 by removing the `attribute_cache` from the `CrateMetadata` struct. Seeing as performance was referenced in the original issue, I also cleaned up a `self.entry(node_id);` call which might have occasionally happened redundantly.

r? @michaelwoerister

8 files changed, 86 insertions, 153 deletions

diff --git a/src/librustc_incremental/persist/dirty_clean.rs b/src/librustc_incremental/persist/dirty_clean.rs
index 8f406161831..1549ef5e928 100644
--- a/src/librustc_incremental/persist/dirty_clean.rs
+++ b/src/librustc_incremental/persist/dirty_clean.rs
@@ -453,16 +453,20 @@ impl<'a, 'tcx> DirtyCleanVisitor<'a, 'tcx> {
         out
     }
 
-    fn dep_nodes(&self, labels: &Labels, def_id: DefId) -> Vec<DepNode> {
-        let mut out = Vec::with_capacity(labels.len());
+    fn dep_nodes<'l>(
+        &self,
+        labels: &'l Labels,
+        def_id: DefId
+    ) -> impl Iterator<Item = DepNode> + 'l {
         let def_path_hash = self.tcx.def_path_hash(def_id);
-        for label in labels.iter() {
-            match DepNode::from_label_string(label, def_path_hash) {
-                Ok(dep_node) => out.push(dep_node),
-                Err(()) => unreachable!(),
-            }
-        }
-        out
+        labels
+            .iter()
+            .map(move |label| {
+                match DepNode::from_label_string(label, def_path_hash) {
+                    Ok(dep_node) => dep_node,
+                    Err(()) => unreachable!(),
+                }
+            })
     }
 
     fn dep_node_str(&self, dep_node: &DepNode) -> String {
diff --git a/src/librustc_incremental/persist/work_product.rs b/src/librustc_incremental/persist/work_product.rs
index d0c7766cbae..c0ccbd67a31 100644
--- a/src/librustc_incremental/persist/work_product.rs
+++ b/src/librustc_incremental/persist/work_product.rs
@@ -28,7 +28,6 @@ pub fn copy_cgu_workproducts_to_incr_comp_cache_dir(
     if sess.opts.incremental.is_none() {
         return None
     }
-    let work_product_id = WorkProductId::from_cgu_name(cgu_name);
 
     let saved_files: Option<Vec<_>> =
         files.iter()
@@ -63,6 +62,7 @@ pub fn copy_cgu_workproducts_to_incr_comp_cache_dir(
         saved_files,
     };
 
+    let work_product_id = WorkProductId::from_cgu_name(cgu_name);
     Some((work_product_id, work_product))
 }
 
diff --git a/src/librustc_metadata/creader.rs b/src/librustc_metadata/creader.rs
index 6c1f72f5f9c..2467d5cf97c 100644
--- a/src/librustc_metadata/creader.rs
+++ b/src/librustc_metadata/creader.rs
@@ -58,9 +58,9 @@ pub struct CrateLoader<'a> {
 fn dump_crates(cstore: &CStore) {
     info!("resolved crates:");
     cstore.iter_crate_data(|_, data| {
-        info!("  name: {}", data.name());
+        info!("  name: {}", data.root.name);
         info!("  cnum: {}", data.cnum);
-        info!("  hash: {}", data.hash());
+        info!("  hash: {}", data.root.hash);
         info!("  reqd: {:?}", *data.dep_kind.lock());
         let CrateSource { dylib, rlib, rmeta } = data.source.clone();
         dylib.map(|dl| info!("  dylib: {}", dl.0.display()));
@@ -113,7 +113,7 @@ impl<'a> CrateLoader<'a> {
             if data.name != name { return }
 
             match hash {
-                Some(hash) if *hash == data.hash() => { ret = Some(cnum); return }
+                Some(hash) if *hash == data.root.hash => { ret = Some(cnum); return }
                 Some(..) => return,
                 None => {}
             }
@@ -172,9 +172,9 @@ impl<'a> CrateLoader<'a> {
 
         // Check for conflicts with any crate loaded so far
         self.cstore.iter_crate_data(|_, other| {
-            if other.name() == root.name && // same crate-name
-               other.disambiguator() == root.disambiguator &&  // same crate-disambiguator
-               other.hash() != root.hash { // but different SVH
+            if other.root.name == root.name && // same crate-name
+               other.root.disambiguator == root.disambiguator &&  // same crate-disambiguator
+               other.root.hash != root.hash { // but different SVH
                 span_fatal!(self.sess, span, E0523,
                         "found two different crates with name `{}` that are \
                          not distinguished by differing `-C metadata`. This \
@@ -214,7 +214,6 @@ impl<'a> CrateLoader<'a> {
         let root = if root.is_some() { root } else { &crate_paths };
 
         let Library { dylib, rlib, rmeta, metadata } = lib;
-
         let cnum_map = self.resolve_crate_deps(root, &crate_root, &metadata, cnum, span, dep_kind);
 
         let dependencies: Vec<CrateNum> = cnum_map.iter().cloned().collect();
@@ -243,13 +242,12 @@ impl<'a> CrateLoader<'a> {
             cnum,
             dependencies: Lock::new(dependencies),
             codemap_import_info: RwLock::new(vec![]),
-            attribute_cache: Lock::new([Vec::new(), Vec::new()]),
             dep_kind: Lock::new(dep_kind),
             source: cstore::CrateSource {
                 dylib,
                 rlib,
                 rmeta,
-            },
+            }
         };
 
         let cmeta = Lrc::new(cmeta);
@@ -345,7 +343,7 @@ impl<'a> CrateLoader<'a> {
         if locate_ctxt.triple == &self.sess.opts.target_triple {
             let mut result = LoadResult::Loaded(library);
             self.cstore.iter_crate_data(|cnum, data| {
-                if data.name() == root.name && root.hash == data.hash() {
+                if data.root.name == root.name && root.hash == data.root.hash {
                     assert!(locate_ctxt.hash.is_none());
                     info!("load success, going to previous cnum: {}", cnum);
                     result = LoadResult::Previous(cnum);
@@ -642,15 +640,14 @@ impl<'a> CrateLoader<'a> {
         let mut needs_panic_runtime = attr::contains_name(&krate.attrs,
                                                           "needs_panic_runtime");
 
-        let sess = self.sess;
         self.cstore.iter_crate_data(|cnum, data| {
             needs_panic_runtime = needs_panic_runtime ||
-                                  data.needs_panic_runtime(sess);
-            if data.is_panic_runtime(sess) {
+                                  data.root.needs_panic_runtime;
+            if data.root.panic_runtime {
                 // Inject a dependency from all #![needs_panic_runtime] to this
                 // #![panic_runtime] crate.
                 self.inject_dependency_if(cnum, "a panic runtime",
-                                          &|data| data.needs_panic_runtime(sess));
+                                          &|data| data.root.needs_panic_runtime);
                 runtime_found = runtime_found || *data.dep_kind.lock() == DepKind::Explicit;
             }
         });
@@ -687,11 +684,11 @@ impl<'a> CrateLoader<'a> {
 
         // Sanity check the loaded crate to ensure it is indeed a panic runtime
         // and the panic strategy is indeed what we thought it was.
-        if !data.is_panic_runtime(self.sess) {
+        if !data.root.panic_runtime {
             self.sess.err(&format!("the crate `{}` is not a panic runtime",
                                    name));
         }
-        if data.panic_strategy() != desired_strategy {
+        if data.root.panic_strategy != desired_strategy {
             self.sess.err(&format!("the crate `{}` does not have the panic \
                                     strategy `{}`",
                                    name, desired_strategy.desc()));
@@ -699,7 +696,7 @@ impl<'a> CrateLoader<'a> {
 
         self.sess.injected_panic_runtime.set(Some(cnum));
         self.inject_dependency_if(cnum, "a panic runtime",
-                                  &|data| data.needs_panic_runtime(self.sess));
+                                  &|data| data.root.needs_panic_runtime);
     }
 
     fn inject_sanitizer_runtime(&mut self) {
@@ -794,7 +791,7 @@ impl<'a> CrateLoader<'a> {
                                        PathKind::Crate, dep_kind);
 
                 // Sanity check the loaded crate to ensure it is indeed a sanitizer runtime
-                if !data.is_sanitizer_runtime(self.sess) {
+                if !data.root.sanitizer_runtime {
                     self.sess.err(&format!("the crate `{}` is not a sanitizer runtime",
                                            name));
                 }
@@ -817,7 +814,7 @@ impl<'a> CrateLoader<'a> {
                                    PathKind::Crate, dep_kind);
 
             // Sanity check the loaded crate to ensure it is indeed a profiler runtime
-            if !data.is_profiler_runtime(self.sess) {
+            if !data.root.profiler_runtime {
                 self.sess.err(&format!("the crate `profiler_builtins` is not \
                                         a profiler runtime"));
             }
@@ -834,7 +831,7 @@ impl<'a> CrateLoader<'a> {
         let mut needs_allocator = attr::contains_name(&krate.attrs,
                                                       "needs_allocator");
         self.cstore.iter_crate_data(|_, data| {
-            needs_allocator = needs_allocator || data.needs_allocator(self.sess);
+            needs_allocator = needs_allocator || data.root.needs_allocator;
         });
         if !needs_allocator {
             self.sess.injected_allocator.set(None);
@@ -876,7 +873,7 @@ impl<'a> CrateLoader<'a> {
             None
         };
         self.cstore.iter_crate_data(|_, data| {
-            if !data.has_global_allocator() {
+            if !data.root.has_global_allocator {
                 return
             }
             match global_allocator {
@@ -885,14 +882,14 @@ impl<'a> CrateLoader<'a> {
                                             conflicts with this global \
                                             allocator in: {}",
                                            other_crate,
-                                           data.name()));
+                                           data.root.name));
                 }
                 Some(None) => {
                     self.sess.err(&format!("the #[global_allocator] in this \
                                             crate conflicts with global \
-                                            allocator in: {}", data.name()));
+                                            allocator in: {}", data.root.name));
                 }
-                None => global_allocator = Some(Some(data.name())),
+                None => global_allocator = Some(Some(data.root.name)),
             }
         });
         if global_allocator.is_some() {
@@ -954,7 +951,7 @@ impl<'a> CrateLoader<'a> {
             // error.
             let mut allocator = None;
             self.cstore.iter_crate_data(|_, data| {
-                if allocator.is_none() && data.has_default_lib_allocator() {
+                if allocator.is_none() && data.root.has_default_lib_allocator {
                     allocator = Some(data.clone());
                 }
             });
@@ -1030,9 +1027,9 @@ impl<'a> CrateLoader<'a> {
                 self.sess.err(&format!("the crate `{}` cannot depend \
                                         on a crate that needs {}, but \
                                         it depends on `{}`",
-                                       self.cstore.get_crate_data(krate).name(),
+                                       self.cstore.get_crate_data(krate).root.name,
                                        what,
-                                       data.name()));
+                                       data.root.name));
             }
         }
 
diff --git a/src/librustc_metadata/cstore.rs b/src/librustc_metadata/cstore.rs
index f2d2d090e0a..763563eabe0 100644
--- a/src/librustc_metadata/cstore.rs
+++ b/src/librustc_metadata/cstore.rs
@@ -13,18 +13,14 @@
 
 use schema;
 
-use rustc::hir::def_id::{CRATE_DEF_INDEX, CrateNum, DefIndex};
+use rustc::hir::def_id::{CrateNum, DefIndex};
 use rustc::hir::map::definitions::DefPathTable;
-use rustc::hir::svh::Svh;
 use rustc::middle::cstore::{DepKind, ExternCrate, MetadataLoader};
-use rustc::session::{Session, CrateDisambiguator};
-use rustc_target::spec::PanicStrategy;
 use rustc_data_structures::indexed_vec::IndexVec;
 use rustc::util::nodemap::{FxHashMap, NodeMap};
 
 use rustc_data_structures::sync::{Lrc, RwLock, Lock};
-use syntax::{ast, attr};
-use syntax::edition::Edition;
+use syntax::ast;
 use syntax::ext::base::SyntaxExtension;
 use syntax::symbol::Symbol;
 use syntax_pos;
@@ -69,7 +65,6 @@ pub struct CrateMetadata {
     pub cnum: CrateNum,
     pub dependencies: Lock<Vec<CrateNum>>,
     pub codemap_import_info: RwLock<Vec<ImportedFileMap>>,
-    pub attribute_cache: Lock<[Vec<Option<Lrc<[ast::Attribute]>>>; 2]>,
 
     pub root: schema::CrateRoot,
 
@@ -177,66 +172,3 @@ impl CStore {
         self.extern_mod_crate_map.borrow().get(&emod_id).cloned()
     }
 }
-
-impl CrateMetadata {
-    pub fn name(&self) -> Symbol {
-        self.root.name
-    }
-    pub fn hash(&self) -> Svh {
-        self.root.hash
-    }
-    pub fn disambiguator(&self) -> CrateDisambiguator {
-        self.root.disambiguator
-    }
-
-    pub fn needs_allocator(&self, sess: &Session) -> bool {
-        let attrs = self.get_item_attrs(CRATE_DEF_INDEX, sess);
-        attr::contains_name(&attrs, "needs_allocator")
-    }
-
-    pub fn has_global_allocator(&self) -> bool {
-        self.root.has_global_allocator.clone()
-    }
-
-    pub fn has_default_lib_allocator(&self) -> bool {
-        self.root.has_default_lib_allocator.clone()
-    }
-
-    pub fn is_panic_runtime(&self, sess: &Session) -> bool {
-        let attrs = self.get_item_attrs(CRATE_DEF_INDEX, sess);
-        attr::contains_name(&attrs, "panic_runtime")
-    }
-
-    pub fn needs_panic_runtime(&self, sess: &Session) -> bool {
-        let attrs = self.get_item_attrs(CRATE_DEF_INDEX, sess);
-        attr::contains_name(&attrs, "needs_panic_runtime")
-    }
-
-    pub fn is_compiler_builtins(&self, sess: &Session) -> bool {
-        let attrs = self.get_item_attrs(CRATE_DEF_INDEX, sess);
-        attr::contains_name(&attrs, "compiler_builtins")
-    }
-
-    pub fn is_sanitizer_runtime(&self, sess: &Session) -> bool {
-        let attrs = self.get_item_attrs(CRATE_DEF_INDEX, sess);
-        attr::contains_name(&attrs, "sanitizer_runtime")
-    }
-
-    pub fn is_profiler_runtime(&self, sess: &Session) -> bool {
-        let attrs = self.get_item_attrs(CRATE_DEF_INDEX, sess);
-        attr::contains_name(&attrs, "profiler_runtime")
-    }
-
-    pub fn is_no_builtins(&self, sess: &Session) -> bool {
-        let attrs = self.get_item_attrs(CRATE_DEF_INDEX, sess);
-        attr::contains_name(&attrs, "no_builtins")
-    }
-
-    pub fn panic_strategy(&self) -> PanicStrategy {
-        self.root.panic_strategy.clone()
-    }
-
-    pub fn edition(&self) -> Edition {
-        self.root.edition
-    }
-}
diff --git a/src/librustc_metadata/cstore_impl.rs b/src/librustc_metadata/cstore_impl.rs
index c8f25f935e9..4691027e3b1 100644
--- a/src/librustc_metadata/cstore_impl.rs
+++ b/src/librustc_metadata/cstore_impl.rs
@@ -170,17 +170,17 @@ provide! { <'tcx> tcx, def_id, other, cdata,
     is_mir_available => { cdata.is_item_mir_available(def_id.index) }
 
     dylib_dependency_formats => { Lrc::new(cdata.get_dylib_dependency_formats()) }
-    is_panic_runtime => { cdata.is_panic_runtime(tcx.sess) }
-    is_compiler_builtins => { cdata.is_compiler_builtins(tcx.sess) }
-    has_global_allocator => { cdata.has_global_allocator() }
-    is_sanitizer_runtime => { cdata.is_sanitizer_runtime(tcx.sess) }
-    is_profiler_runtime => { cdata.is_profiler_runtime(tcx.sess) }
-    panic_strategy => { cdata.panic_strategy() }
+    is_panic_runtime => { cdata.root.panic_runtime }
+    is_compiler_builtins => { cdata.root.compiler_builtins }
+    has_global_allocator => { cdata.root.has_global_allocator }
+    is_sanitizer_runtime => { cdata.root.sanitizer_runtime }
+    is_profiler_runtime => { cdata.root.profiler_runtime }
+    panic_strategy => { cdata.root.panic_strategy }
     extern_crate => {
         let r = Lrc::new(*cdata.extern_crate.lock());
         r
     }
-    is_no_builtins => { cdata.is_no_builtins(tcx.sess) }
+    is_no_builtins => { cdata.root.no_builtins }
     impl_defaultness => { cdata.get_impl_defaultness(def_id.index) }
     reachable_non_generics => {
         let reachable_non_generics = tcx
@@ -209,9 +209,9 @@ provide! { <'tcx> tcx, def_id, other, cdata,
             DefId { krate: def_id.krate, index }
         })
     }
-    crate_disambiguator => { cdata.disambiguator() }
-    crate_hash => { cdata.hash() }
-    original_crate_name => { cdata.name() }
+    crate_disambiguator => { cdata.root.disambiguator }
+    crate_hash => { cdata.root.hash }
+    original_crate_name => { cdata.root.name }
 
     extra_filename => { cdata.root.extra_filename.clone() }
 
@@ -457,17 +457,17 @@ impl CrateStore for cstore::CStore {
 
     fn crate_disambiguator_untracked(&self, cnum: CrateNum) -> CrateDisambiguator
     {
-        self.get_crate_data(cnum).disambiguator()
+        self.get_crate_data(cnum).root.disambiguator
     }
 
     fn crate_hash_untracked(&self, cnum: CrateNum) -> hir::svh::Svh
     {
-        self.get_crate_data(cnum).hash()
+        self.get_crate_data(cnum).root.hash
     }
 
     fn crate_edition_untracked(&self, cnum: CrateNum) -> Edition
     {
-        self.get_crate_data(cnum).edition()
+        self.get_crate_data(cnum).root.edition
     }
 
     /// Returns the `DefKey` for a given `DefId`. This indicates the
@@ -519,7 +519,7 @@ impl CrateStore for cstore::CStore {
         } else if data.name == "proc_macro" &&
                   self.get_crate_data(id.krate).item_name(id.index) == "quote" {
             let ext = SyntaxExtension::ProcMacro(Box::new(::proc_macro::__internal::Quoter),
-                                                 data.edition());
+                                                 data.root.edition);
             return LoadedMacro::ProcMacro(Lrc::new(ext));
         }
 
diff --git a/src/librustc_metadata/decoder.rs b/src/librustc_metadata/decoder.rs
index 53d1ff15627..8af4649ed5f 100644
--- a/src/librustc_metadata/decoder.rs
+++ b/src/librustc_metadata/decoder.rs
@@ -557,12 +557,14 @@ impl<'a, 'tcx> CrateMetadata {
                        -> &'tcx ty::AdtDef {
         let item = self.entry(item_id);
         let did = self.local_def_id(item_id);
-        let kind = match item.kind {
-            EntryKind::Enum(_) => ty::AdtKind::Enum,
-            EntryKind::Struct(_, _) => ty::AdtKind::Struct,
-            EntryKind::Union(_, _) => ty::AdtKind::Union,
+
+        let (kind, repr) = match item.kind {
+            EntryKind::Enum(repr) => (ty::AdtKind::Enum, repr),
+            EntryKind::Struct(_, repr) => (ty::AdtKind::Struct, repr),
+            EntryKind::Union(_, repr) => (ty::AdtKind::Union, repr),
             _ => bug!("get_adt_def called on a non-ADT {:?}", did),
         };
+
         let variants = if let ty::AdtKind::Enum = kind {
             item.children
                 .decode(self)
@@ -573,12 +575,6 @@ impl<'a, 'tcx> CrateMetadata {
         } else {
             vec![self.get_variant(&item, item_id)]
         };
-        let (kind, repr) = match item.kind {
-            EntryKind::Enum(repr) => (ty::AdtKind::Enum, repr),
-            EntryKind::Struct(_, repr) => (ty::AdtKind::Struct, repr),
-            EntryKind::Union(_, repr) => (ty::AdtKind::Union, repr),
-            _ => bug!("get_adt_def called on a non-ADT {:?}", did),
-        };
 
         tcx.alloc_adt_def(did, kind, variants, repr)
     }
@@ -880,34 +876,22 @@ impl<'a, 'tcx> CrateMetadata {
     }
 
     pub fn get_item_attrs(&self, node_id: DefIndex, sess: &Session) -> Lrc<[ast::Attribute]> {
-        let (node_as, node_index) =
-            (node_id.address_space().index(), node_id.as_array_index());
         if self.is_proc_macro(node_id) {
             return Lrc::new([]);
         }
 
-        if let Some(&Some(ref val)) =
-            self.attribute_cache.borrow()[node_as].get(node_index) {
-            return val.clone();
-        }
-
         // The attributes for a tuple struct are attached to the definition, not the ctor;
         // we assume that someone passing in a tuple struct ctor is actually wanting to
         // look at the definition
-        let mut item = self.entry(node_id);
         let def_key = self.def_key(node_id);
-        if def_key.disambiguated_data.data == DefPathData::StructCtor {
-            item = self.entry(def_key.parent.unwrap());
-        }
-        let result: Lrc<[ast::Attribute]> = Lrc::from(self.get_attributes(&item, sess));
-        let vec_ = &mut self.attribute_cache.borrow_mut()[node_as];
-        if vec_.len() < node_index + 1 {
-            vec_.resize(node_index + 1, None);
-        }
-        // This can overwrite the result produced by another thread, but the value
-        // written should be the same
-        vec_[node_index] = Some(result.clone());
-        result
+        let item_id = if def_key.disambiguated_data.data == DefPathData::StructCtor {
+            def_key.parent.unwrap()
+        } else {
+            node_id
+        };
+
+        let item = self.entry(item_id);
+        Lrc::from(self.get_attributes(&item, sess))
     }
 
     pub fn get_struct_field_names(&self, id: DefIndex) -> Vec<ast::Name> {
diff --git a/src/librustc_metadata/encoder.rs b/src/librustc_metadata/encoder.rs
index a8836694933..ab30ff7f072 100644
--- a/src/librustc_metadata/encoder.rs
+++ b/src/librustc_metadata/encoder.rs
@@ -483,10 +483,10 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
         let index = items.write_index(&mut self.opaque.cursor);
         let index_bytes = self.position() - i;
 
+        let attrs = tcx.hir.krate_attrs();
         let link_meta = self.link_meta;
         let is_proc_macro = tcx.sess.crate_types.borrow().contains(&CrateTypeProcMacro);
-        let has_default_lib_allocator =
-            attr::contains_name(tcx.hir.krate_attrs(), "default_lib_allocator");
+        let has_default_lib_allocator = attr::contains_name(&attrs, "default_lib_allocator");
         let has_global_allocator = *tcx.sess.has_global_allocator.get();
 
         let root = self.lazy(&CrateRoot {
@@ -510,6 +510,14 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
                 None
             },
 
+            compiler_builtins: attr::contains_name(&attrs, "compiler_builtins"),
+            needs_allocator: attr::contains_name(&attrs, "needs_allocator"),
+            needs_panic_runtime: attr::contains_name(&attrs, "needs_panic_runtime"),
+            no_builtins: attr::contains_name(&attrs, "no_builtins"),
+            panic_runtime: attr::contains_name(&attrs, "panic_runtime"),
+            profiler_runtime: attr::contains_name(&attrs, "profiler_runtime"),
+            sanitizer_runtime: attr::contains_name(&attrs, "sanitizer_runtime"),
+
             crate_deps,
             dylib_dependency_formats,
             lang_items,
diff --git a/src/librustc_metadata/schema.rs b/src/librustc_metadata/schema.rs
index 8e17b7f8d69..2e89ea6d2c1 100644
--- a/src/librustc_metadata/schema.rs
+++ b/src/librustc_metadata/schema.rs
@@ -210,6 +210,14 @@ pub struct CrateRoot {
     pub interpret_alloc_index: LazySeq<u32>,
 
     pub index: LazySeq<index::Index>,
+
+    pub compiler_builtins: bool,
+    pub needs_allocator: bool,
+    pub needs_panic_runtime: bool,
+    pub no_builtins: bool,
+    pub panic_runtime: bool,
+    pub profiler_runtime: bool,
+    pub sanitizer_runtime: bool,
 }
 
 #[derive(RustcEncodable, RustcDecodable)]