From 9dee5505fb600b3c0b9cbea91f2ad2ebc58eeb22 Mon Sep 17 00:00:00 2001
From: Andrew Morris <voltrevo@gmail.com>
Date: Thu, 2 Nov 2023 23:25:07 +1100
Subject: [PATCH] Avoid unnecessary tranasctions, fix ptr indirection

---
 storage/src/demo_val.rs               | 23 +++++-----------
 storage/src/storage.rs                | 39 +++------------------------
 storage/src/storage_auto_ptr.rs       | 12 ++++-----
 storage/src/storage_backend.rs        | 16 ++++++++++-
 storage/src/storage_entity.rs         |  2 +-
 storage/src/storage_io.rs             | 21 ++++++++-------
 storage/src/tests.rs                  |  2 +-
 valuescript_vm/src/bytecode.rs        |  2 +-
 valuescript_vm/src/native_function.rs | 11 ++++++--
 valuescript_vm/src/number_methods.rs  |  6 ++---
 valuescript_vm/src/operations.rs      |  2 +-
 valuescript_vm/src/string_methods.rs  |  4 +--
 valuescript_vm/src/val_storage.rs     | 10 +++----
 valuescript_vm/src/vs_value.rs        |  1 +
 vstc/src/db_command.rs                |  2 +-
 15 files changed, 65 insertions(+), 88 deletions(-)

diff --git a/storage/src/demo_val.rs b/storage/src/demo_val.rs
index 2222603..61350bb 100644
--- a/storage/src/demo_val.rs
+++ b/storage/src/demo_val.rs
@@ -1,4 +1,3 @@
-use std::error::Error;
 use std::rc::Rc;
 
 use crate::rc_key::RcKey;
@@ -6,7 +5,7 @@ use crate::storage_entity::StorageEntity;
 use crate::storage_entry::{StorageEntry, StorageEntryReader};
 use crate::storage_io::{StorageReader, StorageTxMut};
 use crate::storage_ptr::StorageEntryPtr;
-use crate::{GenericError, Storage, StorageBackend};
+use crate::{GenericError, StorageBackend};
 
 const NUMBER_TAG: u8 = 0;
 const ARRAY_TAG: u8 = 1;
@@ -20,16 +19,6 @@ pub enum DemoVal {
 }
 
 impl DemoVal {
-  pub(crate) fn numbers<SB: StorageBackend>(
-    &self,
-    storage: &mut Storage<SB>,
-  ) -> Result<Vec<u64>, Box<dyn Error>> {
-    storage
-      .sb
-      .borrow()
-      .transaction(Rc::downgrade(&storage.sb), |sb| self.numbers_impl(sb))
-  }
-
   fn write_to_entry<SB: StorageBackend, Tx: StorageTxMut<SB>>(
     &self,
     tx: &mut Tx,
@@ -63,7 +52,7 @@ impl DemoVal {
   }
 
   fn read_from_entry<SB: StorageBackend, Tx: StorageReader<SB>>(
-    _tx: &mut Tx,
+    _tx: &Tx,
     reader: &mut StorageEntryReader,
   ) -> Result<DemoVal, GenericError> {
     let tag = reader.read_u8()?;
@@ -91,7 +80,7 @@ impl DemoVal {
     })
   }
 
-  fn numbers_impl<SB: StorageBackend, Tx: StorageReader<SB>>(
+  pub fn numbers<SB: StorageBackend, Tx: StorageReader<SB>>(
     &self,
     tx: &mut Tx,
   ) -> Result<Vec<u64>, GenericError> {
@@ -99,13 +88,13 @@ impl DemoVal {
       DemoVal::Number(n) => Ok(vec![*n]),
       DemoVal::Ptr(ptr) => {
         let entry = tx.read_or_err(*ptr)?;
-        Self::from_storage_entry(tx, entry)?.numbers_impl(tx)
+        Self::from_storage_entry(tx, entry)?.numbers(tx)
       }
       DemoVal::Array(arr) => {
         let mut numbers = Vec::new();
 
         for item in arr.iter() {
-          numbers.extend(item.numbers_impl(tx)?);
+          numbers.extend(item.numbers(tx)?);
         }
 
         Ok(numbers)
@@ -146,7 +135,7 @@ impl<SB: StorageBackend> StorageEntity<SB> for DemoVal {
   }
 
   fn from_storage_entry<'a, Tx: StorageReader<SB>>(
-    tx: &mut Tx,
+    tx: &Tx,
     entry: StorageEntry,
   ) -> Result<Self, GenericError> {
     Self::read_from_entry(tx, &mut StorageEntryReader::new(&entry))
diff --git a/storage/src/storage.rs b/storage/src/storage.rs
index 53023b1..941d013 100644
--- a/storage/src/storage.rs
+++ b/storage/src/storage.rs
@@ -2,8 +2,6 @@ use std::cell::RefCell;
 use std::error::Error;
 use std::rc::Rc;
 
-use crate::errors::error_str;
-use crate::storage_auto_ptr::StorageAutoPtr;
 use crate::storage_entity::StorageEntity;
 use crate::storage_ptr::{tmp_at_ptr, tmp_count_ptr, StorageEntryPtr, StorageHeadPtr};
 use crate::{StorageBackend, StorageReader, StorageTxMut};
@@ -19,36 +17,6 @@ impl<SB: StorageBackend> Storage<SB> {
     }
   }
 
-  pub fn get_head<SE: StorageEntity<SB>>(
-    &mut self,
-    ptr: StorageHeadPtr,
-  ) -> Result<Option<SE>, Box<dyn Error>> {
-    self
-      .sb
-      .borrow()
-      .transaction(Rc::downgrade(&self.sb), |sb| sb.get_head(ptr))
-  }
-
-  pub fn get<SE: StorageEntity<SB>>(&self, ptr: StorageEntryPtr) -> Result<SE, Box<dyn Error>> {
-    // TODO: Avoid going through a transaction when read-only
-    self.sb.borrow().transaction(Rc::downgrade(&self.sb), |sb| {
-      let entry = sb.read(ptr)?.ok_or(error_str("Ptr not found"))?;
-
-      SE::from_storage_entry(sb, entry)
-    })
-  }
-
-  pub fn get_auto_ptr<SE: StorageEntity<SB>>(
-    &mut self,
-    ptr: StorageEntryPtr,
-  ) -> StorageAutoPtr<SB, SE> {
-    StorageAutoPtr {
-      _marker: std::marker::PhantomData,
-      sb: Rc::downgrade(&self.sb),
-      ptr,
-    }
-  }
-
   pub fn set_head<SE: StorageEntity<SB>>(
     &mut self,
     ptr: StorageHeadPtr,
@@ -113,9 +81,10 @@ impl<SB: StorageBackend> Storage<SB> {
     &mut self,
     ptr: StorageEntryPtr,
   ) -> Result<Option<u64>, Box<dyn Error>> {
-    self.sb.borrow().transaction(Rc::downgrade(&self.sb), |sb| {
-      Ok(sb.read(ptr)?.map(|entry| entry.ref_count))
-    })
+    self
+      .sb
+      .read(ptr)
+      .map(|entry| entry.map(|entry| entry.ref_count))
   }
 }
 
diff --git a/storage/src/storage_auto_ptr.rs b/storage/src/storage_auto_ptr.rs
index e6934b2..ae7bbf8 100644
--- a/storage/src/storage_auto_ptr.rs
+++ b/storage/src/storage_auto_ptr.rs
@@ -25,13 +25,11 @@ impl<SB: StorageBackend, SE: StorageEntity<SB>> StorageAutoPtr<SB, SE> {
       None => return Err("Storage backend dropped".into()),
     };
 
-    let res = sb.borrow().transaction(self.sb.clone(), |sb| {
-      Ok(match sb.read(self.ptr)? {
-        Some(entry) => Some(SE::from_storage_entry(sb, entry)?),
-        None => None,
-      })
-    });
+    let res = match sb.read(self.ptr)? {
+      Some(entry) => Some(SE::from_storage_entry(&sb, entry)?),
+      None => None,
+    };
 
-    res
+    Ok(res)
   }
 }
diff --git a/storage/src/storage_backend.rs b/storage/src/storage_backend.rs
index 4a6a051..e89826f 100644
--- a/storage/src/storage_backend.rs
+++ b/storage/src/storage_backend.rs
@@ -1,4 +1,8 @@
-use std::{cell::RefCell, error::Error, rc::Weak};
+use std::{
+  cell::RefCell,
+  error::Error,
+  rc::{Rc, Weak},
+};
 
 use crate::{
   storage_io::{StorageReader, StorageTxMut},
@@ -29,3 +33,13 @@ pub trait StorageBackend: Sized {
   #[cfg(test)]
   fn len(&self) -> usize;
 }
+
+impl<SB: StorageBackend> StorageReader<SB> for Rc<RefCell<SB>> {
+  fn read_bytes<T>(&self, ptr: StoragePtr<T>) -> Result<Option<Vec<u8>>, GenericError> {
+    self.borrow().read_bytes(ptr)
+  }
+
+  fn get_backend(&self) -> Weak<RefCell<SB>> {
+    Rc::downgrade(self)
+  }
+}
diff --git a/storage/src/storage_entity.rs b/storage/src/storage_entity.rs
index 998ea65..fd8ba96 100644
--- a/storage/src/storage_entity.rs
+++ b/storage/src/storage_entity.rs
@@ -11,7 +11,7 @@ pub trait StorageEntity<SB: StorageBackend>: Sized {
   ) -> Result<StorageEntry, GenericError>;
 
   fn from_storage_entry<Tx: StorageReader<SB>>(
-    tx: &mut Tx,
+    tx: &Tx,
     entry: StorageEntry,
   ) -> Result<Self, GenericError>;
 }
diff --git a/storage/src/storage_io.rs b/storage/src/storage_io.rs
index 69439a4..ce27a0f 100644
--- a/storage/src/storage_io.rs
+++ b/storage/src/storage_io.rs
@@ -4,18 +4,15 @@ use rand::thread_rng;
 use serde::{Deserialize, Serialize};
 
 use crate::{
-  GenericError, RcKey, StorageAutoPtr, StorageBackend, StorageEntity, StorageEntryPtr,
-  StorageHeadPtr, StoragePtr,
+  errors::error_str, GenericError, RcKey, StorageAutoPtr, StorageBackend, StorageEntity,
+  StorageEntryPtr, StorageHeadPtr, StoragePtr,
 };
 
 pub trait StorageReader<SB: StorageBackend>: Sized {
   fn read_bytes<T>(&self, ptr: StoragePtr<T>) -> Result<Option<Vec<u8>>, GenericError>;
   fn get_backend(&self) -> Weak<RefCell<SB>>;
 
-  fn get_auto_ptr<SE: StorageEntity<SB>>(
-    &mut self,
-    ptr: StorageEntryPtr,
-  ) -> StorageAutoPtr<SB, SE> {
+  fn get_auto_ptr<SE: StorageEntity<SB>>(&self, ptr: StorageEntryPtr) -> StorageAutoPtr<SB, SE> {
     StorageAutoPtr {
       _marker: std::marker::PhantomData,
       sb: self.get_backend(),
@@ -24,7 +21,7 @@ pub trait StorageReader<SB: StorageBackend>: Sized {
   }
 
   fn read<T: for<'de> Deserialize<'de>>(
-    &mut self,
+    &self,
     ptr: StoragePtr<T>,
   ) -> Result<Option<T>, GenericError> {
     let data = match self.read_bytes(ptr)? {
@@ -36,7 +33,7 @@ pub trait StorageReader<SB: StorageBackend>: Sized {
   }
 
   fn read_or_err<T: for<'de> Deserialize<'de>>(
-    &mut self,
+    &self,
     ptr: StoragePtr<T>,
   ) -> Result<T, GenericError> {
     match self.read(ptr)? {
@@ -46,7 +43,7 @@ pub trait StorageReader<SB: StorageBackend>: Sized {
   }
 
   fn get_head<SE: StorageEntity<SB>>(
-    &mut self,
+    &self,
     head_ptr: StorageHeadPtr,
   ) -> Result<Option<SE>, GenericError> {
     let entry_ptr = match self.read(head_ptr)? {
@@ -61,6 +58,12 @@ pub trait StorageReader<SB: StorageBackend>: Sized {
 
     SE::from_storage_entry(self, entry).map(Some)
   }
+
+  fn get<SE: StorageEntity<SB>>(&self, ptr: StorageEntryPtr) -> Result<SE, GenericError> {
+    let entry = self.read(ptr)?.ok_or(error_str("Ptr not found"))?;
+
+    SE::from_storage_entry(self, entry)
+  }
 }
 
 pub trait StorageTxMut<SB: StorageBackend>: StorageReader<SB> + Sized {
diff --git a/storage/src/tests.rs b/storage/src/tests.rs
index 6a8d1dc..4228d40 100644
--- a/storage/src/tests.rs
+++ b/storage/src/tests.rs
@@ -4,7 +4,7 @@ mod tests_ {
 
   use crate::{
     demo_val::DemoVal, memory_backend::MemoryBackend, sled_backend::SledBackend, storage::Storage,
-    storage_ptr::storage_head_ptr, StorageBackend,
+    storage_ptr::storage_head_ptr, StorageBackend, StorageReader,
   };
 
   fn run(impl_memory: fn(&mut Storage<MemoryBackend>), impl_sled: fn(&mut Storage<SledBackend>)) {
diff --git a/valuescript_vm/src/bytecode.rs b/valuescript_vm/src/bytecode.rs
index 87c749f..3872085 100644
--- a/valuescript_vm/src/bytecode.rs
+++ b/valuescript_vm/src/bytecode.rs
@@ -58,7 +58,7 @@ impl<SB: StorageBackend> StorageEntity<SB> for Bytecode {
   }
 
   fn from_storage_entry<Tx: StorageReader<SB>>(
-    _tx: &mut Tx,
+    _tx: &Tx,
     entry: storage::StorageEntry,
   ) -> Result<Self, GenericError> {
     Ok(Bytecode::new(entry.data))
diff --git a/valuescript_vm/src/native_function.rs b/valuescript_vm/src/native_function.rs
index 2684804..c35d081 100644
--- a/valuescript_vm/src/native_function.rs
+++ b/valuescript_vm/src/native_function.rs
@@ -19,8 +19,11 @@ impl<'a> ThisWrapper<'a> {
     ThisWrapper { const_, this }
   }
 
-  pub fn get(&self) -> &Val {
-    self.this
+  pub fn get(&self) -> Val {
+    match &self.this {
+      Val::StoragePtr(ptr) => ptr.get(),
+      _ => self.this.clone(),
+    }
   }
 
   pub fn get_mut(&mut self) -> Result<&mut Val, Val> {
@@ -28,6 +31,10 @@ impl<'a> ThisWrapper<'a> {
       return Err("Cannot mutate this because it is const".to_type_error());
     }
 
+    if let Val::StoragePtr(ptr) = &self.this {
+      *self.this = ptr.get()
+    }
+
     Ok(self.this)
   }
 }
diff --git a/valuescript_vm/src/number_methods.rs b/valuescript_vm/src/number_methods.rs
index 901d7cb..166abe1 100644
--- a/valuescript_vm/src/number_methods.rs
+++ b/valuescript_vm/src/number_methods.rs
@@ -63,9 +63,9 @@ static TO_EXPONENTIAL: NativeFunction = native_fn(|this, params| {
           return Err("precision must be between 0 and 100".to_range_error());
         }
 
-        format_exponential(*number, Some(precision as usize))
+        format_exponential(number, Some(precision as usize))
       }
-      None => format_exponential(*number, None),
+      None => format_exponential(number, None),
     },
     _ => return Err("number indirection".to_internal_error()),
   })
@@ -91,7 +91,7 @@ static TO_STRING: NativeFunction = native_fn(|this, params| {
 
 static VALUE_OF: NativeFunction = native_fn(|this, _params| {
   Ok(match this.get() {
-    Val::Number(number) => Val::Number(*number),
+    Val::Number(number) => Val::Number(number),
     _ => return Err("number indirection".to_internal_error()),
   })
 });
diff --git a/valuescript_vm/src/operations.rs b/valuescript_vm/src/operations.rs
index 71eb3a1..c6c19a6 100644
--- a/valuescript_vm/src/operations.rs
+++ b/valuescript_vm/src/operations.rs
@@ -778,7 +778,7 @@ static BOOL_TO_STRING: NativeFunction = native_fn(|this, _params| {
 
 static BOOL_VALUE_OF: NativeFunction = native_fn(|this, _params| {
   Ok(match this.get() {
-    Val::Bool(b) => Val::Bool(*b),
+    Val::Bool(b) => Val::Bool(b),
     _ => return Err("bool indirection".to_type_error()),
   })
 });
diff --git a/valuescript_vm/src/string_methods.rs b/valuescript_vm/src/string_methods.rs
index 2f2e0a6..69fbf6e 100644
--- a/valuescript_vm/src/string_methods.rs
+++ b/valuescript_vm/src/string_methods.rs
@@ -404,7 +404,7 @@ static PAD_START: NativeFunction = native_fn(|this, params| {
         }
       }
 
-      prefix.push_str(string_data);
+      prefix.push_str(&string_data);
 
       prefix.to_val()
     }
@@ -426,7 +426,7 @@ static REPEAT: NativeFunction = native_fn(|this, params| {
       let mut result = String::new();
 
       for _ in 0..count {
-        result.push_str(string_data);
+        result.push_str(&string_data);
       }
 
       result.to_val()
diff --git a/valuescript_vm/src/val_storage.rs b/valuescript_vm/src/val_storage.rs
index b17c5e7..0447297 100644
--- a/valuescript_vm/src/val_storage.rs
+++ b/valuescript_vm/src/val_storage.rs
@@ -49,15 +49,11 @@ impl Tag {
 
 impl<SB: StorageBackend + 'static> StorageEntity<SB> for Val {
   fn from_storage_entry<Tx: StorageReader<SB>>(
-    tx: &mut Tx,
+    tx: &Tx,
     entry: StorageEntry,
   ) -> Result<Self, GenericError> {
     let mut reader = StorageEntryReader::new(&entry);
     let res = read_from_entry(tx, &mut reader);
-    match &res {
-      Ok(val) => println!("val: {}", val.pretty()),
-      Err(_) => println!("error"),
-    }
     assert!(reader.done());
 
     res
@@ -267,7 +263,7 @@ fn write_ptr_to_entry<SB: StorageBackend + 'static, Tx: StorageTxMut<SB>>(
 }
 
 fn read_from_entry<SB: StorageBackend + 'static, Tx: StorageReader<SB>>(
-  tx: &mut Tx,
+  tx: &Tx,
   reader: &mut StorageEntryReader,
 ) -> Result<Val, GenericError> {
   let tag = Tag::from_byte(reader.read_u8()?)?;
@@ -400,7 +396,7 @@ fn read_symbol_from_entry(reader: &mut StorageEntryReader) -> Result<VsSymbol, B
 }
 
 fn read_ref_bytecode_from_entry<SB: StorageBackend, Tx: StorageReader<SB>>(
-  tx: &mut Tx,
+  tx: &Tx,
   reader: &mut StorageEntryReader,
 ) -> Result<Rc<Bytecode>, GenericError> {
   let ptr = reader.read_ref()?;
diff --git a/valuescript_vm/src/vs_value.rs b/valuescript_vm/src/vs_value.rs
index 8a9cf86..3ed0a65 100644
--- a/valuescript_vm/src/vs_value.rs
+++ b/valuescript_vm/src/vs_value.rs
@@ -368,6 +368,7 @@ impl ValTrait for Val {
       Function(f) => LoadFunctionResult::StackFrame(f.make_frame()),
       Static(s) => s.load_function(),
       Dynamic(val) => val.load_function(),
+      StoragePtr(ptr) => ptr.get().load_function(),
 
       _ => LoadFunctionResult::NotAFunction,
     };
diff --git a/vstc/src/db_command.rs b/vstc/src/db_command.rs
index fe6e8fd..d88908f 100644
--- a/vstc/src/db_command.rs
+++ b/vstc/src/db_command.rs
@@ -1,6 +1,6 @@
 use std::{process::exit, rc::Rc};
 
-use storage::{storage_head_ptr, SledBackend, Storage};
+use storage::{storage_head_ptr, SledBackend, Storage, StorageReader};
 use valuescript_compiler::asm;
 use valuescript_vm::{
   vs_value::{ToVal, Val},