Skip to content

Commit

Permalink
Auto merge of #57392 - Xanewok:always-calc-glob-map, r=petrochenkov
Browse files Browse the repository at this point in the history
Always calculate glob map but only for glob uses

Previously calculating glob map was *opt-in*, however it did record node id -> ident use for every use directive. This aims to see if we can unconditionally calculate the glob map and not regress performance.

Main motivation is to get rid of some of the moving pieces and simplify the compilation interface - this would allow us to entirely remove `CrateAnalysis`. Later, we could easily expose a relevant query, similar to the likes of `maybe_unused_trait_import` (so using precomputed data from the resolver, but which could be rewritten to be on-demand).

r? @nikomatsakis

Local perf run showed mostly noise (except `ctfe-stress-*`) but I'd appreciate if we could do a perf run run here and double-check that this won't regress performance.
  • Loading branch information
bors committed Jan 16, 2019
2 parents ceb2512 + b1b64bd commit 59c1cac
Show file tree
Hide file tree
Showing 11 changed files with 14 additions and 44 deletions.
2 changes: 1 addition & 1 deletion src/librustc/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ mod sty;
/// *on-demand* infrastructure.
#[derive(Clone)]
pub struct CrateAnalysis {
pub glob_map: Option<hir::GlobMap>,
pub glob_map: hir::GlobMap,
}

#[derive(Clone)]
Expand Down
15 changes: 2 additions & 13 deletions src/librustc_driver/driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use rustc_passes::{self, ast_validation, hir_stats, loops, rvalue_promotion};
use rustc_plugin as plugin;
use rustc_plugin::registry::Registry;
use rustc_privacy;
use rustc_resolve::{MakeGlobMap, Resolver, ResolverArenas};
use rustc_resolve::{Resolver, ResolverArenas};
use rustc_traits;
use rustc_typeck as typeck;
use syntax::{self, ast, attr, diagnostics, visit};
Expand Down Expand Up @@ -179,7 +179,6 @@ pub fn compile_input(
registry,
&crate_name,
addl_plugins,
control.make_glob_map,
|expanded_crate| {
let mut state = CompileState::state_after_expand(
input,
Expand Down Expand Up @@ -394,7 +393,6 @@ pub struct CompileController<'a> {

// FIXME we probably want to group the below options together and offer a
// better API, rather than this ad-hoc approach.
pub make_glob_map: MakeGlobMap,
// Whether the compiler should keep the ast beyond parsing.
pub keep_ast: bool,
// -Zcontinue-parse-after-error
Expand All @@ -417,7 +415,6 @@ impl<'a> CompileController<'a> {
after_hir_lowering: PhaseController::basic(),
after_analysis: PhaseController::basic(),
compilation_done: PhaseController::basic(),
make_glob_map: MakeGlobMap::No,
keep_ast: false,
continue_parse_after_error: false,
provide: box |_| {},
Expand Down Expand Up @@ -739,7 +736,6 @@ pub fn phase_2_configure_and_expand<F>(
registry: Option<Registry>,
crate_name: &str,
addl_plugins: Option<Vec<String>>,
make_glob_map: MakeGlobMap,
after_expand: F,
) -> Result<ExpansionResult, CompileIncomplete>
where
Expand All @@ -759,7 +755,6 @@ where
registry,
crate_name,
addl_plugins,
make_glob_map,
&resolver_arenas,
&mut crate_loader,
after_expand,
Expand All @@ -785,11 +780,7 @@ where
},

analysis: ty::CrateAnalysis {
glob_map: if resolver.make_glob_map {
Some(resolver.glob_map)
} else {
None
},
glob_map: resolver.glob_map
},
}),
Err(x) => Err(x),
Expand All @@ -805,7 +796,6 @@ pub fn phase_2_configure_and_expand_inner<'a, F>(
registry: Option<Registry>,
crate_name: &str,
addl_plugins: Option<Vec<String>>,
make_glob_map: MakeGlobMap,
resolver_arenas: &'a ResolverArenas<'a>,
crate_loader: &'a mut CrateLoader<'a>,
after_expand: F,
Expand Down Expand Up @@ -937,7 +927,6 @@ where
cstore,
&krate,
crate_name,
make_glob_map,
crate_loader,
&resolver_arenas,
);
Expand Down
2 changes: 0 additions & 2 deletions src/librustc_driver/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ extern crate syntax_pos;
use driver::CompileController;
use pretty::{PpMode, UserIdentifiedItem};

use rustc_resolve as resolve;
use rustc_save_analysis as save;
use rustc_save_analysis::DumpHandler;
use rustc_data_structures::sync::{self, Lrc, Ordering::SeqCst};
Expand Down Expand Up @@ -950,7 +949,6 @@ pub fn enable_save_analysis(control: &mut CompileController) {
});
};
control.after_analysis.run_callback_on_error = true;
control.make_glob_map = resolve::MakeGlobMap::Yes;
}

impl RustcDefaultCalls {
Expand Down
2 changes: 0 additions & 2 deletions src/librustc_driver/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ use rustc::ty::{self, Ty, TyCtxt, TypeFoldable};
use rustc_data_structures::sync::{self, Lrc};
use rustc_lint;
use rustc_metadata::cstore::CStore;
use rustc_resolve::MakeGlobMap;
use rustc_target::spec::abi::Abi;
use syntax;
use syntax::ast;
Expand Down Expand Up @@ -134,7 +133,6 @@ fn test_env_with_pool<F>(
None,
"test",
None,
MakeGlobMap::No,
|_| Ok(()),
).expect("phase 2 aborted")
};
Expand Down
23 changes: 7 additions & 16 deletions src/librustc_resolve/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1546,9 +1546,7 @@ pub struct Resolver<'a> {
extern_module_map: FxHashMap<(DefId, bool /* MacrosOnly? */), Module<'a>>,
binding_parent_modules: FxHashMap<PtrKey<'a, NameBinding<'a>>, Module<'a>>,

pub make_glob_map: bool,
/// Maps imports to the names of items actually imported (this actually maps
/// all imports, but only glob imports are actually interesting).
/// Maps glob imports to the names of items actually imported.
pub glob_map: GlobMap,

used_imports: FxHashSet<(NodeId, Namespace)>,
Expand Down Expand Up @@ -1795,7 +1793,6 @@ impl<'a> Resolver<'a> {
cstore: &'a CStore,
krate: &Crate,
crate_name: &str,
make_glob_map: MakeGlobMap,
crate_loader: &'a mut CrateLoader<'a>,
arenas: &'a ResolverArenas<'a>)
-> Resolver<'a> {
Expand Down Expand Up @@ -1879,7 +1876,6 @@ impl<'a> Resolver<'a> {
extern_module_map: FxHashMap::default(),
binding_parent_modules: FxHashMap::default(),

make_glob_map: make_glob_map == MakeGlobMap::Yes,
glob_map: Default::default(),

used_imports: FxHashSet::default(),
Expand Down Expand Up @@ -1989,14 +1985,15 @@ impl<'a> Resolver<'a> {
used.set(true);
directive.used.set(true);
self.used_imports.insert((directive.id, ns));
self.add_to_glob_map(directive.id, ident);
self.add_to_glob_map(&directive, ident);
self.record_use(ident, ns, binding, false);
}
}

fn add_to_glob_map(&mut self, id: NodeId, ident: Ident) {
if self.make_glob_map {
self.glob_map.entry(id).or_default().insert(ident.name);
#[inline]
fn add_to_glob_map(&mut self, directive: &ImportDirective<'_>, ident: Ident) {
if directive.is_glob() {
self.glob_map.entry(directive.id).or_default().insert(ident.name);
}
}

Expand Down Expand Up @@ -4598,7 +4595,7 @@ impl<'a> Resolver<'a> {
let import_id = match binding.kind {
NameBindingKind::Import { directive, .. } => {
self.maybe_unused_trait_imports.insert(directive.id);
self.add_to_glob_map(directive.id, trait_name);
self.add_to_glob_map(&directive, trait_name);
Some(directive.id)
}
_ => None,
Expand Down Expand Up @@ -5305,12 +5302,6 @@ fn err_path_resolution() -> PathResolution {
PathResolution::new(Def::Err)
}

#[derive(PartialEq,Copy, Clone)]
pub enum MakeGlobMap {
Yes,
No,
}

#[derive(Copy, Clone, Debug)]
enum CrateLint {
/// Do not issue the lint
Expand Down
1 change: 0 additions & 1 deletion src/librustc_save_analysis/dump_visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1239,7 +1239,6 @@ impl<'l, 'tcx: 'l, 'll, O: DumpOutput + 'll> DumpVisitor<'l, 'tcx, 'll, O> {

// Make a comma-separated list of names of imported modules.
let glob_map = &self.save_ctxt.analysis.glob_map;
let glob_map = glob_map.as_ref().unwrap();
let names = if glob_map.contains_key(&id) {
glob_map.get(&id).unwrap().iter().map(|n| n.to_string()).collect()
} else {
Expand Down
2 changes: 0 additions & 2 deletions src/librustc_save_analysis/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1127,8 +1127,6 @@ pub fn process_crate<'l, 'tcx, H: SaveHandler>(
mut handler: H,
) {
tcx.dep_graph.with_ignore(|| {
assert!(analysis.glob_map.is_some());

info!("Dumping crate {}", cratename);

// Privacy checking requires and is done after type checking; use a
Expand Down
3 changes: 1 addition & 2 deletions src/librustdoc/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -451,7 +451,6 @@ pub fn run_core(options: RustdocOptions) -> (clean::Crate, RenderInfo, RenderOpt
None,
&name,
None,
resolve::MakeGlobMap::No,
&resolver_arenas,
&mut crate_loader,
|_| Ok(()));
Expand All @@ -476,7 +475,7 @@ pub fn run_core(options: RustdocOptions) -> (clean::Crate, RenderInfo, RenderOpt
}).collect(),
};
let analysis = ty::CrateAnalysis {
glob_map: if resolver.make_glob_map { Some(resolver.glob_map.clone()) } else { None },
glob_map: resolver.glob_map.clone(),
};

let mut arenas = AllArenas::new();
Expand Down
2 changes: 0 additions & 2 deletions src/librustdoc/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ use rustc_driver::{self, driver, target_features, Compilation};
use rustc_driver::driver::phase_2_configure_and_expand;
use rustc_metadata::cstore::CStore;
use rustc_metadata::dynamic_lib::DynamicLibrary;
use rustc_resolve::MakeGlobMap;
use rustc::hir;
use rustc::hir::intravisit;
use rustc::session::{self, CompileIncomplete, config};
Expand Down Expand Up @@ -100,7 +99,6 @@ pub fn run(mut options: Options) -> isize {
None,
"rustdoc-test",
None,
MakeGlobMap::No,
|_| Ok(()),
).expect("phase_2_configure_and_expand aborted in rustdoc!")
};
Expand Down
4 changes: 2 additions & 2 deletions src/test/rustdoc-ui/failed-doctest-output.stdout
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ error[E0425]: cannot find value `no` in this scope
3 | no
| ^^ not found in this scope

thread '$DIR/failed-doctest-output.rs - OtherStruct (line 17)' panicked at 'couldn't compile the test', src/librustdoc/test.rs:321:13
thread '$DIR/failed-doctest-output.rs - OtherStruct (line 17)' panicked at 'couldn't compile the test', src/librustdoc/test.rs:319:13
note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

---- $DIR/failed-doctest-output.rs - SomeStruct (line 11) stdout ----
Expand All @@ -21,7 +21,7 @@ thread '$DIR/failed-doctest-output.rs - SomeStruct (line 11)' panicked at 'test
thread 'main' panicked at 'oh no', $DIR/failed-doctest-output.rs:3:1
note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

', src/librustdoc/test.rs:356:17
', src/librustdoc/test.rs:354:17


failures:
Expand Down
2 changes: 1 addition & 1 deletion src/tools/rls
Submodule rls updated from 1a6361 to ae0d89

0 comments on commit 59c1cac

Please sign in to comment.