Skip to content

Only store StableCrateId once in DefPathTable. #119259

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

Merged
merged 3 commits into from
Dec 29, 2023
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
2 changes: 1 addition & 1 deletion compiler/rustc_data_structures/src/hashes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ impl Hash64 {
pub const ZERO: Hash64 = Hash64 { inner: 0 };

#[inline]
pub(crate) fn new(n: u64) -> Self {
pub fn new(n: u64) -> Self {
Self { inner: n }
}

Expand Down
17 changes: 9 additions & 8 deletions compiler/rustc_hir/src/def_path_hash_map.rs
Original file line number Diff line number Diff line change
@@ -1,21 +1,22 @@
use rustc_data_structures::fingerprint::Fingerprint;
use rustc_span::def_id::{DefIndex, DefPathHash};
use rustc_data_structures::stable_hasher::Hash64;
use rustc_span::def_id::DefIndex;

#[derive(Clone, Default)]
pub struct Config;

impl odht::Config for Config {
type Key = DefPathHash;
// This hash-map is single-crate, so we only need to key by the local hash.
type Key = Hash64;
type Value = DefIndex;

type EncodedKey = [u8; 16];
type EncodedKey = [u8; 8];
type EncodedValue = [u8; 4];

type H = odht::UnHashFn;

#[inline]
fn encode_key(k: &DefPathHash) -> [u8; 16] {
k.0.to_le_bytes()
fn encode_key(k: &Hash64) -> [u8; 8] {
k.as_u64().to_le_bytes()
}

#[inline]
Expand All @@ -24,8 +25,8 @@ impl odht::Config for Config {
}

#[inline]
fn decode_key(k: &[u8; 16]) -> DefPathHash {
DefPathHash(Fingerprint::from_le_bytes(*k))
fn decode_key(k: &[u8; 8]) -> Hash64 {
Hash64::new(u64::from_le_bytes(*k))
}

#[inline]
Expand Down
49 changes: 27 additions & 22 deletions compiler/rustc_hir/src/definitions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,27 +20,42 @@ use std::hash::Hash;
/// Internally the `DefPathTable` holds a tree of `DefKey`s, where each `DefKey`
/// stores the `DefIndex` of its parent.
/// There is one `DefPathTable` for each crate.
#[derive(Clone, Default, Debug)]
#[derive(Debug)]
pub struct DefPathTable {
stable_crate_id: StableCrateId,
index_to_key: IndexVec<DefIndex, DefKey>,
def_path_hashes: IndexVec<DefIndex, DefPathHash>,
// We do only store the local hash, as all the definitions are from the current crate.
def_path_hashes: IndexVec<DefIndex, Hash64>,
def_path_hash_to_index: DefPathHashMap,
}

impl DefPathTable {
fn new(stable_crate_id: StableCrateId) -> DefPathTable {
DefPathTable {
stable_crate_id,
index_to_key: Default::default(),
def_path_hashes: Default::default(),
def_path_hash_to_index: Default::default(),
}
}

fn allocate(&mut self, key: DefKey, def_path_hash: DefPathHash) -> DefIndex {
Copy link
Member

Choose a reason for hiding this comment

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

FWIW it looks like we're also hashing the parent's stable crate ID into the local part of this hash --

parent.hash(&mut hasher);

It's probably pretty cheap (just an extra u64 hashed in) but seems like it would be correct to skip that hashing since we're not crossing crate boundaries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This changes a lot of hash-dependent behaviour, so I'll investigate this in a follow-up PR.

Copy link
Member

Choose a reason for hiding this comment

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

Perf results don't look particularly meaningful either, so it might not really matter.

// Assert that all DefPathHashes correctly contain the local crate's StableCrateId.
debug_assert_eq!(self.stable_crate_id, def_path_hash.stable_crate_id());
let local_hash = def_path_hash.local_hash();

let index = {
let index = DefIndex::from(self.index_to_key.len());
debug!("DefPathTable::insert() - {:?} <-> {:?}", key, index);
self.index_to_key.push(key);
index
};
self.def_path_hashes.push(def_path_hash);
self.def_path_hashes.push(local_hash);
debug_assert!(self.def_path_hashes.len() == self.index_to_key.len());

// Check for hash collisions of DefPathHashes. These should be
// exceedingly rare.
if let Some(existing) = self.def_path_hash_to_index.insert(&def_path_hash, &index) {
if let Some(existing) = self.def_path_hash_to_index.insert(&local_hash, &index) {
let def_path1 = DefPath::make(LOCAL_CRATE, existing, |idx| self.def_key(idx));
let def_path2 = DefPath::make(LOCAL_CRATE, index, |idx| self.def_key(idx));

Expand All @@ -58,13 +73,6 @@ impl DefPathTable {
);
}

// Assert that all DefPathHashes correctly contain the local crate's
// StableCrateId
#[cfg(debug_assertions)]
if let Some(root) = self.def_path_hashes.get(CRATE_DEF_INDEX) {
assert!(def_path_hash.stable_crate_id() == root.stable_crate_id());
}

index
}

Expand All @@ -73,19 +81,19 @@ impl DefPathTable {
self.index_to_key[index]
}

#[instrument(level = "trace", skip(self), ret)]
#[inline(always)]
pub fn def_path_hash(&self, index: DefIndex) -> DefPathHash {
let hash = self.def_path_hashes[index];
debug!("def_path_hash({:?}) = {:?}", index, hash);
hash
DefPathHash::new(self.stable_crate_id, hash)
}

pub fn enumerated_keys_and_path_hashes(
&self,
) -> impl Iterator<Item = (DefIndex, &DefKey, &DefPathHash)> + ExactSizeIterator + '_ {
) -> impl Iterator<Item = (DefIndex, &DefKey, DefPathHash)> + ExactSizeIterator + '_ {
self.index_to_key
.iter_enumerated()
.map(move |(index, key)| (index, key, &self.def_path_hashes[index]))
.map(move |(index, key)| (index, key, self.def_path_hash(index)))
}
}

Expand All @@ -96,9 +104,6 @@ impl DefPathTable {
pub struct Definitions {
table: DefPathTable,
next_disambiguator: UnordMap<(LocalDefId, DefPathData), u32>,

/// The [StableCrateId] of the local crate.
stable_crate_id: StableCrateId,
}

/// A unique identifier that we can use to lookup a definition
Expand Down Expand Up @@ -329,11 +334,11 @@ impl Definitions {
let def_path_hash = key.compute_stable_hash(parent_hash);

// Create the root definition.
let mut table = DefPathTable::default();
let mut table = DefPathTable::new(stable_crate_id);
let root = LocalDefId { local_def_index: table.allocate(key, def_path_hash) };
assert_eq!(root.local_def_index, CRATE_DEF_INDEX);

Definitions { table, next_disambiguator: Default::default(), stable_crate_id }
Definitions { table, next_disambiguator: Default::default() }
}

/// Adds a definition with a parent definition.
Expand Down Expand Up @@ -375,10 +380,10 @@ impl Definitions {
hash: DefPathHash,
err: &mut dyn FnMut() -> !,
) -> LocalDefId {
debug_assert!(hash.stable_crate_id() == self.stable_crate_id);
debug_assert!(hash.stable_crate_id() == self.table.stable_crate_id);
self.table
.def_path_hash_to_index
.get(&hash)
.get(&hash.local_hash())
.map(|local_def_index| LocalDefId { local_def_index })
.unwrap_or_else(|| err())
}
Expand Down
4 changes: 3 additions & 1 deletion compiler/rustc_metadata/src/rmeta/def_path_hash_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ impl DefPathHashMapRef<'_> {
#[inline]
pub fn def_path_hash_to_def_index(&self, def_path_hash: &DefPathHash) -> DefIndex {
match *self {
DefPathHashMapRef::OwnedFromMetadata(ref map) => map.get(def_path_hash).unwrap(),
DefPathHashMapRef::OwnedFromMetadata(ref map) => {
map.get(&def_path_hash.local_hash()).unwrap()
}
DefPathHashMapRef::BorrowedFromTcx(_) => {
panic!("DefPathHashMap::BorrowedFromTcx variant only exists for serialization")
}
Expand Down