Performance improvements (#162)

crates/steel-core/Cargo.toml

@@ -26,14 +26,12 @@ serde = { version = "1.0.193", features = ["derive", "rc"] }
 serde_derive = "1.0.193"
 bincode = "1.3.3"
 pretty = "0.12.1"
-im-lists = "0.7.1"
+im-lists = "0.8.0"
 strsim = "0.11.0"
-
 quickscope = "0.2.0"
 lasso = { version = "0.7.2", features = ["multi-threaded", "serialize"] }
 once_cell = "1.18.0"
 fxhash = "0.2.1"
-# lazy_static = "1.4.0"
 steel-gen = { path = "../steel-gen", version = "0.2.0" }
 steel-parser = { path = "../steel-parser", version = "0.6.0" }
 steel-derive = { path = "../steel-derive", version = "0.5.0" }

crates/steel-core/src/compiler/code_gen.rs

@@ -36,7 +36,6 @@ use crate::rvals::Result;
 pub(crate) static FUNCTION_ID: AtomicUsize = AtomicUsize::new(0);
 
 fn fresh_function_id() -> usize {
-    // println!("{:?}", FUNCTION_ID);
     FUNCTION_ID.fetch_add(1, std::sync::atomic::Ordering::Relaxed)
 }
 
@@ -124,11 +123,11 @@ impl<'a> CodeGenerator<'a> {
         let value = eval_atom(syn)?;
 
         let idx = self.constant_map.add_or_get(value);
+        // First, lets check that the value fits
         self.push(
             LabeledInstruction::builder(OpCode::PUSHCONST)
                 .payload(idx)
-                .contents(syn.clone())
-                .constant(true),
+                .contents(syn.clone()),
         );
         Ok(())
     }
@@ -651,8 +650,7 @@ impl<'a> VisitorMut for CodeGenerator<'a> {
                 // TODO: This is a little suspect, we're doing a bunch of stuff twice
                 // that we really don't need. In fact, we probably can get away with just...
                 // embedding the steel val directly here.
-                .list_contents(crate::parser::ast::ExprKind::Quote(Box::new(quote.clone())))
-                .constant(true),
+                .list_contents(crate::parser::ast::ExprKind::Quote(Box::new(quote.clone()))),
         );
 
         Ok(())

crates/steel-core/src/compiler/program.rs

@@ -175,7 +175,7 @@ pub fn convert_call_globals(instructions: &mut [Instruction]) {
 
                 if let TokenType::Identifier(ident) = ident.ty {
                     match ident {
-                        _ if ident == *PRIM_CONS_SYMBOL => {
+                        _ if ident == *PRIM_CONS_SYMBOL && arity == 2 => {
                             if let Some(x) = instructions.get_mut(i) {
                                 x.op_code = OpCode::CONS;
                                 x.payload_size = 2;
@@ -192,34 +192,55 @@ pub fn convert_call_globals(instructions: &mut [Instruction]) {
                         //         continue;
                         //     }
                         // }
-                        _ if ident == *BOX || ident == *PRIM_BOX => {
+                        _ if ident == *BOX || ident == *PRIM_BOX && arity == 1 => {
                             if let Some(x) = instructions.get_mut(i) {
                                 x.op_code = OpCode::NEWBOX;
                                 continue;
                             }
                         }
 
-                        _ if ident == *UNBOX || ident == *PRIM_UNBOX => {
+                        _ if ident == *UNBOX || ident == *PRIM_UNBOX && arity == 1 => {
                             if let Some(x) = instructions.get_mut(i) {
                                 x.op_code = OpCode::UNBOX;
                                 continue;
                             }
                         }
 
-                        _ if ident == *SETBOX || ident == *PRIM_SETBOX => {
+                        _ if ident == *SETBOX || ident == *PRIM_SETBOX && arity == 1 => {
                             if let Some(x) = instructions.get_mut(i) {
                                 x.op_code = OpCode::SETBOX;
                                 continue;
                             }
                         }
 
-                        _ if ident == *PRIM_CAR => {
+                        _ if ident == *PRIM_CAR && arity == 1 => {
                             if let Some(x) = instructions.get_mut(i) {
                                 x.op_code = OpCode::CAR;
                                 continue;
                             }
                         }
 
+                        _ if ident == *PRIM_CDR && arity == 1 => {
+                            if let Some(x) = instructions.get_mut(i) {
+                                x.op_code = OpCode::CDR;
+                                continue;
+                            }
+                        }
+
+                        _ if ident == *PRIM_NOT && arity == 1 => {
+                            if let Some(x) = instructions.get_mut(i) {
+                                x.op_code = OpCode::NOT;
+                                continue;
+                            }
+                        }
+
+                        _ if ident == *PRIM_NULL && arity == 1 => {
+                            if let Some(x) = instructions.get_mut(i) {
+                                x.op_code = OpCode::NULL;
+                                continue;
+                            }
+                        }
+
                         // _ if ident == *CDR_SYMBOL || ident == *PRIM_CAR => {
                         //     if let Some(x) = instructions.get_mut(i) {
                         //         x.op_code = OpCode::CAR;
@@ -347,9 +368,12 @@ define_primitive_symbols! {
     (PRIM_DIV, DIV) => "/",
     (PRIM_STAR, STAR) => "*",
     (PRIM_EQUAL, EQUAL) => "equal?",
+    (PRIM_NUM_EQUAL, NUM_EQUAL) => "=",
     (PRIM_LTE, LTE) => "<=",
     (PRIM_CAR, CAR_SYMBOL) => "car",
     (PRIM_CDR, CDR_SYMBOL) => "cdr",
+    (PRIM_NOT, NOT_SYMBOL) => "not",
+    (PRIM_NULL, NULL_SYMBOL) => "null?",
 }
 
 define_symbols! {
@@ -435,9 +459,11 @@ pub fn inline_num_operations(instructions: &mut [Instruction]) {
             let replaced = match *ident {
                 x if x == *PRIM_PLUS && *payload_size == 2 => Some(OpCode::BINOPADD),
                 x if x == *PRIM_PLUS && *payload_size > 0 => Some(OpCode::ADD),
+                // x if x == *PRIM_MINUS && *payload_size == 2 => Some(OpCode::BINOPSUB),
                 x if x == *PRIM_MINUS && *payload_size > 0 => Some(OpCode::SUB),
                 x if x == *PRIM_DIV && *payload_size > 0 => Some(OpCode::DIV),
                 x if x == *PRIM_STAR && *payload_size > 0 => Some(OpCode::MUL),
+                x if x == *PRIM_NUM_EQUAL && *payload_size == 2 => Some(OpCode::NUMEQUAL),
                 x if x == *PRIM_EQUAL && *payload_size > 0 => Some(OpCode::EQUAL),
                 x if x == *PRIM_LTE && *payload_size > 0 => Some(OpCode::LTE),
                 _ => None,
@@ -1116,9 +1142,8 @@ fn extract_spans(
                     DenseInstruction::new(
                         i.op_code,
                         i.payload_size.try_into().unwrap_or_else(|_| {
-                            // println!("{:?}", len);
                             println!("{:?}", i);
-                            panic!("Uh oh ")
+                            panic!("Unable to lower instruction to bytecode!")
                         }),
                     )
                 })

crates/steel-core/src/core/instructions.rs

@@ -117,6 +117,8 @@ pub fn disassemble(instructions: &[Instruction]) -> String {
 pub struct DenseInstruction {
     pub op_code: OpCode,
     // Function IDs need to be interned _again_ before patched into the code?
+    // Also: We should be able to get away with a u16 here. Just grab places where u16
+    // won't fit and convert to something else.
     pub payload_size: u32,
 }
 

crates/steel-core/src/core/labels.rs

@@ -23,7 +23,7 @@ pub enum Expr {
 }
 
 #[derive(Clone, Debug)]
-pub struct LabeledInstruction {
+pub struct CompactInstruction {
     pub op_code: OpCode,
     pub payload_size: usize,
     pub contents: Option<Expr>,
@@ -32,6 +32,15 @@ pub struct LabeledInstruction {
     pub constant: bool,
 }
 
+#[derive(Clone, Debug)]
+pub struct LabeledInstruction {
+    pub op_code: OpCode,
+    pub payload_size: usize,
+    pub contents: Option<Expr>,
+    pub tag: Option<Label>,
+    pub goto: Option<Label>,
+}
+
 impl LabeledInstruction {
     pub fn builder(op_code: OpCode) -> Self {
         Self {
@@ -40,7 +49,6 @@ impl LabeledInstruction {
             contents: None,
             tag: None,
             goto: None,
-            constant: false,
         }
     }
 
@@ -69,11 +77,6 @@ impl LabeledInstruction {
         self
     }
 
-    pub fn constant(mut self, constant: bool) -> Self {
-        self.constant = constant;
-        self
-    }
-
     pub fn set_goto(&mut self, goto: Label) {
         self.goto = Some(goto);
     }

crates/steel-core/src/env.rs

@@ -38,12 +38,6 @@ impl Env {
         }
     }
 
-    /// Search starting from the current environment
-    /// for `idx`, looking through the parent chain in order.
-    ///
-    /// if found, return that value
-    ///
-    /// Otherwise, error with `FreeIdentifier`
     #[inline(always)]
     pub fn repl_lookup_idx(&self, idx: usize) -> SteelVal {
         self.bindings_vec[idx].clone()

crates/steel-core/src/primitives/lists.rs

@@ -19,16 +19,16 @@ use crate::core::utils::{
 };
 
 declare_const_ref_functions! {
-    REVERSE => reverse,
     LAST => last,
     TRY_LIST_REF => try_list_ref,
     LIST_TO_STRING => steel_list_to_string,
 }
 
 declare_const_mut_ref_functions! {
+    REVERSE => reverse,
     CONS => cons,
     REST => rest,
-    CDR => cdr,
+    CDR => steel_cdr,
     APPEND => append,
     PUSH_BACK => push_back,
 }
@@ -240,7 +240,7 @@ pub fn new(args: &[SteelVal]) -> Result<SteelVal> {
 /// > (empty? '()) ;; => #true
 /// ```
 #[steel_derive::function(name = "empty?")]
-fn is_empty(list: &SteelVal) -> bool {
+pub fn is_empty(list: &SteelVal) -> bool {
     list.list().map(|x| x.is_empty()).unwrap_or_default()
 }
 
@@ -279,7 +279,10 @@ pub fn cons(args: &mut [SteelVal]) -> Result<SteelVal> {
     if args.len() != 2 {
         stop!(ArityMismatch => "cons takes only two arguments")
     }
-    match (args[0].clone(), &mut args[1]) {
+    match (
+        std::mem::replace(&mut args[0], SteelVal::Void),
+        &mut args[1],
+    ) {
         (left, SteelVal::ListV(right)) => {
             right.cons_mut(left);
 
@@ -317,20 +320,15 @@ macro_rules! debug_unreachable {
 // This is really just for use cases where we absolutely, 100% know, that within the body of a function,
 // it is _not possible_ for the value to be anything but a list. Eliding these checks in hot loops
 // can prove to be beneficial.
-// unsafe fn unsafe_cons(args: &mut [SteelVal]) -> SteelVal {
+
+// pub(crate) fn unsafe_cons(args: &mut [SteelVal]) -> Result<SteelVal> {
 //     match (args[0].clone(), &mut args[1]) {
 //         (left, SteelVal::ListV(right)) => {
 //             right.cons_mut(left);
-
 //             // Consider moving in a default value instead of cloning?
-//             SteelVal::ListV(right.clone())
+//             Ok(SteelVal::ListV(right.clone()))
 //         }
-//         _ => debug_unreachable!(),
-//         // Silly, but this then gives us a special "pair" that is different
-//         // from a real bonafide list
-//         // (left, right) => Ok(SteelVal::Pair(Gc::new(Pair::cons(left, right.clone())))),
-//         // TODO: Replace with an immutable pair here!
-//         // (left, right) => Ok(SteelVal::ListV(vec![left, right.clone()].into())),
+//         _ => unsafe { debug_unreachable!() },
 //     }
 // }
 
@@ -386,11 +384,11 @@ This function takes time proportional to the length of lst."#,
     examples: &[("> (reverse (list 1 2 3 4))", "=> '(4 3 2 1)")],
 };
 
-fn reverse(args: &[SteelVal]) -> Result<SteelVal> {
+fn reverse(args: &mut [SteelVal]) -> Result<SteelVal> {
     arity_check!(reverse, args, 1);
 
-    if let SteelVal::ListV(l) = &args[0] {
-        Ok(SteelVal::ListV(l.clone().reverse()))
+    if let SteelVal::ListV(l) = std::mem::replace(&mut args[0], SteelVal::Void) {
+        Ok(SteelVal::ListV(l.reverse()))
     } else {
         stop!(TypeMismatch => "reverse expects a list")
     }
@@ -447,7 +445,7 @@ fn first(list: &List<SteelVal>) -> Result<SteelVal> {
 /// > (car (cons 2 3)) ;; => 2
 /// ```
 #[steel_derive::function(name = "car", constant = true)]
-fn car(list: &SteelVal) -> Result<SteelVal> {
+pub(crate) fn car(list: &SteelVal) -> Result<SteelVal> {
     match list {
         SteelVal::ListV(l) => l
             .car()
@@ -496,24 +494,23 @@ fn cdr_is_null(args: &[SteelVal]) -> Result<SteelVal> {
     }
 }
 
-fn cdr(args: &mut [SteelVal]) -> Result<SteelVal> {
-    arity_check!(rest, args, 1);
-
-    match &mut args[0] {
-        SteelVal::ListV(l) => {
+#[steel_derive::function(name = "cdr", constant = true)]
+pub(crate) fn cdr(arg: &mut SteelVal) -> Result<SteelVal> {
+    match std::mem::replace(arg, SteelVal::Void) {
+        SteelVal::ListV(mut l) => {
             if l.is_empty() {
                 stop!(Generic => "cdr expects a non empty list");
             }
 
             match l.rest_mut() {
-                Some(l) => Ok(SteelVal::ListV(l.clone())),
-                None => Ok(SteelVal::ListV(l.clone())),
+                Some(_) => Ok(SteelVal::ListV(l)),
+                None => Ok(SteelVal::ListV(l)),
             }
         }
 
         SteelVal::Pair(p) => Ok(p.cdr()),
         _ => {
-            stop!(TypeMismatch => format!("cdr expects a list, found: {}", &args[0]))
+            stop!(TypeMismatch => format!("cdr expects a list, found: {}", &arg))
         }
     }
 }
@@ -766,39 +763,39 @@ mod list_operation_tests {
     #[test]
     fn cdr_normal_input_2_elements() {
         let mut args = [crate::list![1i32, 2i32]];
-        let res = cdr(&mut args);
+        let res = steel_cdr(&mut args);
         let expected = crate::list![2i32];
         assert_eq!(res.unwrap(), expected);
     }
 
     #[test]
     fn cdr_normal_input_3_elements() {
         let mut args = [crate::list![1i32, 2i32, 3i32]];
-        let res = cdr(&mut args);
+        let res = steel_cdr(&mut args);
         let expected = crate::list![2i32, 3i32];
         assert_eq!(res.unwrap(), expected);
     }
 
     #[test]
     fn cdr_bad_input() {
         let mut args = [SteelVal::IntV(1)];
-        let res = cdr(&mut args);
+        let res = steel_cdr(&mut args);
         let expected = ErrorKind::TypeMismatch;
         assert_eq!(res.unwrap_err().kind(), expected);
     }
 
     #[test]
     fn cdr_too_many_args() {
         let mut args = [SteelVal::NumV(1.0), SteelVal::NumV(2.0)];
-        let res = cdr(&mut args);
+        let res = steel_cdr(&mut args);
         let expected = ErrorKind::ArityMismatch;
         assert_eq!(res.unwrap_err().kind(), expected);
     }
 
     #[test]
     fn cdr_single_element_list() {
         let mut args = [SteelVal::ListV(vec![SteelVal::NumV(1.0)].into())];
-        let res = cdr(&mut args);
+        let res = steel_cdr(&mut args);
         let expected = SteelVal::ListV(List::new());
         assert_eq!(res.unwrap(), expected);
     }