From 3d24fadae43f628c506448a8c9fa2aa11a13e11b Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 7 Apr 2014 12:14:33 -0700 Subject: [PATCH 1/3] rustc: Use CStore, not a separate crate cache This separate crate cache is one factor which is causing libstd to be loaded twice during normal compilation. The crates loaded for syntax extensions have a separate cache than the crates loaded for linking, so all crates are loaded once per #[phase] they're tagged with. This removes the cache and instead uses the CStore structure itself as the cache for loaded crates. This should allow crates loaded during the syntax phase to be shared with the crates loaded during the link phase. --- src/librustc/metadata/creader.rs | 119 +++++++++++++------------------ src/librustc/metadata/cstore.rs | 12 ++++ 2 files changed, 61 insertions(+), 70 deletions(-) diff --git a/src/librustc/metadata/creader.rs b/src/librustc/metadata/creader.rs index 8ce4a2f8eed86..9c0c73288eb0d 100644 --- a/src/librustc/metadata/creader.rs +++ b/src/librustc/metadata/creader.rs @@ -18,6 +18,7 @@ use driver::{driver, session}; use driver::session::Session; use metadata::csearch; use metadata::cstore; +use metadata::cstore::CStore; use metadata::decoder; use metadata::loader; use metadata::loader::Os; @@ -38,6 +39,13 @@ use syntax::parse::token; use syntax::crateid::CrateId; use syntax::visit; +struct Env<'a> { + sess: &'a Session, + os: loader::Os, + next_crate_num: ast::CrateNum, + intr: Rc +} + // Traverses an AST, reading all the information about use'd crates and extern // libraries necessary for later resolving, typechecking, linking, etc. pub fn read_crates(sess: &Session, @@ -47,16 +55,13 @@ pub fn read_crates(sess: &Session, let mut e = Env { sess: sess, os: os, - crate_cache: @RefCell::new(Vec::new()), - next_crate_num: 1, + next_crate_num: sess.cstore.next_crate_num(), intr: intr }; visit_crate(&e, krate); visit::walk_crate(&mut e, krate, ()); - dump_crates(e.crate_cache.borrow().as_slice()); - warn_if_multiple_versions(&mut e, - sess.diagnostic(), - e.crate_cache.borrow().as_slice()); + dump_crates(&sess.cstore); + warn_if_multiple_versions(sess.diagnostic(), &sess.cstore) } impl<'a> visit::Visitor<()> for Env<'a> { @@ -70,55 +75,36 @@ impl<'a> visit::Visitor<()> for Env<'a> { } } -#[deriving(Clone)] -struct cache_entry { - cnum: ast::CrateNum, - span: Span, - hash: Svh, - crate_id: CrateId, -} - -fn dump_crates(crate_cache: &[cache_entry]) { +fn dump_crates(cstore: &CStore) { debug!("resolved crates:"); - for entry in crate_cache.iter() { - debug!("cnum: {:?}", entry.cnum); - debug!("span: {:?}", entry.span); - debug!("hash: {:?}", entry.hash); - } + cstore.iter_crate_data(|_, data| { + debug!("crate_id: {}", data.crate_id()); + debug!(" cnum: {}", data.cnum); + debug!(" hash: {}", data.hash()); + }) } -fn warn_if_multiple_versions(e: &mut Env, - diag: &SpanHandler, - crate_cache: &[cache_entry]) { - if crate_cache.len() != 0u { - let name = crate_cache[crate_cache.len() - 1].crate_id.name.clone(); - - let (matches, non_matches) = crate_cache.partitioned(|entry| - name == entry.crate_id.name); +fn warn_if_multiple_versions(diag: &SpanHandler, cstore: &CStore) { + let mut map = HashMap::new(); - assert!(!matches.is_empty()); + cstore.iter_crate_data(|cnum, data| { + let crateid = data.crate_id(); + let key = (crateid.name.clone(), crateid.path.clone()); + map.find_or_insert_with(key, |_| Vec::new()).push(cnum); + }); - if matches.len() != 1u { - diag.handler().warn( - format!("using multiple versions of crate `{}`", name)); - for match_ in matches.iter() { - diag.span_note(match_.span, "used here"); - loader::note_crateid_attr(diag, &match_.crate_id); - } + for ((name, _), dupes) in map.move_iter() { + if dupes.len() == 1 { continue } + diag.handler().warn( + format!("using multiple versions of crate `{}`", name)); + for dupe in dupes.move_iter() { + let data = cstore.get_crate_data(dupe); + diag.span_note(data.span, "used here"); + loader::note_crateid_attr(diag, &data.crate_id()); } - - warn_if_multiple_versions(e, diag, non_matches); } } -struct Env<'a> { - sess: &'a Session, - os: loader::Os, - crate_cache: @RefCell>, - next_crate_num: ast::CrateNum, - intr: Rc -} - fn visit_crate(e: &Env, c: &ast::Crate) { for a in c.attrs.iter().filter(|m| m.name().equiv(&("link_args"))) { match a.value_str() { @@ -269,14 +255,18 @@ fn visit_item(e: &Env, i: &ast::Item) { fn existing_match(e: &Env, crate_id: &CrateId, hash: Option<&Svh>) -> Option { - for c in e.crate_cache.borrow().iter() { - if !crate_id.matches(&c.crate_id) { continue } - match hash { - Some(hash) if *hash != c.hash => {} - Some(..) | None => return Some(c.cnum) + let mut ret = None; + e.sess.cstore.iter_crate_data(|cnum, data| { + let other_id = data.crate_id(); + if crate_id.matches(&other_id) { + let other_hash = data.hash(); + match hash { + Some(hash) if *hash != other_hash => {} + Some(..) | None => { ret = Some(cnum); } + } } - } - None + }); + return ret; } fn resolve_crate<'a>(e: &mut Env, @@ -304,17 +294,8 @@ fn resolve_crate<'a>(e: &mut Env, dylib, rlib, metadata } = load_ctxt.load_library_crate(root); - let crate_id = decoder::get_crate_id(metadata.as_slice()); - let hash = decoder::get_crate_hash(metadata.as_slice()); - // Claim this crate number and cache it let cnum = e.next_crate_num; - e.crate_cache.borrow_mut().push(cache_entry { - cnum: cnum, - span: span, - hash: hash, - crate_id: crate_id, - }); e.next_crate_num += 1; // Stash paths for top-most crate locally if necessary. @@ -331,16 +312,15 @@ fn resolve_crate<'a>(e: &mut Env, let root = if root.is_some() { root } else { &crate_paths }; // Now resolve the crates referenced by this crate - let cnum_map = resolve_crate_deps(e, - root, - metadata.as_slice(), - span); + let cnum_map = resolve_crate_deps(e, root, metadata.as_slice(), + span); let cmeta = @cstore::crate_metadata { name: load_ctxt.crate_id.name.to_owned(), data: metadata, cnum_map: cnum_map, - cnum: cnum + cnum: cnum, + span: span, }; e.sess.cstore.set_crate_data(cnum, cmeta); @@ -390,8 +370,7 @@ impl<'a> Loader<'a> { env: Env { sess: sess, os: os, - crate_cache: @RefCell::new(Vec::new()), - next_crate_num: 1, + next_crate_num: sess.cstore.next_crate_num(), intr: token::get_ident_interner(), } } @@ -406,7 +385,7 @@ impl<'a> CrateLoader for Loader<'a> { let library = self.env.sess.cstore.get_used_crate_source(cnum).unwrap(); MacroCrate { lib: library.dylib, - cnum: cnum + cnum: cnum, } } diff --git a/src/librustc/metadata/cstore.rs b/src/librustc/metadata/cstore.rs index e3a3239bdb5b8..2d46f92e88fa4 100644 --- a/src/librustc/metadata/cstore.rs +++ b/src/librustc/metadata/cstore.rs @@ -22,6 +22,8 @@ use std::c_vec::CVec; use std::rc::Rc; use collections::HashMap; use syntax::ast; +use syntax::crateid::CrateId; +use syntax::codemap::Span; use syntax::parse::token::IdentInterner; // A map from external crate numbers (as decoded from some crate file) to @@ -40,6 +42,7 @@ pub struct crate_metadata { pub data: MetadataBlob, pub cnum_map: cnum_map, pub cnum: ast::CrateNum, + pub span: Span, } #[deriving(Eq)] @@ -88,6 +91,10 @@ impl CStore { } } + pub fn next_crate_num(&self) -> ast::CrateNum { + self.metas.borrow().len() as ast::CrateNum + 1 + } + pub fn get_crate_data(&self, cnum: ast::CrateNum) -> @crate_metadata { *self.metas.borrow().get(&cnum) } @@ -121,6 +128,9 @@ impl CStore { .map(|source| source.clone()) } + pub fn dump_phase_syntax_crates(&self) { + } + pub fn reset(&self) { self.metas.borrow_mut().clear(); self.extern_mod_crate_map.borrow_mut().clear(); @@ -202,6 +212,8 @@ impl CStore { impl crate_metadata { pub fn data<'a>(&'a self) -> &'a [u8] { self.data.as_slice() } + pub fn crate_id(&self) -> CrateId { decoder::get_crate_id(self.data()) } + pub fn hash(&self) -> Svh { decoder::get_crate_hash(self.data()) } } impl MetadataBlob { From e2d34d7c5b30bf656a8e8399c96e16690e638e92 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 7 Apr 2014 12:16:43 -0700 Subject: [PATCH 2/3] rustc: Never register syntax crates in CStore When linking, all crates in the local CStore are used to link the final product. With #[phase(syntax)], crates want to be omitted from this linkage phase, and this was achieved by dumping the entire CStore after loading crates. This causes crates like the standard library to get loaded twice. This loading process is a fairly expensive operation when dealing with decompressing metadata. This commit alters the loading process to never register syntax crates in CStore. Instead, only phase(link) crates ever make their way into the map of crates. The CrateLoader trait was altered to return everything in one method instead of having separate methods for finding information. --- src/librustc/driver/driver.rs | 2 - src/librustc/metadata/creader.rs | 79 ++++++++++++++++++-------------- src/libsyntax/ext/base.rs | 5 +- src/libsyntax/ext/expand.rs | 8 ++-- 4 files changed, 51 insertions(+), 43 deletions(-) diff --git a/src/librustc/driver/driver.rs b/src/librustc/driver/driver.rs index 0668abea2b12f..8a593d5f92a2a 100644 --- a/src/librustc/driver/driver.rs +++ b/src/librustc/driver/driver.rs @@ -241,8 +241,6 @@ pub fn phase_2_configure_and_expand(sess: &Session, cfg, krate) }); - // dump the syntax-time crates - sess.cstore.reset(); // strip again, in case expansion added anything with a #[cfg]. krate = time(time_passes, "configuration 2", krate, |krate| diff --git a/src/librustc/metadata/creader.rs b/src/librustc/metadata/creader.rs index 9c0c73288eb0d..50c22f6bf1bba 100644 --- a/src/librustc/metadata/creader.rs +++ b/src/librustc/metadata/creader.rs @@ -114,22 +114,25 @@ fn visit_crate(e: &Env, c: &ast::Crate) { } } -fn visit_view_item(e: &mut Env, i: &ast::ViewItem) { - let should_load = i.attrs.iter().all(|attr| { +fn should_link(i: &ast::ViewItem) -> bool { + i.attrs.iter().all(|attr| { attr.name().get() != "phase" || attr.meta_item_list().map_or(false, |phases| { attr::contains_name(phases.as_slice(), "link") }) - }); + }) +} - if !should_load { +fn visit_view_item(e: &mut Env, i: &ast::ViewItem) { + if !should_link(i) { return; } match extract_crate_info(e, i) { Some(info) => { - let cnum = resolve_crate(e, &None, info.ident, &info.crate_id, None, - i.span); + let (cnum, _, _) = resolve_crate(e, &None, info.ident, + &info.crate_id, None, true, + i.span); e.sess.cstore.add_extern_mod_stmt_cnum(info.id, cnum); } None => () @@ -140,6 +143,7 @@ struct CrateInfo { ident: ~str, crate_id: CrateId, id: ast::NodeId, + should_link: bool, } fn extract_crate_info(e: &Env, i: &ast::ViewItem) -> Option { @@ -165,6 +169,7 @@ fn extract_crate_info(e: &Env, i: &ast::ViewItem) -> Option { ident: ident.get().to_str(), crate_id: crate_id, id: id, + should_link: should_link(i), }) } _ => None @@ -274,8 +279,10 @@ fn resolve_crate<'a>(e: &mut Env, ident: &str, crate_id: &CrateId, hash: Option<&Svh>, + should_link: bool, span: Span) - -> ast::CrateNum { + -> (ast::CrateNum, @cstore::crate_metadata, + cstore::CrateSource) { match existing_match(e, crate_id, hash) { None => { let id_hash = link::crate_id_hash(crate_id); @@ -312,8 +319,11 @@ fn resolve_crate<'a>(e: &mut Env, let root = if root.is_some() { root } else { &crate_paths }; // Now resolve the crates referenced by this crate - let cnum_map = resolve_crate_deps(e, root, metadata.as_slice(), - span); + let cnum_map = if should_link { + resolve_crate_deps(e, root, metadata.as_slice(), span) + } else { + @RefCell::new(HashMap::new()) + }; let cmeta = @cstore::crate_metadata { name: load_ctxt.crate_id.name.to_owned(), @@ -323,15 +333,21 @@ fn resolve_crate<'a>(e: &mut Env, span: span, }; - e.sess.cstore.set_crate_data(cnum, cmeta); - e.sess.cstore.add_used_crate_source(cstore::CrateSource { + let source = cstore::CrateSource { dylib: dylib, rlib: rlib, cnum: cnum, - }); - cnum + }; + + if should_link { + e.sess.cstore.set_crate_data(cnum, cmeta); + e.sess.cstore.add_used_crate_source(source.clone()); + } + (cnum, cmeta, source) } - Some(cnum) => cnum + Some(cnum) => (cnum, + e.sess.cstore.get_crate_data(cnum), + e.sess.cstore.get_used_crate_source(cnum).unwrap()) } } @@ -348,11 +364,12 @@ fn resolve_crate_deps(e: &mut Env, for dep in r.iter() { let extrn_cnum = dep.cnum; debug!("resolving dep crate {} hash: `{}`", dep.crate_id, dep.hash); - let local_cnum = resolve_crate(e, root, - dep.crate_id.name.as_slice(), - &dep.crate_id, - Some(&dep.hash), - span); + let (local_cnum, _, _) = resolve_crate(e, root, + dep.crate_id.name.as_slice(), + &dep.crate_id, + Some(&dep.hash), + true, + span); cnum_map.insert(extrn_cnum, local_cnum); } return @RefCell::new(cnum_map); @@ -380,23 +397,17 @@ impl<'a> Loader<'a> { impl<'a> CrateLoader for Loader<'a> { fn load_crate(&mut self, krate: &ast::ViewItem) -> MacroCrate { let info = extract_crate_info(&self.env, krate).unwrap(); - let cnum = resolve_crate(&mut self.env, &None, info.ident, - &info.crate_id, None, krate.span); - let library = self.env.sess.cstore.get_used_crate_source(cnum).unwrap(); + let (cnum, data, library) = resolve_crate(&mut self.env, &None, + info.ident, &info.crate_id, + None, true, krate.span); + let macros = decoder::get_exported_macros(data); + let cstore = &self.env.sess.cstore; + let registrar = csearch::get_macro_registrar_fn(cstore, cnum) + .map(|did| csearch::get_symbol(cstore, did)); MacroCrate { lib: library.dylib, - cnum: cnum, + macros: macros.move_iter().collect(), + registrar_symbol: registrar, } } - - fn get_exported_macros(&mut self, cnum: ast::CrateNum) -> Vec<~str> { - csearch::get_exported_macros(&self.env.sess.cstore, cnum).move_iter() - .collect() - } - - fn get_registrar_symbol(&mut self, cnum: ast::CrateNum) -> Option<~str> { - let cstore = &self.env.sess.cstore; - csearch::get_macro_registrar_fn(cstore, cnum) - .map(|did| csearch::get_symbol(cstore, did)) - } } diff --git a/src/libsyntax/ext/base.rs b/src/libsyntax/ext/base.rs index 7ff7792313251..3bf1ed95f380e 100644 --- a/src/libsyntax/ext/base.rs +++ b/src/libsyntax/ext/base.rs @@ -293,13 +293,12 @@ pub fn syntax_expander_table() -> SyntaxEnv { pub struct MacroCrate { pub lib: Option, - pub cnum: ast::CrateNum, + pub macros: Vec<~str>, + pub registrar_symbol: Option<~str>, } pub trait CrateLoader { fn load_crate(&mut self, krate: &ast::ViewItem) -> MacroCrate; - fn get_exported_macros(&mut self, crate_num: ast::CrateNum) -> Vec<~str> ; - fn get_registrar_symbol(&mut self, crate_num: ast::CrateNum) -> Option<~str>; } // One of these is made during expansion and incrementally updated as we go; diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index 747ab583e792a..5c3c7d995d67a 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -487,7 +487,8 @@ pub fn expand_view_item(vi: &ast::ViewItem, } fn load_extern_macros(krate: &ast::ViewItem, fld: &mut MacroExpander) { - let MacroCrate { lib, cnum } = fld.cx.ecfg.loader.load_crate(krate); + let MacroCrate { lib, macros, registrar_symbol } = + fld.cx.ecfg.loader.load_crate(krate); let crate_name = match krate.node { ast::ViewItemExternCrate(name, _, _) => name, @@ -495,8 +496,7 @@ fn load_extern_macros(krate: &ast::ViewItem, fld: &mut MacroExpander) { }; let name = format!("<{} macros>", token::get_ident(crate_name)); - let exported_macros = fld.cx.ecfg.loader.get_exported_macros(cnum); - for source in exported_macros.iter() { + for source in macros.iter() { let item = parse::parse_item_from_source_str(name.clone(), (*source).clone(), fld.cx.cfg(), @@ -512,7 +512,7 @@ fn load_extern_macros(krate: &ast::ViewItem, fld: &mut MacroExpander) { // Make sure the path contains a / or the linker will search for it. let path = os::make_absolute(&path); - let registrar = match fld.cx.ecfg.loader.get_registrar_symbol(cnum) { + let registrar = match registrar_symbol { Some(registrar) => registrar, None => return }; From fe78af4e754678336b6dc55586da4224e2c3a2fe Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 7 Apr 2014 13:13:21 -0700 Subject: [PATCH 3/3] rustc: Don't read both rlib and dylib metadata This is an optimization which is quite impactful for compiling small crates. Reading libstd's metadata takes about 50ms, and a hello world before this change took about 100ms (this change halves that time). Recent changes made it such that this optimization wasn't performed, but I think it's a better idea do to this for now. See #10786 for tracking this issue. --- src/librustc/metadata/loader.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/librustc/metadata/loader.rs b/src/librustc/metadata/loader.rs index 4f4ef31e15b37..00820a5d34cdc 100644 --- a/src/librustc/metadata/loader.rs +++ b/src/librustc/metadata/loader.rs @@ -317,15 +317,23 @@ impl<'a> Context<'a> { // read the metadata from it if `*slot` is `None`. If the metadata couldn't // be read, it is assumed that the file isn't a valid rust library (no // errors are emitted). - // - // FIXME(#10786): for an optimization, we only read one of the library's - // metadata sections. In theory we should read both, but - // reading dylib metadata is quite slow. fn extract_one(&mut self, m: HashSet, flavor: &str, slot: &mut Option) -> Option { let mut ret = None::; let mut error = 0; + if slot.is_some() { + // FIXME(#10786): for an optimization, we only read one of the + // library's metadata sections. In theory we should + // read both, but reading dylib metadata is quite + // slow. + if m.len() == 0 { + return None + } else if m.len() == 1 { + return Some(m.move_iter().next().unwrap()) + } + } + for lib in m.move_iter() { info!("{} reading metadata from: {}", flavor, lib.display()); let metadata = match get_metadata_section(self.os, &lib) {