Skip to content

Commit

Permalink
Don't generate multiple VM constants for variables with aliases
Browse files Browse the repository at this point in the history
While poking around with Numbat's VM, I noticed if there's a variable
with aliases, like so:

```
@Aliases(foo2)
let foo = 2 m ^ 3
```

then that is (hand-waving a bit here) compiled down as if it were this:

```
let foo = 2 m ^ 3
let foo2 = 2 m ^ 3
```

leading to redundant VM constants (e.g. two 2 constants),
redundant calculations, and even possibly different values if the
right-hand side of the assignment is not pure (if it contains `random()`
for instance).

This commit resolves that duplication by using the variable's name and
all its aliases when resolving identifiers to locals, instead of pushing
a separate local for each alias. While at it, I've also removed the
`depth` field from the `Local` struct, as it is equivalent to the index
of the vector of locals in which the local resides. The only place in
which the field is used is completely gratuituous anyway (the condition
always evaluates to true since all locals in `self.locals[d]` have the
depth `d`).

There are no real performance improvements from this change, but it does
give a nice reduction in the number of instructions and constants
generated by the bytecode writer. Specifically, for a very simple
Numbat program that just evaluates `1`, the number of constants goes
down from 780 to 736, and the number of instructions in the <main>
.CODE section goes down from 1235 to 1055 instructions.
  • Loading branch information
triallax authored and sharkdp committed Mar 9, 2025
1 parent 7aa1155 commit 6496e1d
Showing 1 changed file with 14 additions and 18 deletions.
32 changes: 14 additions & 18 deletions numbat/src/bytecode_interpreter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,7 @@ pub struct LocalMetadata {

#[derive(Debug, Clone)]
pub struct Local {
identifier: CompactString,
depth: usize,
identifiers: Vec<CompactString>,
pub metadata: LocalMetadata,
}

Expand Down Expand Up @@ -62,12 +61,12 @@ impl BytecodeInterpreter {

if let Some(position) = self.locals[current_depth]
.iter()
.rposition(|l| l.identifier == identifier && l.depth == current_depth)
.rposition(|l| l.identifiers.iter().any(|n| n == identifier))
{
self.vm.add_op1(Op::GetLocal, position as u16); // TODO: check overflow
} else if let Some(upvalue_position) = self.locals[0]
.iter()
.rposition(|l| l.identifier == identifier)
.rposition(|l| l.identifiers.iter().any(|n| n == identifier))
{
self.vm.add_op1(Op::GetUpvalue, upvalue_position as u16);
} else if LAST_RESULT_IDENTIFIERS.contains(identifier) {
Expand Down Expand Up @@ -283,25 +282,21 @@ impl BytecodeInterpreter {
let current_depth = self.current_depth();

// For variables, we ignore the prefix info and only use the names
let aliases = crate::decorator::name_and_aliases(identifier, decorators)
let identifiers = crate::decorator::name_and_aliases(identifier, decorators)
.map(|(name, _)| name.to_compact_string())
.collect::<Vec<_>>();
let metadata = LocalMetadata {
name: crate::decorator::name(decorators).map(CompactString::from),
url: crate::decorator::url(decorators).map(CompactString::from),
description: crate::decorator::description(decorators),
aliases: aliases.clone(),
aliases: identifiers.clone(),
};

for alias_name in aliases {
self.compile_expression(expr)?;

self.locals[current_depth].push(Local {
identifier: alias_name.clone(),
depth: current_depth,
metadata: metadata.clone(),
});
}
self.compile_expression(expr)?;
self.locals[current_depth].push(Local {
identifiers,
metadata,
});
Ok(())
}

Expand Down Expand Up @@ -336,8 +331,7 @@ impl BytecodeInterpreter {
let current_depth = self.current_depth();
for parameter in parameters {
self.locals[current_depth].push(Local {
identifier: parameter.1.to_compact_string(),
depth: current_depth,
identifiers: [parameter.1.to_compact_string()].into(),
metadata: LocalMetadata::default(),
});
}
Expand Down Expand Up @@ -544,7 +538,9 @@ impl BytecodeInterpreter {
}

pub fn lookup_global(&self, name: &str) -> Option<&Local> {
self.locals[0].iter().find(|l| l.identifier == name)
self.locals[0]
.iter()
.find(|l| l.identifiers.iter().any(|n| n == name))
}
}

Expand Down

0 comments on commit 6496e1d

Please sign in to comment.