fix: remove unncessary parameter cloning

ristretto_vm/src/frame.rs

@@ -48,24 +48,17 @@ pub struct Frame {
     thread: Weak<Thread>,
     class: Arc<Class>,
     method: Arc<Method>,
-    parameters: Vec<Value>,
     program_counter: AtomicUsize,
 }
 
 impl Frame {
     /// Create a new frame for the specified class. To invoke a method on an object reference, the
     /// object reference must be the first parameter in the parameters vector.
-    pub fn new(
-        thread: &Weak<Thread>,
-        class: &Arc<Class>,
-        method: &Arc<Method>,
-        parameters: Vec<Value>,
-    ) -> Self {
+    pub fn new(thread: &Weak<Thread>, class: &Arc<Class>, method: &Arc<Method>) -> Self {
         Frame {
             thread: thread.clone(),
             class: class.clone(),
             method: method.clone(),
-            parameters,
             program_counter: AtomicUsize::new(0),
         }
     }
@@ -108,13 +101,10 @@ impl Frame {
     /// * if the program counter is invalid
     /// * if an invalid instruction is encountered
     #[async_recursion(?Send)]
-    pub async fn execute(&self) -> Result<Option<Value>> {
+    pub async fn execute(&self, mut parameters: Vec<Value>) -> Result<Option<Value>> {
         let max_locals = self.method.max_locals();
-        // TODO: Implement the local variable from parameters to avoid cloning?
-        let locals = &mut LocalVariables::with_max_size(max_locals);
-        for (index, parameter) in self.parameters.iter().enumerate() {
-            locals.set(index, parameter.clone())?;
-        }
+        Frame::adjust_parameters(&mut parameters, max_locals);
+        let locals = &mut LocalVariables::new(parameters);
         let max_stack = self.method.max_stack();
         let stack = &mut OperandStack::with_max_size(max_stack);
         let code = self.method.code();
@@ -151,6 +141,26 @@ impl Frame {
         }
     }
 
+    /// The JVM specification requires that Long and Double take two places in the parameters list
+    /// when passed to a method. This method adjusts the parameters list to account for this.
+    ///
+    /// See: <https://docs.oracle.com/javase/specs/jvms/se23/html/jvms-2.html#jvms-2.6.1>
+    fn adjust_parameters(parameters: &mut Vec<Value>, max_size: usize) {
+        let mut index = parameters.len();
+        while index > 0 {
+            index -= 1;
+            match &parameters[index] {
+                Value::Long(_) | Value::Double(_) => {
+                    parameters.insert(index + 1, Value::Unused);
+                }
+                _ => {}
+            }
+        }
+        if parameters.len() < max_size {
+            parameters.resize(max_size, Value::Unused);
+        }
+    }
+
     /// Debug the execution of an instruction in this frame
     fn debug_execute(
         &self,
@@ -452,9 +462,33 @@ mod tests {
         let (thread, class) = get_class("Expressions").await?;
         let method = class.method("add", "(II)I").expect("method not found");
         let parameters = vec![Value::Int(1), Value::Int(2)];
-        let frame = Frame::new(&Arc::downgrade(&thread), &class, &method, parameters);
-        let result = frame.execute().await?;
+        let frame = Frame::new(&Arc::downgrade(&thread), &class, &method);
+        let result = frame.execute(parameters).await?;
         assert!(matches!(result, Some(Value::Int(3))));
         Ok(())
     }
+
+    #[test]
+    fn test_adjust_parameters() {
+        let mut parameters = vec![
+            Value::Int(1),
+            Value::Long(2),
+            Value::Float(3.0),
+            Value::Double(4.0),
+        ];
+        Frame::adjust_parameters(&mut parameters, 8);
+        assert_eq!(
+            parameters,
+            vec![
+                Value::Int(1),
+                Value::Long(2),
+                Value::Unused,
+                Value::Float(3.0),
+                Value::Double(4.0),
+                Value::Unused,
+                Value::Unused,
+                Value::Unused,
+            ]
+        );
+    }
 }

ristretto_vm/src/local_variables.rs

@@ -10,11 +10,14 @@ pub struct LocalVariables {
 }
 
 impl LocalVariables {
+    /// Create a new local variables
+    pub fn new(locals: Vec<Value>) -> Self {
+        LocalVariables { locals }
+    }
+
     /// Create a new local variables with a maximum size.
     pub fn with_max_size(max_size: usize) -> Self {
-        LocalVariables {
-            locals: vec![Value::Unused; max_size],
-        }
+        Self::new(vec![Value::Unused; max_size])
     }
 
     /// Get a value from the local variables.

ristretto_vm/src/test.rs

@@ -45,7 +45,6 @@ pub(crate) async fn class() -> Result<(Arc<VM>, Arc<Thread>, Arc<Class>)> {
 pub(crate) async fn frame() -> Result<(Arc<VM>, Arc<Thread>, Frame)> {
     let (vm, thread, class) = class().await?;
     let method = class.try_get_method("test", "()V")?;
-    let parameters = Vec::new();
-    let frame = Frame::new(&Arc::downgrade(&thread), &class, &method, parameters);
+    let frame = Frame::new(&Arc::downgrade(&thread), &class, &method);
     Ok((vm, thread, frame))
 }

ristretto_vm/src/thread.rs

@@ -282,8 +282,7 @@ impl Thread {
             }
             .into());
         } else {
-            let parameters = Thread::adjust_parameters(parameters);
-            let frame = Arc::new(Frame::new(&self.thread, class, method, parameters));
+            let frame = Arc::new(Frame::new(&self.thread, class, method));
 
             // Limit the scope of the write lock to just adding the frame to the thread. This
             // is necessary because java.lang.Thread (e.g. countStackFrames) needs to be able to
@@ -292,7 +291,7 @@ impl Thread {
                 let mut frames = self.frames.write().await;
                 frames.push(frame.clone());
             }
-            let result = frame.execute().await;
+            let result = frame.execute(parameters).await;
             (result, true)
         };
 
@@ -340,24 +339,6 @@ impl Thread {
         }
     }
 
-    /// The JVM specification requires that Long and Double take two places in the parameters list
-    /// when passed to a method. This method adjusts the parameters list to account for this.
-    ///
-    /// See: <https://docs.oracle.com/javase/specs/jvms/se23/html/jvms-2.html#jvms-2.6.1>
-    fn adjust_parameters(mut parameters: Vec<Value>) -> Vec<Value> {
-        let mut index = parameters.len();
-        while index > 0 {
-            index -= 1;
-            match &parameters[index] {
-                Value::Long(_) | Value::Double(_) => {
-                    parameters.insert(index + 1, Value::Unused);
-                }
-                _ => {}
-            }
-        }
-        parameters
-    }
-
     /// Create a new VM Object by invoking the constructor of the specified class.
     ///
     /// # Errors
@@ -399,7 +380,7 @@ impl Thread {
 mod tests {
     use super::*;
     use crate::ConfigurationBuilder;
-    use ristretto_classloader::{ClassPath, Value};
+    use ristretto_classloader::ClassPath;
     use std::path::PathBuf;
 
     fn classes_jar_path() -> PathBuf {
@@ -457,28 +438,6 @@ mod tests {
         Ok(())
     }
 
-    #[test]
-    fn test_adjust_parameters() {
-        let parameters = vec![
-            Value::Int(1),
-            Value::Long(2),
-            Value::Float(3.0),
-            Value::Double(4.0),
-        ];
-        let adjusted_parameters = Thread::adjust_parameters(parameters);
-        assert_eq!(
-            adjusted_parameters,
-            vec![
-                Value::Int(1),
-                Value::Long(2),
-                Value::Unused,
-                Value::Float(3.0),
-                Value::Double(4.0),
-                Value::Unused,
-            ]
-        );
-    }
-
     #[tokio::test]
     async fn test_new_object_integer() -> Result<()> {
         let vm = test_vm().await?;