Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unused DefPathTable::retrace_path() #43361

Merged
merged 1 commit into from
Jul 24, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/librustc/hir/def_id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,10 @@ impl DefIndex {
pub fn as_array_index(&self) -> usize {
(self.0 & !DEF_INDEX_HI_START.0) as usize
}

pub fn from_array_index(i: usize, address_space: DefIndexAddressSpace) -> DefIndex {
DefIndex::new(address_space.start() + i)
}
}

/// The start of the "high" range of DefIndexes.
Expand Down
85 changes: 19 additions & 66 deletions src/librustc/hir/map/definitions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use hir;
use hir::def_id::{CrateNum, DefId, DefIndex, LOCAL_CRATE, DefIndexAddressSpace,
CRATE_DEF_INDEX};
use ich::Fingerprint;
use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_data_structures::indexed_vec::IndexVec;
use rustc_data_structures::stable_hasher::StableHasher;
use serialize::{Encodable, Decodable, Encoder, Decoder};
Expand All @@ -36,7 +36,6 @@ use util::nodemap::NodeMap;
/// There is one DefPathTable for each crate.
pub struct DefPathTable {
index_to_key: [Vec<DefKey>; 2],
key_to_index: FxHashMap<DefKey, DefIndex>,
def_path_hashes: [Vec<DefPathHash>; 2],
}

Expand All @@ -47,7 +46,6 @@ impl Clone for DefPathTable {
DefPathTable {
index_to_key: [self.index_to_key[0].clone(),
self.index_to_key[1].clone()],
key_to_index: self.key_to_index.clone(),
def_path_hashes: [self.def_path_hashes[0].clone(),
self.def_path_hashes[1].clone()],
}
Expand All @@ -65,10 +63,9 @@ impl DefPathTable {
let index_to_key = &mut self.index_to_key[address_space.index()];
let index = DefIndex::new(index_to_key.len() + address_space.start());
debug!("DefPathTable::insert() - {:?} <-> {:?}", key, index);
index_to_key.push(key.clone());
index_to_key.push(key);
index
};
self.key_to_index.insert(key, index);
self.def_path_hashes[address_space.index()].push(def_path_hash);
debug_assert!(self.def_path_hashes[address_space.index()].len() ==
self.index_to_key[address_space.index()].len());
Expand All @@ -87,47 +84,6 @@ impl DefPathTable {
[index.as_array_index()]
}

#[inline(always)]
pub fn def_index_for_def_key(&self, key: &DefKey) -> Option<DefIndex> {
self.key_to_index.get(key).cloned()
}

#[inline(always)]
pub fn contains_key(&self, key: &DefKey) -> bool {
self.key_to_index.contains_key(key)
}

pub fn retrace_path(&self,
path_data: &[DisambiguatedDefPathData])
-> Option<DefIndex> {
let root_key = DefKey {
parent: None,
disambiguated_data: DisambiguatedDefPathData {
data: DefPathData::CrateRoot,
disambiguator: 0,
},
};

let root_index = self.key_to_index
.get(&root_key)
.expect("no root key?")
.clone();

debug!("retrace_path: root_index={:?}", root_index);

let mut index = root_index;
for data in path_data {
let key = DefKey { parent: Some(index), disambiguated_data: data.clone() };
debug!("retrace_path: key={:?}", key);
match self.key_to_index.get(&key) {
Some(&i) => index = i,
None => return None,
}
}

Some(index)
}

pub fn add_def_path_hashes_to(&self,
cnum: CrateNum,
out: &mut FxHashMap<DefPathHash, DefId>) {
Expand All @@ -149,7 +105,7 @@ impl DefPathTable {
}

pub fn size(&self) -> usize {
self.key_to_index.len()
self.index_to_key.iter().map(|v| v.len()).sum()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know who calls this? This new version seems significantly more expensive (O(n) vs O(1)), so if it's in any sort of tight-loop, it may be worth keeping a separate counter. I didn't find any obvious callers using ripgrep, but I could easily have missed things...

...if we do keep the O(n) version, maybe rename it to count_keys or something more suggestive that this requires computation? This looks like a "pure getter" to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index_to_key here is an array with two elements as far as I can tell (https://github.com/rust-lang/rust/pull/43361/files#diff-b4047ba27734e3c7e860eb844810a301R38) so probably not an issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mark-Simulacrum is right, this is a 2-element array. And it's called only once per upstream crate IIRC. I would not worry about it.

}
}

Expand Down Expand Up @@ -179,19 +135,8 @@ impl Decodable for DefPathTable {
let index_to_key = [index_to_key_lo, index_to_key_hi];
let def_path_hashes = [def_path_hashes_lo, def_path_hashes_hi];

let mut key_to_index = FxHashMap();

for space in &[DefIndexAddressSpace::Low, DefIndexAddressSpace::High] {
key_to_index.extend(index_to_key[space.index()]
.iter()
.enumerate()
.map(|(index, key)| (key.clone(),
DefIndex::new(index + space.start()))))
}

Ok(DefPathTable {
index_to_key,
key_to_index,
def_path_hashes,
})
}
Expand All @@ -208,6 +153,7 @@ pub struct Definitions {
pub(super) node_to_hir_id: IndexVec<ast::NodeId, hir::HirId>,
macro_def_scopes: FxHashMap<Mark, DefId>,
expansions: FxHashMap<DefIndex, Mark>,
keys_created: FxHashSet<DefKey>,
}

// Unfortunately we have to provide a manual impl of Clone because of the
Expand All @@ -224,6 +170,7 @@ impl Clone for Definitions {
node_to_hir_id: self.node_to_hir_id.clone(),
macro_def_scopes: self.macro_def_scopes.clone(),
expansions: self.expansions.clone(),
keys_created: self.keys_created.clone(),
}
}
}
Expand Down Expand Up @@ -448,14 +395,14 @@ impl Definitions {
Definitions {
table: DefPathTable {
index_to_key: [vec![], vec![]],
key_to_index: FxHashMap(),
def_path_hashes: [vec![], vec![]],
},
node_to_def_index: NodeMap(),
def_index_to_node: [vec![], vec![]],
node_to_hir_id: IndexVec::new(),
macro_def_scopes: FxHashMap(),
expansions: FxHashMap(),
keys_created: FxHashSet(),
}
}

Expand All @@ -478,10 +425,6 @@ impl Definitions {
self.table.def_path_hash(index)
}

pub fn def_index_for_def_key(&self, key: DefKey) -> Option<DefIndex> {
self.table.def_index_for_def_key(&key)
}

/// Returns the path from the crate root to `index`. The root
/// nodes are not included in the path (i.e., this will be an
/// empty vector for the crate root). For an inlined item, this
Expand Down Expand Up @@ -583,9 +526,10 @@ impl Definitions {
}
};

while self.table.contains_key(&key) {
while self.keys_created.contains(&key) {
key.disambiguated_data.disambiguator += 1;
}
self.keys_created.insert(key.clone());

let parent_hash = self.table.def_path_hash(parent);
let def_path_hash = key.compute_stable_hash(parent_hash);
Expand Down Expand Up @@ -710,6 +654,8 @@ macro_rules! define_global_metadata_kind {
$($variant),*
}

const GLOBAL_MD_ADDRESS_SPACE: DefIndexAddressSpace = DefIndexAddressSpace::High;

impl GlobalMetaDataKind {
fn allocate_def_indices(definitions: &mut Definitions) {
$({
Expand All @@ -718,7 +664,7 @@ macro_rules! define_global_metadata_kind {
CRATE_DEF_INDEX,
ast::DUMMY_NODE_ID,
DefPathData::GlobalMetaData(instance.name()),
DefIndexAddressSpace::High,
GLOBAL_MD_ADDRESS_SPACE,
Mark::root()
);

Expand All @@ -736,7 +682,14 @@ macro_rules! define_global_metadata_kind {
}
};

def_path_table.key_to_index[&def_key]
// These DefKeys are all right after the root,
// so a linear search is fine.
let index = def_path_table.index_to_key[GLOBAL_MD_ADDRESS_SPACE.index()]
.iter()
.position(|k| *k == def_key)
.unwrap();

DefIndex::from_array_index(index, GLOBAL_MD_ADDRESS_SPACE)
}

fn name(&self) -> Symbol {
Expand Down
6 changes: 1 addition & 5 deletions src/librustc/hir/map/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ pub use self::definitions::{Definitions, DefKey, DefPath, DefPathData,

use dep_graph::{DepGraph, DepNode, DepKind};

use hir::def_id::{CRATE_DEF_INDEX, DefId, DefIndex, DefIndexAddressSpace};
use hir::def_id::{CRATE_DEF_INDEX, DefId, DefIndexAddressSpace};

use syntax::abi::Abi;
use syntax::ast::{self, Name, NodeId, CRATE_NODE_ID};
Expand Down Expand Up @@ -377,10 +377,6 @@ impl<'hir> Map<'hir> {
self.definitions.def_path(def_id.index)
}

pub fn def_index_for_def_key(&self, def_key: DefKey) -> Option<DefIndex> {
self.definitions.def_index_for_def_key(def_key)
}

pub fn local_def_id(&self, node: NodeId) -> DefId {
self.opt_local_def_id(node).unwrap_or_else(|| {
bug!("local_def_id: no entry for `{}`, which has a map of `{:?}`",
Expand Down
14 changes: 1 addition & 13 deletions src/librustc/middle/cstore.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,7 @@
use hir::def;
use hir::def_id::{CrateNum, DefId, DefIndex};
use hir::map as hir_map;
use hir::map::definitions::{Definitions, DefKey, DisambiguatedDefPathData,
DefPathTable};
use hir::map::definitions::{Definitions, DefKey, DefPathTable};
use hir::svh::Svh;
use ich;
use middle::lang_items;
Expand Down Expand Up @@ -269,10 +268,6 @@ pub trait CrateStore {
fn is_no_builtins(&self, cnum: CrateNum) -> bool;

// resolve
fn retrace_path(&self,
cnum: CrateNum,
path_data: &[DisambiguatedDefPathData])
-> Option<DefId>;
fn def_key(&self, def: DefId) -> DefKey;
fn def_path(&self, def: DefId) -> hir_map::DefPath;
fn def_path_hash(&self, def: DefId) -> hir_map::DefPathHash;
Expand Down Expand Up @@ -392,13 +387,6 @@ impl CrateStore for DummyCrateStore {
fn is_no_builtins(&self, cnum: CrateNum) -> bool { bug!("is_no_builtins") }

// resolve
fn retrace_path(&self,
cnum: CrateNum,
path_data: &[DisambiguatedDefPathData])
-> Option<DefId> {
None
}

fn def_key(&self, def: DefId) -> DefKey { bug!("def_key") }
fn def_path(&self, def: DefId) -> hir_map::DefPath {
bug!("relative_def_path")
Expand Down
19 changes: 1 addition & 18 deletions src/librustc/ty/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use hir::TraitMap;
use hir::def::{Def, ExportMap};
use hir::def_id::{CrateNum, DefId, LOCAL_CRATE};
use hir::map as hir_map;
use hir::map::{DisambiguatedDefPathData, DefPathHash};
use hir::map::DefPathHash;
use middle::free_region::FreeRegionMap;
use middle::lang_items;
use middle::resolve_lifetime;
Expand Down Expand Up @@ -570,23 +570,6 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
}
}

pub fn retrace_path(self,
krate: CrateNum,
path_data: &[DisambiguatedDefPathData])
-> Option<DefId> {
debug!("retrace_path(path={:?}, krate={:?})", path_data, self.crate_name(krate));

if krate == LOCAL_CRATE {
self.hir
.definitions()
.def_path_table()
.retrace_path(path_data)
.map(|def_index| DefId { krate: krate, index: def_index })
} else {
self.sess.cstore.retrace_path(krate, path_data)
}
}

pub fn alloc_generics(self, generics: ty::Generics) -> &'gcx ty::Generics {
self.global_arenas.generics.alloc(generics)
}
Expand Down
12 changes: 1 addition & 11 deletions src/librustc_metadata/cstore_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use rustc::session::Session;
use rustc::ty::{self, TyCtxt};
use rustc::ty::maps::Providers;
use rustc::hir::def_id::{CrateNum, DefId, DefIndex, CRATE_DEF_INDEX, LOCAL_CRATE};
use rustc::hir::map::{DefKey, DefPath, DisambiguatedDefPathData, DefPathHash};
use rustc::hir::map::{DefKey, DefPath, DefPathHash};
use rustc::hir::map::blocks::FnLikeNode;
use rustc::hir::map::definitions::{DefPathTable, GlobalMetaDataKind};
use rustc::util::nodemap::{NodeSet, DefIdMap};
Expand Down Expand Up @@ -307,16 +307,6 @@ impl CrateStore for cstore::CStore {
self.get_crate_data(cnum).is_no_builtins(&self.dep_graph)
}

fn retrace_path(&self,
cnum: CrateNum,
path: &[DisambiguatedDefPathData])
-> Option<DefId> {
let cdata = self.get_crate_data(cnum);
cdata.def_path_table
.retrace_path(&path)
.map(|index| DefId { krate: cnum, index: index })
}

/// Returns the `DefKey` for a given `DefId`. This indicates the
/// parent `DefId` as well as some idea of what kind of data the
/// `DefId` refers to.
Expand Down