🚧 wip: interning Tables

Performs better but the tests are all busted, won't even compile.
The instruction decoding change messed that up too. Might roll that back.

lunar_wave_vm/src/state.rs

@@ -5,6 +5,7 @@ use crate::{
 	string_interner::Interner,
 	value::{
 		BogusClosure,
+		Table,
 		Value,
 	},
 };
@@ -59,6 +60,7 @@ pub struct State {
 	current_instructions: Rc <[u32]>,
 	pub upvalues: Vec <Value>,
 	pub si: Interner,
+	tables: Vec <crate::value::Table>,
 }
 
 fn lw_io_write (l: &mut State, num_args: usize) -> usize {
@@ -114,7 +116,7 @@ fn lw_table_concat (l: &mut State, num_args: usize) -> usize {
 	assert_eq! (num_args, 2);
 	
 	let s = {
-		let t = l.reg (0).as_table ().unwrap ().borrow ();
+		let t = &l.tables [l.reg (0).as_table ().unwrap ()];
 		let joiner = l.reg (1).as_str ().unwrap ();
 		
 		let mut s = String::new ();
@@ -135,9 +137,9 @@ fn lw_table_concat (l: &mut State, num_args: usize) -> usize {
 fn lw_table_pack (l: &mut State, num_args: usize) -> usize {
 	let mut v = vec! [];
 	for i in 0..num_args {
-		v.push (l.reg (u8::try_from (i).unwrap ()).clone ());
+		v.push ((Value::from (i + 1), l.reg (u8::try_from (i).unwrap ()).clone ()));
 	}
-	*l.reg_mut (0) = Value::from_iter (v.into_iter ());
+	*l.reg_mut (0) = l.intern_table (Table::from_iter (v));
 	1
 }
 
@@ -197,17 +199,17 @@ impl State {
 			current_instructions,
 			upvalues,
 			si: Default::default (),
+			tables: Default::default (),
 		}
 	}
 	
-	pub fn new_with_args <I: Iterator <Item = String>> (chunk: Chunk, mut si: Interner, args: I) -> Self {
+	pub fn new_with_args <I: Iterator <Item = String>> (chunk: Chunk, si: Interner, args: I) -> Self {
-		let upvalues = Self::upvalues_from_args (&mut si, args);
 		let current_instructions = match chunk.blocks.get (0) {
 			Some (x) => Rc::clone (&x.instructions),
 			None => Rc::from ([]),
 		};
 		
-		Self {
+		let mut that = Self {
 			// TODO: Stack is actually supposed to grow to a limit of
 			// idk 10,000. I thought it was fixed at 256.
 			registers: vec! [Value::Nil; 256],
@@ -217,9 +219,13 @@ impl State {
 			debug_print: false,
 			chunk,
 			current_instructions,
-			upvalues,
+			upvalues: Default::default (),
 			si,
-		}
+			tables: Default::default (),
+		};
+		that.upvalues = that.upvalues_from_args (args);
+		
+		that
 	}
 	
 	pub fn at_breakpoint (&self, bp: &Breakpoint) -> bool {
@@ -227,40 +233,46 @@ impl State {
 		frame.block_idx == bp.block_idx && frame.program_counter == bp.program_counter
 	}
 	
-	pub fn upvalues_from_args <I: Iterator <Item = String>> (si: &mut Interner, args: I) -> Vec <Value>
+	fn intern_table (&mut self, table: Table) -> Value {
+		let t = self.tables.len ();
+		self.tables.push (table);
+		Value::Table (t)
+	}
+	
+	pub fn upvalues_from_args <I: Iterator <Item = String>> (&mut self, args: I) -> Vec <Value>
 	{
-		let arg = args.map (|s| si.intern (&s)).enumerate ();
+		let arg: Vec <_> = args.map (|s| self.si.intern (&s)).enumerate ().collect ();
-		let arg = Value::from_iter (arg.map (|(i, v)| (Value::from (i), Value::String (v))));
+		let arg = self.intern_table (Table::from_iter (arg.into_iter ().map (|(i, v)| (Value::from (i), Value::String (v)))));
 		
 		let io: Vec <_> = [
 			("write", Value::RsFunc (lw_io_write)),
-		].into_iter ().map (|(k, v)| (si.intern (k), v)).collect ();
+		].into_iter ().map (|(k, v)| (Value::String (self.si.intern (k)), v)).collect ();
 		
 		let math: Vec <_> = [
 			("sqrt", Value::RsFunc (lw_sqrt)),
-		].into_iter ().map (|(k, v)| (si.intern (k), v)).collect ();
+		].into_iter ().map (|(k, v)| (Value::String (self.si.intern (k)), v)).collect ();
 		
 		let string: Vec <_> = [
 			("format", Value::RsFunc (lw_string_format)),
-		].into_iter ().map (|(k, v)| (si.intern (k), v)).collect ();
+		].into_iter ().map (|(k, v)| (Value::String (self.si.intern (k)), v)).collect ();
 		
 		let table: Vec <_> = [
 			("concat", Value::RsFunc (lw_table_concat)),
 			("pack", Value::RsFunc (lw_table_pack)),
-		].into_iter ().map (|(k, v)| (si.intern (k), v)).collect ();
+		].into_iter ().map (|(k, v)| (Value::String (self.si.intern (k)), v)).collect ();
 		
-		let env = [
+		let env: Vec <_> = [
 			("arg", arg),
-			("io", Value::from_iter (io.into_iter ())),
+			("io", self.intern_table (Table::from_iter (io))),
-			("math", Value::from_iter (math.into_iter ())),
+			("math", self.intern_table (Table::from_iter (math))),
 			("print", Value::RsFunc (lw_print)),
-			("string", Value::from_iter (string.into_iter ())),
+			("string", self.intern_table (Table::from_iter (string))),
-			("table", Value::from_iter (table.into_iter ())),
+			("table", self.intern_table (Table::from_iter (table))),
 			("tonumber", Value::RsFunc (lw_tonumber)),
-		].into_iter ().map (|(k, v)| (si.intern (k), v));
+		].into_iter ().map (|(k, v)| (Value::String (self.si.intern (k)), v)).collect ();
 		
 		vec! [
-			Value::from_iter (env.into_iter ()),
+			self.intern_table (Table::from_iter (env)),
 		]
 	}
 	
@@ -413,7 +425,7 @@ impl State {
 		
 		let val = match &self.registers [self.stack_top.register_offset + usize::from (b)] {
 			Value::Nil => panic! ("R[B] must not be nil"),
-			Value::Table (t) => t.borrow ().get_str (*key).clone (),
+			Value::Table (t) => self.tables [*t].get_str (*key).clone (),
 			_ => panic! ("R[B] must be a table"),
 		};
 		
@@ -422,13 +434,13 @@ impl State {
 	
 	fn op_get_table (&mut self, a: u8, b: u8, c: u8) {
 		let t = match self.reg (b) {
-			Value::Table (t) => t,
+			Value::Table (t) => *t,
 			_ => panic! ("R[B] must be a table"),
 		};
 		
 		let key = self.reg (c);
 		
-		let val = t.borrow ().get (key.clone ()).clone ();
+		let val = self.tables [t].get (key.clone ()).clone ();
 		
 		*self.reg_mut (a) = val;
 	}
@@ -482,8 +494,9 @@ impl State {
 			self.reg_mut (c)
 		}.clone ();
 		
-		let mut dst = self.reg (a).as_table ()
+		let t = self.reg (a).as_table ()
-		.expect ("SetField only works on tables").borrow_mut ();
+		.expect ("SetField only works on tables");
+		let dst = &mut self.tables [t];
 		
 		dst.insert_str (key, value);
 	}
@@ -666,7 +679,7 @@ impl State {
 					self.upvalues.get (b).unwrap ().clone ()
 				};
 				
-				let table = value.as_table ().expect ("GetTabUp only works on tables").borrow ();
+				let table = &self.tables [value.as_table ().expect ("GetTabUp only works on tables")];
 				
 				let key = match self.constants ().get (c).unwrap () {
 					Value::String (s) => *s,
@@ -679,7 +692,7 @@ impl State {
 				let key = i64::try_from (i.c ()).unwrap ();
 				
 				let value = {
-					let table = self.reg (i.b ()).as_table ().expect ("GetI only works on tables").borrow ();
+					let table = &self.tables [self.reg (i.b ()).as_table ().expect ("GetI only works on tables")];
 					table.get_int (key).clone ()
 				};
 				
@@ -713,7 +726,7 @@ impl State {
 					Value::Nil => Err (make_step_error ("attempt to get length of a nil value"))?,
 					Value::RsFunc (_) => Err (make_step_error ("attempt to get length of a function value"))?,
 					Value::String (s) => self.si.get (*s).len ().into (),
-					Value::Table (t) => t.borrow ().length ().into (),
+					Value::Table (t) => self.tables [*t].length ().into (),
 				};
 				
 				*self.reg_mut (i.a ()) = len;
@@ -784,7 +797,9 @@ impl State {
 				*self.reg_mut (i.a ()) = x;
 			},
 			0x13 => {
-				*self.reg_mut (i.a ()) = Value::Table (Default::default ());
+				let t = self.tables.len ();
+				self.tables.push (Default::default ());
+				*self.reg_mut (i.a ()) = Value::Table (t);
 			},
 			0x33 => {
 				*self.reg_mut (i.a ()) = Value::Boolean (! self.reg (i.b ()).is_truthy());
@@ -879,13 +894,14 @@ impl State {
 				}
 				.clone ();
 				
-				let mut dst = self.reg_mut (i.a ()).as_table ().expect ("SetI only works on tables").borrow_mut ();
+				let t = self.reg_mut (i.a ()).as_table ().expect ("SetI only works on tables");
+				let dst = &mut self.tables [t];
 				
 				dst.insert_int (i64::from (i.b ()), value);
 			},
 			0x4e => {
 				let a = i.a ();
-				let b = i.b ();
+				let b = i.b () as usize;
 				
 				if b == 0 {
 					panic! ("SetList with b == 0 not implemented");
@@ -894,11 +910,12 @@ impl State {
 					panic! ("SetList with k = true not implemented");
 				}
 				
-				let mut dst = self.reg (a).as_table ().expect ("SetList only works on tables").borrow_mut ();
+				let t = self.reg (a).as_table ().expect ("SetList only works on tables");
+				let dst = &mut self.tables [t];
 				
 				for j in 1..=b {
-					let src = self.reg (a + j);
+					let src = self.registers [self.stack_top.register_offset + a as usize + j].clone ();
-					dst.insert_int (i64::from (i.c () + j), src.clone ());
+					dst.insert_int (i64::try_from (i.c () as usize + j).unwrap (), src);
 				}
 			},
 			0x0f => {
@@ -915,8 +932,8 @@ impl State {
 				.clone ();
 				
 				let key = self.constants ().get (b).unwrap ().as_str ().expect ("SetTabUp K[B] must be a string");
-				let table = self.upvalues.get_mut (a).unwrap ().as_table ().unwrap ();
+				let t = self.upvalues.get_mut (a).unwrap ().as_table ().unwrap ();
-				table.borrow_mut ().insert_str (key, value);
+				self.tables [t].insert_str (key, value);
 			},
 			0x23 => {
 				if self.op_sub (i.a (), i.b (), i.c ()) {

lunar_wave_vm/src/tests.rs

@@ -30,7 +30,7 @@ fn calculate_hash<T: Hash>(t: &T) -> u64 {
 /// and returns the output
 
 fn run_chunk (vm: &mut State, args: &[&str], chunk: Chunk) -> Vec <Value> {
-	vm.upvalues = State::upvalues_from_args(&mut vm.si, args.into_iter ().map (|s| s.to_string ()));
+	vm.upvalues = vm.upvalues_from_args(args.into_iter ().map (|s| s.to_string ()));
 	vm.set_chunk (chunk);
 	vm.execute ().unwrap ()
 }

lunar_wave_vm/src/value.rs

@@ -33,7 +33,7 @@ pub enum Value {
 	Integer (i64),
 	RsFunc (fn (&mut crate::state::State, usize) -> usize),
 	String (InternedString),
-	Table (Rc <RefCell <Table>>),
+	Table (usize),
 	
 	// These are all bogus, I haven't figured out how to implement
 	// closures yet
@@ -53,7 +53,7 @@ impl fmt::Debug for Value {
 			Value::Integer (x) => write! (f, "{}", x),
 			Value::RsFunc (x) => write! (f, "function: {:?}", x),
 			Value::String (s) => write! (f, "unimplemented Debug",),
-			Value::Table (t) => write! (f, "{:?}", t.borrow ()),
+			Value::Table (t) => write! (f, "table"),
 			
 			Value::BogusClosure (x) => write! (f, "{:?}", x.borrow ()),
 		}
@@ -76,7 +76,7 @@ impl fmt::Display for Value {
 			Value::Integer (x) => write! (f, "{}", x),
 			Value::RsFunc (x) => write! (f, "function: {:?}", x),
 			Value::String (s) => write! (f, "unimplemented Display"),
-			Value::Table (t) => write! (f, "table: {:?}", std::rc::Rc::as_ptr (t)),
+			Value::Table (t) => write! (f, "table"),
 			
 			Value::BogusClosure (x) => write! (f, "BogusClosure: {:?}", std::rc::Rc::as_ptr (x)),
 		}
@@ -125,31 +125,6 @@ impl From <InternedString> for Value {
 	}
 }
 
-impl From <Table> for Value {
-	fn from (x: Table) -> Self {
-		Self::Table (Rc::new (RefCell::new (x)))
-	}
-}
-
-impl FromIterator <(Value, Value)> for Value {
-	fn from_iter <I: IntoIterator <Item=(Value, Value)>> (i: I) -> Self {
-		let table = Table::from_iter (i);
-		Self::from (table)
-	}
-}
-
-impl FromIterator <(InternedString, Value)> for Value {
-	fn from_iter <I: IntoIterator <Item=(InternedString, Value)>> (i: I) -> Self {
-		Self::from_iter (i.into_iter ().map (|(s, v)| (Value::String (s), v)))
-	}
-}
-
-impl FromIterator <Value> for Value {
-	fn from_iter <I: IntoIterator <Item=Value>> (i: I) -> Self {
-		Self::from_iter ((1..).zip (i.into_iter ()).map (|(i, v)| (Value::from (i), v)))
-	}
-}
-
 impl Eq for Value {}
 
 impl std::hash::Hash for Value {
@@ -160,14 +135,31 @@ impl std::hash::Hash for Value {
 		match self {
 			// TODO: Weaken to a Lua error
 			Self::Nil => panic! ("can't hash a nil value"),
-			Self::Boolean (x) => x.hash (state),
+			Self::Boolean (x) => {
-			Self::Float (x) => x.to_ne_bytes ().hash (state),
+				[0x01].hash (state);
-			Self::Integer (x) => x.hash (state),
+				x.hash (state)
-			
+			},
-			Self::RsFunc (x) => x.hash (state),
+			Self::Float (x) => {
+				[0x02].hash (state);
+				x.to_ne_bytes ().hash (state)
+			},
+			Self::Integer (x) => {
+				[0x03].hash (state);
+				x.hash (state)
+			},
+			Self::RsFunc (x) => {
+				[0x04].hash (state);
+				x.hash (state)
+			},
 			// TODO: Implement string interning so we don't hash the whole string here
-			Self::String (x) => x.hash (state),
+			Self::String (x) => {
-			Self::Table (x) => Rc::as_ptr (&x).hash (state),
+				[0x05].hash (state);
+				x.hash (state)
+			},
+			Self::Table (x) => {
+				[0x06].hash (state);
+				x.hash (state)
+			},
 			
 			Self::BogusClosure (_) => panic! ("can't hash Bogus values"),
 		}
@@ -215,9 +207,9 @@ impl Value {
 		}
 	}
 	
-	pub fn as_table (&self) -> Option <&Rc <RefCell <Table>>> {
+	pub fn as_table (&self) -> Option <usize> {
 		match self {
-			Self::Table (t) => Some (t),
+			Self::Table (t) => Some (*t),
 			_ => None,
 		}
 	}

notes.md

@@ -49,7 +49,9 @@ Did absolutely nothing. I couldn't outsmart LLVM.
 
 ## Remove RefCell
 
-(upcoming)
+Result: Worked well. Dropped from about 3,700 to 3,200 samples. The code got even uglier.
+
+Plan:
 
 I think the `borrow` and `borrow_mut` calls slow down OP_GETFIELD and OP_SETFIELD. I can remove them if I store all the tables in State directly, replacing `Rc <RefCell <Table>>` with my own ref counting. This might
 remove a layer of indirection, too.