From 77195ba93a875567d101c6a2d7b6ba28504602a0 Mon Sep 17 00:00:00 2001 From: brianheineman Date: Tue, 21 Jan 2025 17:20:51 -0700 Subject: [PATCH] fix: remove unncessary parameter cloning --- ristretto_vm/src/frame.rs | 66 ++++++++++++++++++++++------- ristretto_vm/src/local_variables.rs | 9 ++-- ristretto_vm/src/test.rs | 3 +- ristretto_vm/src/thread.rs | 47 ++------------------ 4 files changed, 60 insertions(+), 65 deletions(-) diff --git a/ristretto_vm/src/frame.rs b/ristretto_vm/src/frame.rs index 876109b..28bcad0 100644 --- a/ristretto_vm/src/frame.rs +++ b/ristretto_vm/src/frame.rs @@ -48,24 +48,17 @@ pub struct Frame { thread: Weak, class: Arc, method: Arc, - parameters: Vec, 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, - class: &Arc, - method: &Arc, - parameters: Vec, - ) -> Self { + pub fn new(thread: &Weak, class: &Arc, method: &Arc) -> 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> { + pub async fn execute(&self, mut parameters: Vec) -> Result> { 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: + fn adjust_parameters(parameters: &mut Vec, max_size: usize) { + let mut index = parameters.len(); + while index > 0 { + index -= 1; + match ¶meters[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, + ] + ); + } } diff --git a/ristretto_vm/src/local_variables.rs b/ristretto_vm/src/local_variables.rs index 89e1a09..c7acfbd 100644 --- a/ristretto_vm/src/local_variables.rs +++ b/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) -> 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. diff --git a/ristretto_vm/src/test.rs b/ristretto_vm/src/test.rs index 605c221..558bb76 100644 --- a/ristretto_vm/src/test.rs +++ b/ristretto_vm/src/test.rs @@ -45,7 +45,6 @@ pub(crate) async fn class() -> Result<(Arc, Arc, Arc)> { pub(crate) async fn frame() -> Result<(Arc, Arc, 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)) } diff --git a/ristretto_vm/src/thread.rs b/ristretto_vm/src/thread.rs index fac9f00..eb3e8a8 100644 --- a/ristretto_vm/src/thread.rs +++ b/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: - fn adjust_parameters(mut parameters: Vec) -> Vec { - let mut index = parameters.len(); - while index > 0 { - index -= 1; - match ¶meters[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?;