h/fish-shell
1
Fork 0
mirror of https://github.com/fish-shell/fish-shell.git synced 2026-06-03 06:41:14 -03:00
Code Issues Packages Projects Releases Wiki Activity

ast: Bravely stop boxing items in lists

Browse Source 
Prior to this commit, lists of items (e.g. an argument list to a command) would
each be Boxed, i.e. we had effectively Vec<Box<Item>>. The rationale here is
that we had raw pointers and pointer stability was important to enforce.

But we have fewer raw pointers now - only the parent pointers - and we can be
confident that the Ast will not change or move after construction. So remove
this intermediate Box, simplifying some logic and reducing ast size by ~5%.

This slows down Ast construction because we're still constructing
the Box and moving things in and out of it - that will be addressed in
subsequent commits.
This commit is contained in:
Peter Ammon
2025-03-16 14:48:25 -07:00
parent 3b3063287b
commit 0b4883d07f
3 changed files with 15 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files

25
src/ast.rs
View File

@@ -438,8 +438,8 @@ fn allows_keyword(&self, kw: ParseKeyword) -> bool {
// A simple variable-sized array, possibly empty.
pub trait List: Node {
type ContentsNode: Node + Default;
fn contents(&self) -> &[Box<Self::ContentsNode>];
fn contents_mut(&mut self) -> &mut Box<[Box<Self::ContentsNode>]>;
fn contents(&self) -> &[Self::ContentsNode];
fn contents_mut(&mut self) -> &mut Box<[Self::ContentsNode]>;
/// Return our count.
fn count(&self) -> usize {
self.contents().len()
@@ -449,11 +449,11 @@ fn is_empty(&self) -> bool {
self.contents().is_empty()
}
/// Iteration support.
fn iter(&self) -> std::slice::Iter<Box<Self::ContentsNode>> {
fn iter(&self) -> std::slice::Iter<Self::ContentsNode> {
self.contents().iter()
}
fn get(&self, index: usize) -> Option<&Self::ContentsNode> {
self.contents().get(index).map(|b| &**b)
self.contents().get(index)
}
}
@@ -636,22 +636,21 @@ macro_rules! define_list_node {
#[derive(Default, Debug)]
pub struct $name {
parent: Option<*const dyn Node>,
// Note we box the nodes themselves, for pointer stability.
list_contents: Box<[Box<$contents>]>,
list_contents: Box<[$contents]>,
}
implement_node!($name, list, $type);
impl List for $name {
type ContentsNode = $contents;
fn contents(&self) -> &[Box<Self::ContentsNode>] {
fn contents(&self) -> &[Self::ContentsNode] {
&self.list_contents
}
fn contents_mut(&mut self) -> &mut Box<[Box<Self::ContentsNode>]> {
fn contents_mut(&mut self) -> &mut Box<[Self::ContentsNode]> {
&mut self.list_contents
}
}
impl<'a> IntoIterator for &'a $name {
type Item = &'a Box<$contents>;
type IntoIter = std::slice::Iter<'a, Box<$contents>>;
type Item = &'a $contents;
type IntoIter = std::slice::Iter<'a, $contents>;
fn into_iter(self) -> Self::IntoIter {
self.contents().into_iter()
}
@@ -659,12 +658,12 @@ fn into_iter(self) -> Self::IntoIter {
impl Index<usize> for $name {
type Output = <$name as List>::ContentsNode;
fn index(&self, index: usize) -> &Self::Output {
&*self.contents()[index]
&self.contents()[index]
}
}
impl IndexMut<usize> for $name {
fn index_mut(&mut self, index: usize) -> &mut Self::Output {
&mut *self.contents_mut()[index]
&mut self.contents_mut()[index]
}
}
impl Acceptor for $name {
@@ -3598,7 +3597,7 @@ fn populate_list<ListType: List>(&mut self, list: &mut ListType, exhaust_stream:
if contents.is_empty() {
contents.reserve(16);
}
contents.push(node);
contents.push(*node);
} else if exhaust_stream && self.peek_type(0) != ParseTokenType::terminate {
// We aren't allowed to stop. Produce an error and keep going.
self.consume_excess_token_generating_error()

24
src/builtins/fish_indent.rs
View File

@@ -120,32 +120,10 @@ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
}
}
/// If this is a list node, return the amount of memory in bytes occupied by its allocated pointers.
/// Note this is separate from the memory occupied by the nodes themselves,
/// as nodes are boxed to ensure pointer stability. We don't bother to look at the boxed type.
fn list_pointer_memory_size(n: &dyn Node) -> usize {
let count = match n.typ() {
Type::variable_assignment_list => n.as_variable_assignment_list().unwrap().count(),
Type::argument_or_redirection_list => n.as_argument_or_redirection_list().unwrap().count(),
Type::elseif_clause_list => n.as_elseif_clause_list().unwrap().count(),
Type::job_continuation_list => n.as_job_continuation_list().unwrap().count(),
Type::andor_job_list => n.as_andor_job_list().unwrap().count(),
Type::freestanding_argument_list => 0, // not actually a list node
Type::job_conjunction_continuation_list => {
n.as_job_conjunction_continuation_list().unwrap().count()
}
Type::case_item_list => n.as_case_item_list().unwrap().count(),
Type::argument_list => n.as_argument_list().unwrap().count(),
Type::job_list => n.as_job_list().unwrap().count(),
_ => 0,
};
count * std::mem::size_of::<Box<()>>()
}
impl<'a> NodeVisitor<'a> for AstSizeMetrics {
fn visit(&mut self, node: &'a dyn Node) {
self.node_count += 1;
self.memory_size += node.self_memory_size() + list_pointer_memory_size(node);
self.memory_size += node.self_memory_size();
match node.category() {
Category::branch => self.branch_count += 1,
Category::leaf => self.leaf_count += 1,

4
src/parse_execution.rs
View File

@@ -611,7 +611,7 @@ fn apply_variable_assignments(
}
*block = Some(ctx.parser().push_block(Block::variable_assignment_block()));
for variable_assignment in variable_assignment_list {
let source = self.node_source(&**variable_assignment);
let source = self.node_source(variable_assignment);
let equals_pos = variable_assignment_equals_pos(source).unwrap();
let variable_name = &source[..equals_pos];
let expression = &source[equals_pos + 1..];
@@ -1347,7 +1347,7 @@ fn run_begin_statement(
fn get_argument_nodes(args: &ast::ArgumentList) -> AstArgsList<'_> {
let mut result = AstArgsList::new();
for arg in args {
result.push(&**arg);
result.push(arg);
}
result
}