Apply review suggestions: lazy locals in invoke_exact_args, hoist mapping(), avoid redundant locals clone in setup_annotations

youknowone · youknowone · commit 35e839406962 · 2026-03-02T18:04:26.000+09:00
diff --git a/benches/microbenchmarks.rs b/benches/microbenchmarks.rs
@@ -135,7 +135,7 @@ fn bench_rustpython_code(group: &mut BenchmarkGroup<WallTime>, bench: &MicroBenc
                 scope
                     .locals
                     .as_ref()
-                    .unwrap()
+                    .expect("new_scope_with_builtins always provides locals")
                     .as_object()
                     .set_item("ITERATIONS", vm.new_pyobj(idx), vm)
                     .expect("Error adding ITERATIONS local variable");
diff --git a/crates/vm/src/builtins/function.rs b/crates/vm/src/builtins/function.rs
@@ -628,11 +628,9 @@ impl Py<PyFunction> {
     pub fn invoke_exact_args(&self, args: &[PyObjectRef], vm: &VirtualMachine) -> PyResult {
         let code = self.code.lock().clone();
 
-        let locals = ArgMapping::from_dict_exact(vm.ctx.new_dict());
-
         let frame = Frame::new(
             code.clone(),
-            Scope::new(Some(locals), self.globals.clone()),
+            Scope::new(None, self.globals.clone()),
             self.builtins.clone(),
             self.closure.as_ref().map_or(&[], |c| c.as_slice()),
             Some(self.to_owned().into()),
diff --git a/crates/vm/src/frame.rs b/crates/vm/src/frame.rs
@@ -460,11 +460,12 @@ impl Frame {
         let code = &**self.code;
         // SAFETY: Called before generator resume; no concurrent access.
         let fastlocals = unsafe { self.fastlocals.borrow_mut() };
+        let locals_map = self.locals.mapping(vm);
         for (i, &varname) in code.varnames.iter().enumerate() {
             if i >= fastlocals.len() {
                 break;
             }
-            match self.locals.mapping(vm).subscript(varname, vm) {
+            match locals_map.subscript(varname, vm) {
                 Ok(value) => fastlocals[i] = Some(value),
                 Err(e) if e.fast_isinstance(vm.ctx.exceptions.key_error) => {}
                 Err(e) => return Err(e),
@@ -483,12 +484,13 @@ impl Frame {
         let code = &**self.code;
         let map = &code.varnames;
         let j = core::cmp::min(map.len(), code.varnames.len());
+        let locals_map = locals.mapping(vm);
         if !code.varnames.is_empty() {
             // SAFETY: Either _guard holds the state mutex (frame not executing),
             // or we're in a trace callback on the same thread that holds it.
             let fastlocals = unsafe { self.fastlocals.borrow() };
             for (&k, v) in zip(&map[..j], fastlocals) {
-                match locals.mapping(vm).ass_subscript(k, v.clone(), vm) {
+                match locals_map.ass_subscript(k, v.clone(), vm) {
                     Ok(()) => {}
                     Err(e) if e.fast_isinstance(vm.ctx.exceptions.key_error) => {}
                     Err(e) => return Err(e),
@@ -498,7 +500,7 @@ impl Frame {
         if !code.cellvars.is_empty() || !code.freevars.is_empty() {
             for (i, &k) in code.cellvars.iter().enumerate() {
                 let cell_value = self.get_cell_contents(i);
-                match locals.mapping(vm).ass_subscript(k, cell_value, vm) {
+                match locals_map.ass_subscript(k, cell_value, vm) {
                     Ok(()) => {}
                     Err(e) if e.fast_isinstance(vm.ctx.exceptions.key_error) => {}
                     Err(e) => return Err(e),
@@ -507,7 +509,7 @@ impl Frame {
             if code.flags.contains(bytecode::CodeFlags::OPTIMIZED) {
                 for (i, &k) in code.freevars.iter().enumerate() {
                     let cell_value = self.get_cell_contents(code.cellvars.len() + i);
-                    match locals.mapping(vm).ass_subscript(k, cell_value, vm) {
+                    match locals_map.ass_subscript(k, cell_value, vm) {
                         Ok(()) => {}
                         Err(e) if e.fast_isinstance(vm.ctx.exceptions.key_error) => {}
                         Err(e) => return Err(e),
@@ -4324,18 +4326,16 @@ impl ExecutingFrame<'_> {
     #[cold]
     fn setup_annotations(&mut self, vm: &VirtualMachine) -> FrameResult {
         let __annotations__ = identifier!(vm, __annotations__);
+        let locals_obj = self.locals.as_object(vm);
         // Try using locals as dict first, if not, fallback to generic method.
-        let has_annotations = match self.locals.into_object(vm).downcast_exact::<PyDict>(vm) {
-            Ok(d) => d.contains_key(__annotations__, vm),
-            Err(o) => {
-                let needle = __annotations__.as_object();
-                self._in(vm, needle, &o)?
-            }
-        };
+        let has_annotations =
+            if let Some(d) = locals_obj.downcast_ref_if_exact::<PyDict>(vm) {
+                d.contains_key(__annotations__, vm)
+            } else {
+                self._in(vm, __annotations__.as_object(), locals_obj)?
+            };
         if !has_annotations {
-            self.locals
-                .as_object(vm)
-                .set_item(__annotations__, vm.ctx.new_dict().into(), vm)?;
+            locals_obj.set_item(__annotations__, vm.ctx.new_dict().into(), vm)?;
         }
         Ok(None)
     }
