employer
/
tame
1
0
Fork 0
Code Packages Releases Activity

Revert "tamer: asg::graph::index: Use FxHashMap in place of Vec" 

This reverts commit 1b7eac337cd5909c01ede3a5b3fba577898d5961.

I don't actually think this ends up being worth it in the end.  Sure, the
implementation is simpler at a glance, but it is more complex at runtime,
adding more cycles for little benefit.

There are ~220 pre-interned symbols at the time of writing, so ~880 bytes (4
bytes per symbol) are potentially wasted if _none_ of the pre-interned
symbols end up serving as identifiers in the graph.  The reality is that
some of them _will_ but, but using HashMap also introduces overhead, so in
practice, the savings is much less.  On a fairly small package, it was <100
bytes memory saving in `tamec`.  For `tameld`, it actually uses _more_
memory, especially on larger packages, because there are 10s of thousands of
symbols involved.  And we're incurring a rehashing cost on resize, unlike
this original plain `Vec` implementation.

So, I'm leaving this in the history to reference in the future or return to
it if others ask; maybe it'll be worth it in the future.
main
Mike Gerwitz 2023-01-27 09:54:26 -05:00
parent 417df548cf
commit d066bb370f
1 changed files with 34 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

42
tamer/src/asg/graph.rs
View File

@ -33,7 +33,6 @@ use crate::{
parse::{util::SPair, Token},
sym::SymbolId,
};
use fxhash::FxHashMap;
use petgraph::{
graph::{DiGraph, Graph, NodeIndex},
visit::EdgeRef,
@ -93,7 +92,10 @@ pub struct Asg {
/// Note that,
/// while we store [`NodeIndex`] internally,
/// the public API encapsulates it within an [`ObjectIndex`].
index: FxHashMap<SymbolId, NodeIndex<Ix>>,
index: Vec<NodeIndex<Ix>>,
/// Empty node indicating that no object exists for a given index.
empty_node: NodeIndex<Ix>,
/// The root node used for reachability analysis and topological
/// sorting.
@ -127,8 +129,12 @@ impl Asg {
/// edge to another object.
pub fn with_capacity(objects: usize, edges: usize) -> Self {
let mut graph = Graph::with_capacity(objects, edges);
let index =
FxHashMap::with_capacity_and_hasher(objects, Default::default());
let mut index = Vec::with_capacity(objects);
// Exhaust the first index to be used as a placeholder
// (its value does not matter).
let empty_node = graph.add_node(Object::Root.into());
index.push(empty_node);
// Automatically add the root which will be used to determine what
// identifiers ought to be retained by the final program.
@ -138,6 +144,7 @@ impl Asg {
Self {
graph,
index,
empty_node,
root_node,
}
}
@ -154,11 +161,27 @@ impl Asg {
/// After an identifier is indexed it is not expected to be reassigned
/// to another node.
/// Debug builds contain an assertion that will panic in this instance.
///
/// Panics
/// ======
/// Will panic if unable to allocate more space for the index.
fn index_identifier(&mut self, name: SymbolId, node: NodeIndex<Ix>) {
let prev = self.index.insert(name, node);
let i = name.as_usize();
if i >= self.index.len() {
// If this is ever a problem we can fall back to usize max and
// re-compare before panicing
let new_size = (i + 1)
.checked_next_power_of_two()
.expect("internal error: cannot allocate space for ASG index");
self.index.resize(new_size, self.empty_node);
}
// We should never overwrite indexes
debug_assert!(prev == None);
debug_assert!(self.index[i] == self.empty_node);
self.index[i] = node;
}
/// Lookup `ident` or add a missing identifier to the graph and return a
@ -594,9 +617,12 @@ impl Asg {
/// that, see [`Asg::get`].
#[inline]
pub fn lookup(&self, id: SPair) -> Option<ObjectIndex<Ident>> {
let i = id.symbol().as_usize();
self.index
.get(&id.symbol())
.map(|&ni| ObjectIndex::new(ni, id.span()))
.get(i)
.filter(|ni| ni.index() > 0)
.map(|ni| ObjectIndex::new(*ni, id.span()))
}
/// Declare that `dep` is a dependency of `ident`.