Skip to content

Commit

Permalink
cache symbol names in ty::maps
Browse files Browse the repository at this point in the history
this fixes a performance regression introduced in commit
39a58c3.
  • Loading branch information
arielb1 committed Apr 26, 2017
1 parent b0a4074 commit a517343
Show file tree
Hide file tree
Showing 16 changed files with 148 additions and 176 deletions.
2 changes: 2 additions & 0 deletions src/librustc/dep_graph/dep_node.rs
Expand Up @@ -99,6 +99,7 @@ pub enum DepNode<D: Clone + Debug> {
TypeckTables(D),
UsedTraitImports(D),
ConstEval(D),
SymbolName(D),

// The set of impls for a given trait. Ultimately, it would be
// nice to get more fine-grained here (e.g., to include a
Expand Down Expand Up @@ -236,6 +237,7 @@ impl<D: Clone + Debug> DepNode<D> {
TypeckTables(ref d) => op(d).map(TypeckTables),
UsedTraitImports(ref d) => op(d).map(UsedTraitImports),
ConstEval(ref d) => op(d).map(ConstEval),
SymbolName(ref d) => op(d).map(SymbolName),
TraitImpls(ref d) => op(d).map(TraitImpls),
TraitItems(ref d) => op(d).map(TraitItems),
ReprHints(ref d) => op(d).map(ReprHints),
Expand Down
35 changes: 33 additions & 2 deletions src/librustc/ty/maps.rs
Expand Up @@ -24,6 +24,7 @@ use std::cell::{RefCell, RefMut};
use std::ops::Deref;
use std::rc::Rc;
use syntax_pos::{Span, DUMMY_SP};
use syntax::symbol::Symbol;

trait Key {
fn map_crate(&self) -> CrateNum;
Expand All @@ -40,6 +41,16 @@ impl<'tcx> Key for ty::InstanceDef<'tcx> {
}
}

impl<'tcx> Key for ty::Instance<'tcx> {
fn map_crate(&self) -> CrateNum {
LOCAL_CRATE
}

fn default_span(&self, tcx: TyCtxt) -> Span {
tcx.def_span(self.def_id())
}
}

impl Key for CrateNum {
fn map_crate(&self) -> CrateNum {
*self
Expand Down Expand Up @@ -108,13 +119,18 @@ impl<'tcx> Value<'tcx> for Ty<'tcx> {
}
}


impl<'tcx> Value<'tcx> for ty::DtorckConstraint<'tcx> {
fn from_cycle_error<'a>(_: TyCtxt<'a, 'tcx, 'tcx>) -> Self {
Self::empty()
}
}

impl<'tcx> Value<'tcx> for ty::SymbolName {
fn from_cycle_error<'a>(_: TyCtxt<'a, 'tcx, 'tcx>) -> Self {
ty::SymbolName { name: Symbol::intern("<error>").as_str() }
}
}

pub struct CycleError<'a, 'tcx: 'a> {
span: Span,
cycle: RefMut<'a, [(Span, Query<'tcx>)]>,
Expand Down Expand Up @@ -242,6 +258,12 @@ impl<'tcx> QueryDescription for queries::const_eval<'tcx> {
}
}

impl<'tcx> QueryDescription for queries::symbol_name<'tcx> {
fn describe(_tcx: TyCtxt, instance: ty::Instance<'tcx>) -> String {
format!("computing the symbol for `{}`", instance)
}
}

macro_rules! define_maps {
(<$tcx:tt>
$($(#[$attr:meta])*
Expand Down Expand Up @@ -513,7 +535,10 @@ define_maps! { <'tcx>

pub reachable_set: reachability_dep_node(CrateNum) -> Rc<NodeSet>,

pub mir_shims: mir_shim_dep_node(ty::InstanceDef<'tcx>) -> &'tcx RefCell<mir::Mir<'tcx>>
pub mir_shims: mir_shim_dep_node(ty::InstanceDef<'tcx>) -> &'tcx RefCell<mir::Mir<'tcx>>,

pub def_symbol_name: SymbolName(DefId) -> ty::SymbolName,
pub symbol_name: symbol_name_dep_node(ty::Instance<'tcx>) -> ty::SymbolName
}

fn coherent_trait_dep_node((_, def_id): (CrateNum, DefId)) -> DepNode<DefId> {
Expand All @@ -532,6 +557,12 @@ fn mir_shim_dep_node(instance: ty::InstanceDef) -> DepNode<DefId> {
instance.dep_node()
}

fn symbol_name_dep_node(instance: ty::Instance) -> DepNode<DefId> {
// symbol_name uses the substs only to traverse them to find the
// hash, and that does not create any new dep-nodes.
DepNode::SymbolName(instance.def.def_id())
}

fn typeck_item_bodies_dep_node(_: CrateNum) -> DepNode<DefId> {
DepNode::TypeckBodiesKrate
}
Expand Down
20 changes: 20 additions & 0 deletions src/librustc/ty/mod.rs
Expand Up @@ -38,6 +38,7 @@ use serialize::{self, Encodable, Encoder};
use std::cell::{Cell, RefCell, Ref};
use std::collections::BTreeMap;
use std::cmp;
use std::fmt;
use std::hash::{Hash, Hasher};
use std::iter::FromIterator;
use std::ops::Deref;
Expand Down Expand Up @@ -2745,3 +2746,22 @@ impl<'tcx> DtorckConstraint<'tcx> {
self.dtorck_types.retain(|&val| dtorck_types.replace(val).is_none());
}
}

#[derive(Clone, PartialEq, Eq, PartialOrd, Ord)]
pub struct SymbolName {
// FIXME: we don't rely on interning or equality here - better have
// this be a `&'tcx str`.
pub name: InternedString
}

impl Deref for SymbolName {
type Target = str;

fn deref(&self) -> &str { &self.name }
}

impl fmt::Display for SymbolName {
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
fmt::Display::fmt(&self.name, fmt)
}
}
4 changes: 2 additions & 2 deletions src/librustc_driver/driver.rs
Expand Up @@ -225,8 +225,6 @@ pub fn compile_input(sess: &Session,
sess.code_stats.borrow().print_type_sizes();
}

if ::std::env::var("SKIP_LLVM").is_ok() { ::std::process::exit(0); }

let phase5_result = phase_5_run_llvm_passes(sess, &trans, &outputs);

controller_entry_point!(after_llvm,
Expand Down Expand Up @@ -895,13 +893,15 @@ pub fn phase_3_run_analysis_passes<'tcx, F, R>(sess: &'tcx Session,
mir::provide(&mut local_providers);
reachable::provide(&mut local_providers);
rustc_privacy::provide(&mut local_providers);
trans::provide(&mut local_providers);
typeck::provide(&mut local_providers);
ty::provide(&mut local_providers);
reachable::provide(&mut local_providers);
rustc_const_eval::provide(&mut local_providers);

let mut extern_providers = ty::maps::Providers::default();
cstore::provide(&mut extern_providers);
trans::provide(&mut extern_providers);
ty::provide_extern(&mut extern_providers);
// FIXME(eddyb) get rid of this once we replace const_eval with miri.
rustc_const_eval::provide(&mut extern_providers);
Expand Down
17 changes: 7 additions & 10 deletions src/librustc_trans/back/symbol_export.rs
Expand Up @@ -11,7 +11,6 @@
use context::SharedCrateContext;
use monomorphize::Instance;
use symbol_map::SymbolMap;
use back::symbol_names::symbol_name;
use util::nodemap::FxHashMap;
use rustc::hir::def_id::{DefId, CrateNum, LOCAL_CRATE};
use rustc::session::config;
Expand Down Expand Up @@ -56,7 +55,7 @@ impl ExportedSymbols {
let name = symbol_for_def_id(scx.tcx(), def_id, symbol_map);
let export_level = export_level(scx, def_id);
debug!("EXPORTED SYMBOL (local): {} ({:?})", name, export_level);
(name, export_level)
(str::to_owned(&name), export_level)
})
.collect();

Expand Down Expand Up @@ -108,7 +107,7 @@ impl ExportedSymbols {
.exported_symbols(cnum)
.iter()
.map(|&def_id| {
let name = symbol_name(Instance::mono(scx.tcx(), def_id), scx.tcx());
let name = scx.tcx().symbol_name(Instance::mono(scx.tcx(), def_id));
let export_level = if special_runtime_crate {
// We can probably do better here by just ensuring that
// it has hidden visibility rather than public
Expand All @@ -117,9 +116,9 @@ impl ExportedSymbols {
//
// In general though we won't link right if these
// symbols are stripped, and LTO currently strips them.
if name == "rust_eh_personality" ||
name == "rust_eh_register_frames" ||
name == "rust_eh_unregister_frames" {
if &*name == "rust_eh_personality" ||
&*name == "rust_eh_register_frames" ||
&*name == "rust_eh_unregister_frames" {
SymbolExportLevel::C
} else {
SymbolExportLevel::Rust
Expand All @@ -128,7 +127,7 @@ impl ExportedSymbols {
export_level(scx, def_id)
};
debug!("EXPORTED SYMBOL (re-export): {} ({:?})", name, export_level);
(name, export_level)
(str::to_owned(&name), export_level)
})
.collect();

Expand Down Expand Up @@ -228,7 +227,5 @@ fn symbol_for_def_id<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,

let instance = Instance::mono(tcx, def_id);

symbol_map.get(TransItem::Fn(instance))
.map(str::to_owned)
.unwrap_or_else(|| symbol_name(instance, tcx))
str::to_owned(&tcx.symbol_name(instance))
}
50 changes: 43 additions & 7 deletions src/librustc_trans/back/symbol_names.rs
Expand Up @@ -105,14 +105,24 @@ use rustc::hir::map as hir_map;
use rustc::ty::{self, Ty, TyCtxt, TypeFoldable};
use rustc::ty::fold::TypeVisitor;
use rustc::ty::item_path::{self, ItemPathBuffer, RootMode};
use rustc::ty::maps::Providers;
use rustc::ty::subst::Substs;
use rustc::hir::map::definitions::DefPathData;
use rustc::util::common::record_time;

use syntax::attr;
use syntax_pos::symbol::Symbol;

use std::fmt::Write;

pub fn provide(providers: &mut Providers) {
*providers = Providers {
def_symbol_name,
symbol_name,
..*providers
};
}

fn get_symbol_hash<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,

// the DefId of the item this name is for
Expand Down Expand Up @@ -165,8 +175,25 @@ fn get_symbol_hash<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
format!("h{:016x}", hasher.finish())
}

pub fn symbol_name<'a, 'tcx>(instance: Instance<'tcx>,
tcx: TyCtxt<'a, 'tcx, 'tcx>) -> String {
fn def_symbol_name<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId)
-> ty::SymbolName
{
let mut buffer = SymbolPathBuffer::new();
item_path::with_forced_absolute_paths(|| {
tcx.push_item_path(&mut buffer, def_id);
});
buffer.into_interned()
}

fn symbol_name<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, instance: Instance<'tcx>)
-> ty::SymbolName
{
ty::SymbolName { name: Symbol::intern(&compute_symbol_name(tcx, instance)).as_str() }
}

fn compute_symbol_name<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, instance: Instance<'tcx>)
-> String
{
let def_id = instance.def_id();
let substs = instance.substs;

Expand Down Expand Up @@ -253,11 +280,7 @@ pub fn symbol_name<'a, 'tcx>(instance: Instance<'tcx>,

let hash = get_symbol_hash(tcx, Some(def_id), instance_ty, Some(substs));

let mut buffer = SymbolPathBuffer::new();
item_path::with_forced_absolute_paths(|| {
tcx.push_item_path(&mut buffer, def_id);
});
buffer.finish(&hash)
SymbolPathBuffer::from_interned(tcx.def_symbol_name(def_id)).finish(&hash)
}

// Follow C++ namespace-mangling style, see
Expand Down Expand Up @@ -288,6 +311,19 @@ impl SymbolPathBuffer {
result
}

fn from_interned(symbol: ty::SymbolName) -> Self {
let mut result = SymbolPathBuffer {
result: String::with_capacity(64),
temp_buf: String::with_capacity(16)
};
result.result.push_str(&symbol.name);
result
}

fn into_interned(self) -> ty::SymbolName {
ty::SymbolName { name: Symbol::intern(&self.result).as_str() }
}

fn finish(mut self, hash: &str) -> String {
// end name-sequence
self.push(hash);
Expand Down
8 changes: 3 additions & 5 deletions src/librustc_trans/base.rs
Expand Up @@ -65,7 +65,6 @@ use meth;
use mir;
use monomorphize::{self, Instance};
use partitioning::{self, PartitioningStrategy, CodegenUnit};
use symbol_cache::SymbolCache;
use symbol_map::SymbolMap;
use symbol_names_test;
use trans_item::{TransItem, DefPathBasedNames};
Expand Down Expand Up @@ -1139,8 +1138,7 @@ pub fn trans_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,

let cgu_name = String::from(cgu.name());
let cgu_id = cgu.work_product_id();
let symbol_cache = SymbolCache::new(scx.tcx());
let symbol_name_hash = cgu.compute_symbol_name_hash(scx, &symbol_cache);
let symbol_name_hash = cgu.compute_symbol_name_hash(scx);

// Check whether there is a previous work-product we can
// re-use. Not only must the file exist, and the inputs not
Expand Down Expand Up @@ -1175,11 +1173,11 @@ pub fn trans_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
}

// Instantiate translation items without filling out definitions yet...
let lcx = LocalCrateContext::new(scx, cgu, &symbol_cache);
let lcx = LocalCrateContext::new(scx, cgu);
let module = {
let ccx = CrateContext::new(scx, &lcx);
let trans_items = ccx.codegen_unit()
.items_in_deterministic_order(ccx.tcx(), &symbol_cache);
.items_in_deterministic_order(ccx.tcx());
for &(trans_item, linkage) in &trans_items {
trans_item.predefine(&ccx, linkage);
}
Expand Down
3 changes: 1 addition & 2 deletions src/librustc_trans/callee.rs
Expand Up @@ -23,7 +23,6 @@ use monomorphize::{self, Instance};
use rustc::hir::def_id::DefId;
use rustc::ty::TypeFoldable;
use rustc::ty::subst::Substs;
use trans_item::TransItem;
use type_of;

/// Translates a reference to a fn/method item, monomorphizing and
Expand All @@ -50,7 +49,7 @@ pub fn get_fn<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
return llfn;
}

let sym = ccx.symbol_cache().get(TransItem::Fn(instance));
let sym = tcx.symbol_name(instance);
debug!("get_fn({:?}: {:?}) => {}", instance, fn_ty, sym);

// This is subtle and surprising, but sometimes we have to bitcast
Expand Down
9 changes: 3 additions & 6 deletions src/librustc_trans/consts.rs
Expand Up @@ -8,8 +8,6 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.


use back::symbol_names;
use llvm;
use llvm::{SetUnnamedAddr};
use llvm::{ValueRef, True};
Expand Down Expand Up @@ -93,8 +91,7 @@ pub fn get_static(ccx: &CrateContext, def_id: DefId) -> ValueRef {
hir_map::NodeItem(&hir::Item {
ref attrs, span, node: hir::ItemStatic(..), ..
}) => {
let sym = ccx.symbol_cache()
.get(TransItem::Static(id));
let sym = TransItem::Static(id).symbol_name(ccx.tcx());

let defined_in_current_codegen_unit = ccx.codegen_unit()
.items()
Expand All @@ -113,7 +110,7 @@ pub fn get_static(ccx: &CrateContext, def_id: DefId) -> ValueRef {
hir_map::NodeForeignItem(&hir::ForeignItem {
ref attrs, span, node: hir::ForeignItemStatic(..), ..
}) => {
let sym = symbol_names::symbol_name(instance, ccx.tcx());
let sym = ccx.tcx().symbol_name(instance);
let g = if let Some(name) =
attr::first_attr_value_str_by_name(&attrs, "linkage") {
// If this is a static with a linkage specified, then we need to handle
Expand Down Expand Up @@ -173,7 +170,7 @@ pub fn get_static(ccx: &CrateContext, def_id: DefId) -> ValueRef {

g
} else {
let sym = symbol_names::symbol_name(instance, ccx.tcx());
let sym = ccx.tcx().symbol_name(instance);

// FIXME(nagisa): perhaps the map of externs could be offloaded to llvm somehow?
// FIXME(nagisa): investigate whether it can be changed into define_global
Expand Down

0 comments on commit a517343

Please sign in to comment.